[build-hooks] Fix reproducible build: pin apt versions and track autoremove#25552
Conversation
|
/azp run Azure.sonic-buildimage |
|
Azure Pipelines successfully started running 1 pipeline(s). |
There was a problem hiding this comment.
Pull request overview
This PR updates the sonic-build-hooks apt-get hook path to improve reproducible builds when ENABLE_VERSION_CONTROL_DEB=y, by pinning apt-get install package arguments to the versions recorded in versions-deb and by capturing versions prior to apt-get autoremove.
Changes:
- Add
pin_apt_versions()helper to rewriteapt-get installargs intopkg=versionusingversions-deb. - Update the
apt-gethook to apply pinned args before invoking the realapt-get. - Extend version capture to include
apt-get autoremovein addition topurge/remove.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 4 comments.
| File | Description |
|---|---|
| src/sonic-build-hooks/scripts/buildinfo_base.sh | Adds pin_apt_versions() to pin apt-get install package arguments based on versions-deb. |
| src/sonic-build-hooks/hooks/apt-get | Applies pinned args during installs and captures versions on autoremove. |
yxieca
left a comment
There was a problem hiding this comment.
autoremove tracking: Clean, LGTM.
pin_apt_versions(): A few concerns:
-
Grep injection with
+in package names —grep "^${para}=="uses regex, but Debian package names commonly contain+(e.g.,g++-10,libstdc++6,libc++1). The+is a regex quantifier, sogrep "^g++-10=="matchesg-10==,gg-10==, etc. instead of the literal package name. This could silently pin the wrong version or fail to match. Fix: usegrep -Fwith an exact match pattern, e.g.,grep -F "${para}==" "$VERSION_FILE" | head -1 | awk -F'==' '{print $2}'— sincegrep -Ftreats the pattern as a literal string,+is matched correctly. -
set -- $versioned_argsloses quoting — The subshell output is re-split on whitespace via unquoted$versioned_args. Package names don't have spaces, but option values could. Minor risk but worth noting. -
Options after
installkeyword — Args like-t bullseyewill have-tskipped butbullseyetreated as a package name and looked up in versions-deb. It won't match so it passes through safely, but it's a design gap worth a comment in the code.
Item #1 is the actionable one — + in package names is common enough to cause real issues.
8604bc6 to
d57b140
Compare
|
/azp run Azure.sonic-buildimage |
|
Thanks for the thorough review @yxieca! All addressed in the latest push:
Also fixed Copilot's catch: |
|
Azure Pipelines successfully started running 1 pipeline(s). |
yxieca
left a comment
There was a problem hiding this comment.
All feedback addressed — awk for exact match, proper array quoting, options documented. LGTM.
@xumia @liushilongbuaa — could you review the build-hooks changes? The pin_apt_versions() function modifies the apt-get install args to enforce version pinning from versions-deb when ENABLE_VERSION_CONTROL_DEB=y.
|
@yijingyan2 , please review and test. |
d57b140 to
487acfa
Compare
|
/azp run Azure.sonic-buildimage |
|
Azure Pipelines successfully started running 1 pipeline(s). |
|
Addressed all Copilot review comments in latest incremental commit:
|
Hi @rustiqly, I ran a test build with the build option currently, we are using snapshot to pin the package versions. It may cause conflict if we pin the versions based on both versions-deb files and snapshot? |
|
/azp run Azure.sonic-buildimage |
|
Good catch @yijingyan2! You're right — when versions-deb is generated from a FIPS build, packages like I just pushed a fix:
Regarding snapshot vs versions-deb conflict — they serve different purposes: snapshot pins the repo state (which packages are available), while versions-deb pins the installed versions. They should be complementary, not conflicting. But if you see issues there, happy to discuss further. Could you re-test with this latest commit? |
|
Azure Pipelines successfully started running 1 pipeline(s). |
Hi @rustiqly, if The versions for grep and sed are pinned to versions that are only available in arm64 |
0c1889b to
b5ec9ce
Compare
|
Great catches @yijingyan2, both issues are real:
Pushed both fixes. Could you re-test? |
|
/azp run Azure.sonic-buildimage |
b8108d7 to
15f66f2
Compare
|
/azp run Azure.sonic-buildimage |
|
Azure Pipelines successfully started running 1 pipeline(s). |
15f66f2 to
25ca39e
Compare
|
/azp run Azure.sonic-buildimage |
|
Azure Pipelines successfully started running 1 pipeline(s). |
|
CI failures are all infrastructure/flaky issues unrelated to this change:
Could a maintainer please re-trigger CI? Thank you! |
25ca39e to
c615ecd
Compare
|
/azp run Azure.sonic-buildimage |
|
Azure Pipelines successfully started running 1 pipeline(s). |
yejianquan
left a comment
There was a problem hiding this comment.
LGTM. Three solid fixes:
- grep pattern fixed from ^pkg= to ^pkg== matching the actual versions-deb format
- autoremove added to version capture -- intermediate build deps were previously lost
- +fips suffix stripping for locally-rebuilt FIPS packages
Also good: VERSION_FILE made local to avoid leaking into global scope.
🤖 Posted by DevAce, Jianquan's AI Agent, on his behalf.
|
@liushilongbuaa ms_conflict. |
c615ecd to
d336c37
Compare
|
/azp run Azure.sonic-buildimage |
|
Azure Pipelines successfully started running 1 pipeline(s). |
d336c37 to
3f0f93e
Compare
|
/azp run Azure.sonic-buildimage |
|
Azure Pipelines successfully started running 1 pipeline(s). |
|
/azpw ms_conflict |
3f0f93e to
5f48384
Compare
|
/azp run Azure.sonic-buildimage |
|
Azure Pipelines successfully started running 1 pipeline(s). |
…remove When ENABLE_VERSION_CONTROL_DEB=y (SONIC_VERSION_CONTROL_COMPONENTS includes 'deb'), the apt-get hook now actively pins package versions from versions-deb instead of only warning about missing versions. Three fixes: 1. Add pin_apt_versions() - rewrites 'apt-get install foo' to 'apt-get install foo=1.2.3' using versions from versions-deb file. Only activates when deb version control is enabled. Packages not in the versions file pass through unchanged (with existing warning). 2. Track autoremove - collect package versions before apt-get autoremove (like we already do for purge/remove) so intermediate build dependencies are captured in purge-versions-deb. 3. Keep existing APT preferences mechanism (Pin-Priority: 999) as defense-in-depth alongside the explicit version pinning. Fixes sonic-net#7502 Signed-off-by: Rustiqly <[email protected]>
Address Copilot review feedback: - Make VERSION_FILE local in check_apt_version() to avoid global mutation - Replace grep '^pkg=' with awk -F'==' exact match (versions-deb uses ==) Signed-off-by: Rustiqly <[email protected]>
When versions-deb is generated from a FIPS build, package versions may contain a +fips suffix (e.g. openssh-server==1:10.0p1-7+fips). When building without ENABLE_FIPS=y, these versions don't exist in the apt repositories, causing 'apt-get install' to fail. Strip the +fips suffix from pinned versions when ENABLE_FIPS is not enabled. Signed-off-by: Rustiqly <[email protected]>
Address review feedback from liushilongbuaa: remove the pin_apt_versions() function and its call in the apt-get hook. Version pinning is already handled by /etc/apt/preferences.d/01-versions-deb (Pin-Priority: 999), generated by update_preference_deb() and installed by pre_run_buildinfo. Having two mechanisms was confusing and hard to debug. Also add +fips suffix stripping in update_preference_deb() so preferences.d handles FIPS packages correctly (previously only pin_apt_versions did this). Signed-off-by: Rustiqly <[email protected]>
5f48384 to
d5f2b87
Compare
|
/azp run Azure.sonic-buildimage |
|
Azure Pipelines successfully started running 1 pipeline(s). |
Problem
When
ENABLE_VERSION_CONTROL_DEB=y(SONIC_VERSION_CONTROL_COMPONENTSincludesdeb), the apt-get hook has two issues:1. Versions from versions-deb are never applied to apt-get arguments (#7502)
check_apt_version()only warns about missing package versions — it never modifies theapt-get installcommand to pin versions. While the APT preferences mechanism (01-versions-deb→/etc/apt/preferences.d/) provides pinning viaPin-Priority: 999, this doesn't guarantee exact version matching (APT can still pick a different version if the pinned one isn't available).2.
apt-get autoremovedoesn't capture package versionsThe hook captures package versions before
purgeandremove, but notautoremove. Build dependencies installed viaapt-get installand later cleaned up viaapt-get autoremoveare lost from the version tracking, making builds non-reproducible.Fix
pin_apt_versions()— new function inbuildinfo_base.shWhen
ENABLE_VERSION_CONTROL_DEB=y, rewritesapt-get install foo bartoapt-get install foo=1.2.3 bar=4.5.6by looking up versions in theversions-debfile.foo=1.2.3) pass through unchangedautoremove tracking
Added
autoremoveto the list of commands that trigger version capture (dpkg-query -W→purge-versions-deb), alongsidepurgeandremove.Impact
debis not in the defaultSONIC_VERSION_CONTROL_COMPONENTSFixes #7502