Leon/snapshot string in packages #50
Conversation
Build rpms with make command
WalkthroughRefactors build and packaging to a centralized OUT output directory, embeds build metadata via ldflags, removes the make-version.sh generator, templates package control/spec files with @@Version@@, updates .gitignore to ignore Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor Developer
participant Git
participant Makefile
participant GoBuild as "go build (ldflags)"
participant OUT as out/
participant Packaging
Developer->>Git: commit / tag (optional)
Developer->>Makefile: make build / package
Makefile->>Git: resolve COMMIT (git describe fallback)
Makefile->>GoBuild: invoke with GOFLAGS (-X main.version, -X main.commit, -X main.name)
GoBuild->>OUT: place binary into OUT
Note right of OUT `#DDEEEF`: OUT-centered artifacts and version files
Makefile->>Packaging: substitute @@VERSION@@ and produce tar/rpm/deb from OUT
Packaging->>OUT: emit final artifacts
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes
Possibly related PRs
Suggested reviewers
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 passed)
✨ Finishing touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (2)
rpm/SPECS/dnstapir-cli.spec.in (1)
34-34: Non-standard DESTDIR usage could cause confusion.The current implementation sets
DESTDIR=%{buildroot}%{_bindir}and relies on the Makefile's install target appending$(prefix)(which is undefined/empty). While this works, it's non-standard. The conventional pattern is:DESTDIR=%{buildroot} make installAnd the Makefile's install target should handle the full path:
install: $(INSTALL) $(OUT)/$(PROG) $(DESTDIR)/usr/bin/This makes the build process more maintainable and aligns with standard practices.
Makefile (1)
7-7: Prefer$(CURDIR) over $ $(pwd) for path construction.The current
OUT:=$$(pwd)/outuses double-dollarpwd, which re-evaluates the working directory for each use of$(OUT). Consider using Make's built-in$(CURDIR)variable instead:-OUT:=$$(pwd)/out +OUT:=$(CURDIR)/outThis is more efficient and idiomatic in Makefiles.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (8)
.gitignore(1 hunks)Makefile(1 hunks)README.md(1 hunks)VERSION(0 hunks)deb/DEBIAN/control.in(1 hunks)main.go(1 hunks)make-version.sh(0 hunks)rpm/SPECS/dnstapir-cli.spec.in(2 hunks)
💤 Files with no reviewable changes (2)
- VERSION
- make-version.sh
🧰 Additional context used
🧠 Learnings (3)
📚 Learning: 2025-06-09T08:00:25.912Z
Learnt from: zluudg
Repo: dnstapir/cli PR: 33
File: rpm/SPECS/tapir-cli.spec:0-0
Timestamp: 2025-06-09T08:00:25.912Z
Learning: In the dnstapir/cli project, the RPM spec file version field should retain the 'v' prefix (e.g., "Version: v0.3") rather than following the typical RPM convention of omitting it.
Applied to files:
rpm/SPECS/dnstapir-cli.spec.indeb/DEBIAN/control.in
📚 Learning: 2025-08-14T12:03:40.980Z
Learnt from: zluudg
Repo: dnstapir/cli PR: 41
File: rpm/SPECS/tapir-cli.spec:42-42
Timestamp: 2025-08-14T12:03:40.980Z
Learning: User zluudg prefers not to auto-start or auto-enable systemd services in the dnstapir/cli RPM package, preferring manual activation instead.
Applied to files:
rpm/SPECS/dnstapir-cli.spec.indeb/DEBIAN/control.in
📚 Learning: 2025-08-14T11:59:39.396Z
Learnt from: zluudg
Repo: dnstapir/cli PR: 41
File: rpm/SOURCES/restart-pop.service:11-11
Timestamp: 2025-08-14T11:59:39.396Z
Learning: The restart-pop.service in dnstapir/cli is intended as an optional, manually-activated workaround for restarting tapir-pop.service after certificate renewal. The user (zluudg) prefers to keep tapir-renew.service untouched and is okay with manual activation rather than automatic triggering.
Applied to files:
README.mddeb/DEBIAN/control.in
🪛 checkmake (0.2.2)
Makefile
[warning] 4-4: Target "VERSION" should be declared PHONY.
(phonydeclared)
[warning] 6-6: Target "PROG" should be declared PHONY.
(phonydeclared)
[warning] 7-7: Target "OUT" should be declared PHONY.
(phonydeclared)
[warning] 8-8: Target "COMMIT" should be declared PHONY.
(phonydeclared)
[warning] 9-9: Target "GOFLAGS" should be declared PHONY.
(phonydeclared)
[warning] 11-11: Target "GO" should be declared PHONY.
(phonydeclared)
[warning] 12-12: Target "INSTALL" should be declared PHONY.
(phonydeclared)
[warning] 22-22: Target "all" should be declared PHONY.
(phonydeclared)
[warning] 24-24: Target "default" should be declared PHONY.
(phonydeclared)
[warning] 51-51: Target body for "srpm" exceeds allowed length of 5 (6).
(maxbodylength)
[warning] 62-62: Target body for "deb" exceeds allowed length of 5 (7).
(maxbodylength)
🪛 LanguageTool
README.md
[grammar] ~8-~8: Ensure spelling is correct
Context: ...de. One example of this is the creation anad manipulation of DAWG files containing l...
(QB_NEW_EN_ORTHOGRAPHY_ERROR_IDS_1)
[style] ~13-~13: To reduce wordiness, try specifying a number or using “many” or “numerous” instead.
Context: ...lag --standalone. dnstapir-cli has a large number of commands and subcommands. The entire se...
(LARGE_NUMBER_OF)
🪛 markdownlint-cli2 (0.18.1)
README.md
19-19: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
43-43: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
🔇 Additional comments (8)
main.go (1)
8-17: LGTM! Build metadata implementation is correct.The build-time variable injection approach using ldflags (configured in the Makefile) is a standard Go practice. The "BAD-BUILD" defaults serve as appropriate fallback values if the binary is built without the proper ldflags.
rpm/SPECS/dnstapir-cli.spec.in (2)
1-2: LGTM! Standard debug package disabling.The global directive to disable debug package building is a standard RPM practice for binaries that don't benefit from separate debug symbols.
5-5: Version placeholder conflicts with established convention.According to the learning from PR #33, the RPM spec version field should retain the 'v' prefix (e.g., "Version: v0.3"). However, the Makefile substitution will produce versions like "0.0.0^20251119.abc123" without the 'v' prefix. Please verify whether the 'v' prefix convention should be maintained or if it has been intentionally changed.
Based on learnings
README.md (1)
1-69: LGTM! Rebranding is consistent and complete.The rebranding from
tapir-clitodnstapir-clihas been applied consistently throughout the documentation, including command examples, configuration paths, and service references.deb/DEBIAN/control.in (1)
3-3: LGTM! Version placeholder aligns with new build system.The change from a static version to a placeholder enables build-time version substitution, consistent with the RPM spec template and the broader packaging refactor in this PR.
.gitignore (1)
44-49: LGTM! Gitignore updates align with new build system.The changes correctly:
- Track the
.intemplate file instead of the generatedcontrolfile- Exclude the new
out/directory used for build artifactsThese align with the template-based packaging and OUT directory changes introduced in the Makefile.
Makefile (2)
1-4: LGTM! Centralized version constant.The VERSION constant at 0.0.0 serves as a base version, with snapshot versioning logic (lines 15-20) producing appropriate package versions for development builds. This approach centralizes version management as indicated by the header comments.
15-20: LGTM! Snapshot versioning logic is appropriate.The conditional logic correctly generates snapshot versions for development builds (when VERSION is 0.0.0) while preserving explicit versions for releases. The format conventions (^ for RPM, +local for DEB) are appropriate for each package type.
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
README.md (1)
19-19: Add language specifiers to fenced code blocks.Lines 19 and 43 have fenced code blocks without language specifiers. Add
bashor the appropriate language to improve readability and enable syntax highlighting.-``` +```bash dnstapir-cli pop ping -hand:
-``` +```bash dnstapir-cli -hAlso applies to: 43-43
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
Makefile(1 hunks)README.md(1 hunks)
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-08-14T11:59:39.396Z
Learnt from: zluudg
Repo: dnstapir/cli PR: 41
File: rpm/SOURCES/restart-pop.service:11-11
Timestamp: 2025-08-14T11:59:39.396Z
Learning: The restart-pop.service in dnstapir/cli is intended as an optional, manually-activated workaround for restarting tapir-pop.service after certificate renewal. The user (zluudg) prefers to keep tapir-renew.service untouched and is okay with manual activation rather than automatic triggering.
Applied to files:
README.md
🪛 checkmake (0.2.2)
Makefile
[warning] 51-51: Target body for "srpm" exceeds allowed length of 5 (6).
(maxbodylength)
[warning] 62-62: Target body for "deb" exceeds allowed length of 5 (7).
(maxbodylength)
[warning] 4-4: Target "VERSION" should be declared PHONY.
(phonydeclared)
[warning] 6-6: Target "PROG" should be declared PHONY.
(phonydeclared)
[warning] 7-7: Target "OUT" should be declared PHONY.
(phonydeclared)
[warning] 8-8: Target "COMMIT" should be declared PHONY.
(phonydeclared)
[warning] 9-9: Target "GOFLAGS" should be declared PHONY.
(phonydeclared)
[warning] 11-11: Target "GO" should be declared PHONY.
(phonydeclared)
[warning] 12-12: Target "INSTALL" should be declared PHONY.
(phonydeclared)
[warning] 22-22: Target "all" should be declared PHONY.
(phonydeclared)
[warning] 24-24: Target "default" should be declared PHONY.
(phonydeclared)
🪛 LanguageTool
README.md
[style] ~13-~13: To reduce wordiness, try specifying a number or using “many” or “numerous” instead.
Context: ...lag --standalone. dnstapir-cli has a large number of commands and subcommands. The entire se...
(LARGE_NUMBER_OF)
🪛 markdownlint-cli2 (0.18.1)
README.md
19-19: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
43-43: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
🔇 Additional comments (3)
Makefile (3)
57-57: Clarify the$(outdir)variable usage.Line 57 references
$(outdir), which is not defined in the Makefile. This appears to be an optional user-provided parameter (e.g.,make srpm outdir=/some/path), but it's undocumented. Add a comment or documentation explaining this pattern, or verify it's the intended behavior.- test -z "$(outdir)" || cp $(OUT)/$(PROG)-$(RPM_VERSION)-*.src.rpm "$(outdir)" + # Optional: copy to user-provided outdir if specified (e.g., make srpm outdir=/path) + test -z "$(outdir)" || cp $(OUT)/$(PROG)-$(RPM_VERSION)-*.src.rpm "$(outdir)"
14-20: Conditional versioning for development snapshots is well-structured.The VERSION=0.0.0 check with conditional RPM_VERSION/DEB_VERSION logic elegantly handles development builds by appending date and commit info. This allows ldflags injection via
$(COMMIT)to generate version strings like0.0.0^20251119.abc1234(RPM) and0.0.0+local20251119.abc1234(DEB).
28-32: OUT-centric build flow is clean and maintainable.The new
outdirtarget and dependency chain (build → outdir, $(PROG) → build) centralizes artifacts into the OUT directory, replacing ad-hoc in-tree placement. This isolates build artifacts from the source tree.
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com> Signed-off-by: Leon Fernandez <zluudg@zluudg.se>
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
Makefile (1)
71-71: Consider addingtesttarget to .PHONY (or update tool config).The static analysis tool (checkmake) flags
testas a required phony target. Either add a.PHONY: testdeclaration if a test target will be added, or suppress this rule if testing is not part of the Makefile.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
Makefile(1 hunks)
🧰 Additional context used
🪛 checkmake (0.2.2)
Makefile
[warning] 51-51: Target body for "srpm" exceeds allowed length of 5 (6).
(maxbodylength)
[warning] 62-62: Target body for "deb" exceeds allowed length of 5 (7).
(maxbodylength)
[warning] 71-71: Missing required phony target "test"
(minphony)
[warning] 4-4: Target "VERSION" should be declared PHONY.
(phonydeclared)
[warning] 6-6: Target "PROG" should be declared PHONY.
(phonydeclared)
[warning] 7-7: Target "OUT" should be declared PHONY.
(phonydeclared)
[warning] 8-8: Target "COMMIT" should be declared PHONY.
(phonydeclared)
[warning] 9-9: Target "GOFLAGS" should be declared PHONY.
(phonydeclared)
[warning] 11-11: Target "GO" should be declared PHONY.
(phonydeclared)
[warning] 12-12: Target "INSTALL" should be declared PHONY.
(phonydeclared)
🔇 Additional comments (4)
Makefile (4)
62-69: ✅ Line 65 path bug is fixed.The previously flagged issue of
mkdir -p deb/etc/dnstapir/certshas been correctly updated tomkdir -p $(OUT)/deb/etc/dnstapir/certs, ensuring the source tree is not modified.
71-71: ✅ .PHONY declaration is now complete.The .PHONY line now correctly includes all non-file targets, addressing the previous review concern.
1-20: Refactored version strategy with snapshot support looks sound.The hardcoded VERSION with conditional RPM_VERSION and DEB_VERSION snapshot logic (when VERSION is 0.0.0) appropriately handles pre-release packaging. The COMMIT variable correctly implements a file-or-git fallback, enabling tarball-based builds to use an embedded commit hash.
14-20: Verify snapshot versioning logic matches packaging expectations.Lines 18–19 embed a date stamp and commit hash into RPM and DEB version strings when VERSION is 0.0.0. Confirm that this snapshot format aligns with your package management and distribution workflows (e.g., RPM and Debian version comparison semantics).
Summary by CodeRabbit
New Features
Chores
✏️ Tip: You can customize this high-level summary in your review settings.