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

Library doesn't build when using mod=vendor #30

Closed
keisar opened this issue Apr 21, 2020 · 4 comments
Closed

Library doesn't build when using mod=vendor #30

keisar opened this issue Apr 21, 2020 · 4 comments

Comments

@keisar
Copy link
Contributor

keisar commented Apr 21, 2020

When using vendor mode .h files are not copied to the vendor directory because they are in a subdirectory, according to cgo's documentation (https://golang.org/cmd/cgo/) we should not put non-go files in subdirectories.

I have created a fork with all .h files moved from the /include/ subdirectory to the parent library (keisar#1), after these changes the library works in vendor mode.
If all agree I will create the PR here as well.

Another possible solution is to follow this approach: https://github.com/godror/godror/pull/38/files
which means creating dummy go files in the /include subdirectories and importing them from h3.go.
If you prefer I can send a PR for this approach as well.

Related issue: #16

@jogly
Copy link
Collaborator

jogly commented Apr 21, 2020

Good find! As you identified, CGO requires non-go files that should be compiled into the binary to be in the same package; this is the reason why the h3_*.c files are in the same directory as h3.go. Other vendoring tools included include/ directory when vendoring, allowing us to keep the header files separated because they're only used for building, but not actually included in the final binary. So this surprised me because we've seen this library used in vendored environments before. Now I see (golang/go#26366), go mod is depending further on the recommendation to include non-go files at the same level. So we'll grudgingly comply! :D

We'd love your contribution to fix this! I checked out keisar#1 - looks great! My only request is you follow the same naming pattern as the h3_*.c files for the header files, e.g. h3_*.h

@keisar
Copy link
Contributor Author

keisar commented Apr 22, 2020

Hi @joegilley
Great! I will send the PR later today.

There is one issue with changing the header files names, it means it also needs to be changed in the C files (the #include statements), I can alter the update-h3.sh script to modify the C files while copying them but are you sure about this approach?

@keisar
Copy link
Contributor Author

keisar commented Apr 22, 2020

@joegilley I have sent the PR (first one had some issues with CLA so had to recreate).
Please note my comments in the PR description about some manual work that needs to be done after running update-h3.sh.
If you think it would be wise I can open a PR in the Core Library to fix this as well.

If the current implementation in update-h3.sh is too fragile we can always revert the last commit and leave the header files without the "h3_" prefix.

@jogly
Copy link
Collaborator

jogly commented May 19, 2020

included in v3.0.2, thanks @keisar !!!

@jogly jogly closed this as completed May 19, 2020
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

No branches or pull requests

2 participants