Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add function to expose the NativeLibPath #376

Merged
merged 1 commit into from
Feb 13, 2024

Conversation

becheran
Copy link
Contributor

Not sure if you want this in, but I was using pact on Windows and there the default path "user/local/lib" is most likely never right. I am using TDM GCC which is installed at C:\TDM-GCC-64\bin.

With the powershell Get-Command it is possible to detect the install path and put the library in there directly which is a better default on Windows I guess since otherwsie one would set this path manually.

@@ -135,6 +135,13 @@ func (i *Installer) getLibDir() string {
return env
}

if runtime.GOOS == "windows" {
if out, err := exec.Command("powershell", "-Command", "(Get-Command gcc).Source").Output(); err == nil && len(out) > 0 {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Does this error need handling?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not sure. I guess the only sane thing I could do in this case is log the error. What do you think? Is it worth logging? The reason might simpoly be that gcc is not found on the system.

@mefellows
Copy link
Member

Thanks for the PR!

Anything to improve the developer experience is welcome. I think it could also just be dumped into the "current directory" in windows (I can't recall the exact DLL loading semantics, but I recall it looking up the tree from where you are).

@becheran
Copy link
Contributor Author

Thanks for the PR!

Anything to improve the developer experience is welcome. I think it could also just be dumped into the "current directory" in windows (I can't recall the exact DLL loading semantics, but I recall it looking up the tree from where you are).

What do you mean with "current directory"? The dll loading semantics depend on where your gcc is installed on windows AFIK.

@mefellows
Copy link
Member

The search order for DLLs in unpackaged apps is defined here: https://learn.microsoft.com/en-us/windows/win32/dlls/dynamic-link-library-search-order#search-order-for-unpackaged-apps

See point (11) here.

@becheran
Copy link
Contributor Author

@mefellows I need to experiment a bit more before merge. I still don't fully understand what is going on on Windows. It only works for me on my machine, when I have the .dll in C:\TDM-GCC-64\bin AND C:\usr\local\lib.

Don't really know why this is the case yet. If I only put it in C:\usr\local\lib the program will crash with exit code exit status 0xc0000135 if I only put it in C:\TDM-GCC-64\bin I will see this message: C:\Program Files\Go\pkg\tool\windows_amd64\link.exe: running gcc failed: exit status 1 C:/TDM-GCC-64/bin/../lib/gcc/x86_64-w64-mingw32/10.3.0/../../../../x86_64-w64-mingw32/bin/ld.exe: cannot find -lpact_ffi collect2.exe: error: ld returned 1 exit status

@becheran becheran marked this pull request as draft January 24, 2024 08:44
@mefellows
Copy link
Member

It could be because we don't set any additional flags on the windows CGO stanza here: https://github.com/pact-foundation/pact-go/blob/master/internal/native/lib.go#L7. You could try experimenting with that, and if there are universal paths that are both easily written to during install and static they could go there.

But probably, it's just the way LD resolves DLLs on Windows. If you know where the common search paths are, that should resolve it.

Note the cannot find -lpact_ffi part of the message, that basically says it doesn't know where the DLL is and didn't find it during resolution. I could never get Windows to print out the DLL search path as it tried to load things, despite numerous articles attempting to show how to do it. On linux systems it's a single env var and boom, you get the full trace (how does anyone use Windows ;) ).

@becheran
Copy link
Contributor Author

@mefellows you are absolutely right. If one can avoid it, it is best to not use Windows at all. Especially not with go and cgo including gcc. It is a total mess.

I kind of understood now what is going on. The gcc compiler that I have on my system seems to expect lib files in /usr/local/lib which is the default for gcc and makes sense on unix. Later at runtime though the dll is not found anymore and is expected to be loaded with the mechanisms you describe. (see also https://learn.microsoft.com/en-us/windows/win32/dlls/dynamic-link-library-search-order#search-order-for-unpackaged-apps).

When I change the linker to #cgo windows,amd64 LDFLAGS: -LC:\TDM-GCC-64\bin -lpact_ffi in the lib.go file it works. Though I don't think that "C:\TDM-GCC-64\bin" is a good default for everybody.

@becheran becheran changed the title Try to autodetect windows install path Add function to expose the NativeLibPath Feb 2, 2024
@becheran
Copy link
Contributor Author

becheran commented Feb 2, 2024

@mefellows I finally found something more or less satisfying which works on Windows. I ended up putting the dll next to the lib.go path of this library and in the gcc binary folder. So it is duplicated, but I found no other solution which workes out of the box without requireing to configure the windows system and set some variables or putting the -LC:\TDM-GCC-64\bin path to the linker args.

In the end I have this small go binary in our codebase which can be executed via go generate and will automatically setup the dll on the system. So to setup a new dev environment you only need to run go generate once. It looks like this:

package main

import (
	"fmt"
	"os/exec"
	"path/filepath"
	"runtime"
	"strings"

	"github.com/pact-foundation/pact-go/v2/installer"
)

//go:generate go run ./...
func main() {
	fmt.Printf("Download and install %s if not yet installed\n", installer.FFIPackage)
	if runtime.GOOS != "windows" {
		panic("Only works on windows right now")
	}

	fmt.Println("Try to detect gcc environment")
	gccDir := ""
	if out, err := exec.Command("powershell", "-Command", "(Get-Command gcc).Source").Output(); err == nil && len(out) > 0 {
		gccDir = filepath.Dir(strings.TrimSpace(string(out)))
	} else {
		panic("Failed to get GCC path. Make sure that gcc is installed on your system.")
	}

	fmt.Println("Get path to pact lib")

	assertInstalled(gccDir)
	assertInstalled(installer.NativeLibPath())
}

func assertInstalled(dir string) {
	fmt.Printf("Assert that dll is installed at %s\n", dir)
	i, err := installer.NewInstaller()
	if err != nil {
		panic(err)
	}
	i.SetLibDir(dir)
	if err := i.CheckInstallation(); err != nil {
		panic(err)
	}
}

In this PR I would like to add NativeLibPath function. I need it to be somewhere where the dll is not yet loaded. Thats why I put it in the installer package. What do you think? Is this something that could be useful for others as well?

@becheran becheran marked this pull request as ready for review February 2, 2024 08:02
@mefellows
Copy link
Member

mefellows commented Feb 5, 2024

Absolutely, I'll merge that in. Would you mind please rebasing your commits with just the single feat as the commit message?

We could potentially look up update the pact-go install command to take multiple locations to do the install also, if needed? That should remove the need for your go generate function.

@coveralls
Copy link

Coverage Status

coverage: 37.248% (+0.05%) from 37.2%
when pulling f6eca29 on becheran:install_path_windows
into 7acfdce on pact-foundation:master.

@becheran becheran force-pushed the install_path_windows branch from f6eca29 to 6cdebf8 Compare February 5, 2024 07:23
feat: add native lib path function
@becheran becheran force-pushed the install_path_windows branch from 6cdebf8 to 47fcff6 Compare February 5, 2024 07:31
@becheran
Copy link
Contributor Author

becheran commented Feb 5, 2024

@mefellows I squashed my changes to a single feat commit.

By using go generate I want to avoid the use of the pact-go install tool. The simple reason for that is that the binary that I install via go install (go version > 1.19) might be a differnt one that is used by the module via go get. I don't want to handle the confusion of the dll beeing in the directory of pact-go of the binary, but not in the pact-go library that is actually used by my module.

For example when I installed the pact-go binary it ended up in another directory and I would need to make sure that the binary and library versions are somehow synced to end up with the same lib paths.

image

@mefellows mefellows merged commit f893818 into pact-foundation:master Feb 13, 2024
1 check passed
@mefellows
Copy link
Member

Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants