pact-foundation / pact-go

Golang version of Pact. Pact is a contract testing framework for HTTP APIs and non-HTTP asynchronous messaging systems.
http://pact.io
MIT License
828 stars 103 forks source link

Add function to expose the NativeLibPath #376

Closed becheran closed 3 months ago

becheran commented 3 months ago

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.

mefellows commented 3 months ago

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 commented 3 months ago

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 commented 3 months ago

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 commented 3 months ago

@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

mefellows commented 3 months ago

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 commented 3 months ago

@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 commented 3 months ago

@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?

mefellows commented 3 months ago

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 commented 3 months ago

Coverage Status

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

becheran commented 3 months ago

@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 commented 3 months ago

Thanks!