-
Notifications
You must be signed in to change notification settings - Fork 15
refactor: switch to standard Go vendoring for cross-distro packaging #100
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
base: main
Are you sure you want to change the base?
Conversation
607ffa5 to
8ce70d9
Compare
|
/packit rebuild |
fe8b57a to
d03317d
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
devskim found more than 20 potential problems in the proposed changes. Check the Files changed tab for more details.
80949ce to
1911209
Compare
ben-krieger
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Personally, I hate vendoring. Before Go modules, it was the best way (at least before dep), but now it's a huge pain.
- It requires devs to remember an extra step before pushing
- It significantly slows down git clones and gets much worse over time with no great ways to fix
- It makes GitHub reviews much worse for humans
On the plus side, it makes building with Nix easier.
At a bare minimum, the gitattributes must be changed so GitHub doesn't show vendored files in the diffs. If review pages start failing to load due to diff size (especially on mobile), it will be hard for me to help.
1911209 to
33551c5
Compare
I looked at different options without using EPEL and this seems to be a standard practice used by other projects for supporting both Fedora and Centos in a single spec file.
The extra step should only be needed when dependencies are added or updated. We can mitigate this by adding a pre-commit hook that validates vendor/ is in sync and/or a CI check that will fail if not.
A definite drawback. I think cross distro compatibility is worth it, but I'm happy to explore other options if you have suggestions.
I've added gitattributes to exclude vendor files from diffs, which should help the review experience. Thanks for the suggestion. |
Replace go-vendor-tools with standard Go module vendoring to support building on both Fedora and CentOS/RHEL with a single spec file. Changes: - Use `go mod vendor` + `tar` to create separate vendor tarball at release time via packit's create-archive action - Source0: project source from git archive (no vendor/) - Source1: vendor tarball created by `go mod vendor` - Remove go-vendor-tools dependency (EPEL-only package) - Remove %generate_buildrequires and %go_generate_buildrequires macros (not needed when vendoring dependencies) - Fix SOURCE references for sysusers files - Add vendor/ to .gitignore Signed-off-by: Paul Whalen <[email protected]>
33551c5 to
69cd212
Compare
mmartinv
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think the only thing I am missing is the replacement of the licensing macros, maybe we can use the tools externally to generate the files and include them in the spec.
| %setup -q -T -D -a1 %{forgesetupargs} | ||
| #%%autopatch -p1 | ||
|
|
||
| %generate_buildrequires |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Shouldn't we replace this with %go_generate_buildrequires from the go-rpm-macros package? not sure if that would work though
| Source0: %{gosource} | ||
| # Generated by go-vendor-tools | ||
| Source1: %{archivename}-vendor.tar.bz2 | ||
| Source2: go-vendor-tools.toml |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Although we are not going to use the go-vendor-tools.toml file from within the spec file, it might worth it to maintain it in the sources so it's possible to generate the vendor tarball externally with go-vendor-tools. ??
| command -v go_vendor_archive || sudo dnf install -y go-vendor-tools python3-tomlkit; \ | ||
| go_vendor_archive create --config $(GO_VENDOR_TOOLS_FILE) --write-config --output $(VENDOR_TARBALL) .; \ | ||
| rm -rf vendor; | ||
| go mod vendor; \ | ||
| tar -cjf $(VENDOR_TARBALL) vendor/; \ | ||
| rm -rf vendor |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The problem is the spec file depending on go-vendor-tools, not its use in the Makefile. We can keep the current vendor tarball generation I believe
Replace go-vendor-tools with standard Go module vendoring to support
building on both Fedora and CentOS/RHEL with a single spec file.
Changes:
package required by //go:embed directives for build-time compilation
CI workflow updates:
in third-party dependencies
vendored code issues
dependencies don't require go.sum
This approach follows the standard pattern used by major Go packages
in Fedora and ensures compatibility across distributions.