Makefile: Bring in same build step as in the rpm spec file#2239
Makefile: Bring in same build step as in the rpm spec file#2239PeaceRebel wants to merge 6 commits into
Conversation
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request refactors the build process in the Makefile and build script to support modular targets, including cross-compilation for ignition-validate. The review feedback highlights several critical issues: the build script's case statement defaults to 'all' but lacks an 'all' case handler; the install and install-ignition-validate-cross Makefile targets lack build dependencies, which will cause failures on clean repositories; modifying the global NAME variable inside ignition_validate() introduces side effects; and the GOARCH assignment is redundant due to the subsequent eval $(go env) call.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
| GOARCH=${GOARCH:-$(go env GOARCH)} | ||
|
|
||
| eval $(go env) |
There was a problem hiding this comment.
The line GOARCH=${GOARCH:-$(go env GOARCH)} is redundant and inefficient. eval $(go env) on the next line already retrieves and sets the correct GOARCH variable (respecting any inherited environment variables). Calling go env twice spawns an unnecessary subprocess, which slows down the build.
You can safely remove the redundant GOARCH assignment.
eval $(go env)
PeaceRebel
force-pushed
the
pr/makenbuild
branch
2 times, most recently
from
1ccc0a2 to
831adf3
Compare
PeaceRebel
force-pushed
the
pr/makenbuild
branch
2 times, most recently
from
3a45b03 to
46c6aa7
Compare
|
Temporarily added skip-notes label. Will add back once checks are fixed. |
Makefile and the build script are not being used by the rpmspec now. We need to update the steps in these file to match the spec and change the rpmspec to use Makefile. Added new make targets: - ignition: build ignition binary - ignition-validate: build ignition validate binary - ignition-validate-cross: crossbuild ignition validate binary - install-ignition-validate-cross: install cross built validate binary Build script updated to support these changes.
With the changes in build scripts, sourcing the script will cause error. We only need GOARCH here, which is handled by this change.
We have changed how build script works. Define required variables in this script rather than sourcing build script.
`make` now has to be called with the specific binary names. Default output path for the binary has changed. Adjust this job accordingly.
PeaceRebel
force-pushed
the
pr/makenbuild
branch
from
290388d to
7cb8925
Compare
c4rt0
left a comment
c4rt0
left a comment
There was a problem hiding this comment.
/lgtm but it would be probably a good idea to have an aditional set of eyes on this
There was a problem hiding this comment.
This is looking good overall, thank you for working on this!
One thing I noticed -- the Makefile doesn't export VERSION, so when the RPM spec does make ignition BIN_PATH=. VERSION=%{version}, that VERSION won't actually get forwarded to ./build. The build script will fall back to git describe instead of using the spec version. I think we need:
VERSION ?=
export VERSION
alongside the BIN_PATH export.
Also a minor nit: in the build script, GOARCH gets set on line 29 but then immediately overwritten by eval $(go env) on line 31. Not a problem today but could be surprising later.
3 participants
Makefile and the build script are not being used by the rpmspec now. We need to update the steps in these files to match the spec and change the rpmspec to use Makefile.
Added new make targets:
Build script updated to support these changes.
Goes with https://src.fedoraproject.org/rpms/ignition/pull-request/147 for Fedora