Update Makefile: Ensure make install respects DESTDIR and PREFIX, and Add make dist target for source tarball creation#34
Conversation
Summary of ChangesHello @rmiki-dev, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request enhances the project's build and packaging capabilities by improving the Highlights
🧠 New Feature in Public Preview: You can now enable Memory to help Gemini Code Assist learn from your team's feedback. This makes future code reviews more consistent and personalized to your project's style. Click here to enable Memory in your admin console. Changelog
Activity
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
There was a problem hiding this comment.
Code Review
This pull request introduces two main improvements to the Makefile: correctly handling DESTDIR and PREFIX for staging installations, and adding a dist target to create source tarballs for packaging. The changes are well-implemented. My only suggestion is to avoid hardcoding the version number in the Makefile to improve long-term maintainability, especially for the packaging use case mentioned.
|
git provides a handy tool for creating archive files that you might want to consider using: git-archive. The nice thing about git-archive is that you don't have to explicitly list files to tar up so things stay in sync automatically when files are added or removed from the repo. Here's an example of how it's done: https://github.com/fido-device-onboard/go-fdo-server/blob/cf025fe5c72f78de1a3cc783aa15e718200f826f/Makefile#L56 |
20ead63 to
41b56ca
Compare
Signed-off-by: Rosy-Glorious Miki <rmiki@redhat.com>
41b56ca to
f7adae8
Compare
| .PHONY: all build clean fmt install lint test tools unit-test integration-test validate .install.golangci-lint | ||
| PROJECT := tar-diff | ||
| VERSION := $(shell go run ./cmd/tar-diff -version | cut -d' ' -f2) | ||
| PROJDIR := $(PROJECT).$(VERSION) |
There was a problem hiding this comment.
The PROJDIR variable uses a dot, but git archive --prefix uses a hyphen. Should not it be unified, please? Anyway, this variable is not used so it seems that it could be removed.
There was a problem hiding this comment.
I didn't notice, thank you. I'll have the folder match the file name. The PROJDIR var is currently being used in the clean target
There was a problem hiding this comment.
I think that the standard convention is to use a hyphen for name-version formatting so I would recommend reflecting this in the PROJDIR, PROJ_TARBALL variable and in the git archive --prefix command.
Yes, the PROJDIR variable is used only in the clean target so I do not see a reason why it could not be removed. I am very sorry for my previous inaccurate comment ("this variable is not used").
There was a problem hiding this comment.
I have switched to a hyphen for name-version, and took out PROJDIR since it is only used once.
There was a problem hiding this comment.
I see an underscore used for name-version formatting instead of a hyphen. Is that intentional, please?
knecasov
left a comment
There was a problem hiding this comment.
I conducted the code review with AI assistance.
2b9af6c to
47b1c7b
Compare
mmartinv
left a comment
There was a problem hiding this comment.
A couple of nitpicks, feel free to address them if you want. Either case it LGTM
Signed-off-by: Rosy-Glorious Miki <rmiki@redhat.com>
47b1c7b to
2b9dd89
Compare
|
🎉 This PR is included in version 0.2.0 🎉 The release is available on GitHub release Your semantic-release bot 📦🚀 |
make disttarget that produces a distributable source tarball. This is needed for Fedora/RPM packaging workflows where source tarballs are the standard input format