-
Notifications
You must be signed in to change notification settings - Fork 0
Merge bitcoin/bitcoin#29249: depends: add NM output to gen_id #1068
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: backport-sept-30-2025
Are you sure you want to change the base?
Merge bitcoin/bitcoin#29249: depends: add NM output to gen_id #1068
Conversation
6ec2813 depends: add NM output to gen_id (fanquake) Pull request description: `NM` is part of the current toolset, and can be set by the user. Include it in `gen_id`. ACKs for top commit: TheCharlatan: Re-ACK 6ec2813 Tree-SHA512: 2ada61e03783f9eb441f285ef5da50557ad729cb52ce2d2c4b2c38103dab29920a26262d4545fd2ac7fbf1cedc4902cd2359833544fbc0debf829c12a63e9769
|
Note Other AI code review bot(s) detectedCodeRabbit has detected other AI code review bot(s) in this pull request and will avoid duplicating their findings in the review comments. This may lead to a less comprehensive review. WalkthroughMakefile now passes NM into gen_id for both build and host id generation; gen_id was extended to report NM (version, NM_ env vars, BEGIN/END NM) alongside existing tool introspection. Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ Finishing touches🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: ASSERTIVE Plan: Pro 📒 Files selected for processing (1)
🧰 Additional context used📓 Path-based instructions (2)depends/**📄 CodeRabbit inference engine (CLAUDE.md)
Files:
**⚙️ CodeRabbit configuration file
Files:
🧠 Learnings (2)📓 Common learnings📚 Learning: 2025-07-28T23:09:09.522ZApplied to files:
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (5)
🔇 Additional comments (1)
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 |
✅ Backport Verification SuccessfulNo issues found! This backport is complete and faithful to the original Bitcoin change. Original Bitcoin commit: Verification Details:
Reviewer Feedback Analysis:PastaPastaPasta comments reviewed: 0 No human reviewer feedback requiring action. Technical Analysis:Bitcoin commit 0375244 (bitcoin#29249): depends: add NM output to gen_id
Dash PR changes:
Validation Results:✅ Size validation: 100% of Bitcoin change size (within 80-150% range) This PR is ready for merge. ✅ |
Co-authored-by: Konstantin Akimov <knstqq@gmail.com>
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.
This PR is being reviewed by Cursor Bugbot
Details
You are on the Bugbot Free tier. On this plan, Bugbot will review limited PRs each billing cycle.
To receive Bugbot reviews on all of your PRs, visit the Cursor dashboard to activate Pro and start your 14-day free trial.
depends/Makefile
Outdated
| # | ||
| build_id:=$(shell env CC='$(build_CC)' C_STANDARD='$(C_STANDARD)' CXX='$(build_CXX)' CXX_STANDARD='$(CXX_STANDARD)' AR='$(build_AR)' RANLIB='$(build_RANLIB)' STRIP='$(build_STRIP)' SHA256SUM='$(build_SHA256SUM)' DEBUG='$(DEBUG)' LTO='$(LTO)' NO_HARDEN='$(NO_HARDEN)' ./gen_id '$(BUILD_ID_SALT)' 'GUIX_ENVIRONMENT=$(realpath $(GUIX_ENVIRONMENT))') | ||
| $(host_arch)_$(host_os)_id:=$(shell env CC='$(host_CC)' C_STANDARD='$(C_STANDARD)' CXX='$(host_CXX)' CXX_STANDARD='$(CXX_STANDARD)' AR='$(host_AR)' RANLIB='$(host_RANLIB)' STRIP='$(host_STRIP)' SHA256SUM='$(build_SHA256SUM)' DEBUG='$(DEBUG)' LTO='$(LTO)' NO_HARDEN='$(NO_HARDEN)' ./gen_id '$(HOST_ID_SALT)' 'GUIX_ENVIRONMENT=$(realpath $(GUIX_ENVIRONMENT))') | ||
| build_id:=$(shell env CC='$(build_CC)' C_STANDARD='$(C_STANDARD)' CXX='$(build_CXX)' CXX_STANDARD='$(CXX_STANDARD)' AR='$(build_AR) 'NM='$(build_NM)' RANLIB='$(build_RANLIB)' STRIP='$(build_STRIP)' SHA256SUM='$(build_SHA256SUM)' DEBUG='$(DEBUG)' LTO='$(LTO)' NO_HARDEN='$(NO_HARDEN)' ./gen_id '$(BUILD_ID_SALT)' 'GUIX_ENVIRONMENT=$(realpath $(GUIX_ENVIRONMENT))') |
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.
Bug: Shell Quoting Error in Environment Variable Assignment
The build_id assignment for AR and NM has a shell quoting error. The AR variable's value includes an unintended trailing space and is missing its closing quote, causing the NM assignment to be incorrectly concatenated into AR's value instead of being set as a separate environment variable. This differs from the correct syntax used for host_id.
20ce09a to
d96d2ab
Compare
- Fix spacing issue where AR variable was missing closing quote - This caused build_AR value to be concatenated with "NM=" string - Applied fix from Bitcoin PR bitcoin#31627 (commit 8a46286) - Resolves reviewer feedback from knst and cursor[bot] Addresses issue identified in review of PR #1068
Verification Agent: Fixes AppliedI've applied a fix to address the shell quoting error identified by reviewers. Issue FixedShell quoting error in depends/Makefile:150
ContextThe original Bitcoin commit (bitcoin/bitcoin@03752444cd) introduced this typo, which was later fixed in Bitcoin PR bitcoin#31627 (commit 8a46286). Rather than requiring a separate backport of bitcoin#31627, I've applied the fix directly based on reviewer feedback from @knst and cursor[bot]. Analysis Summary
The fix has been committed and pushed. CI will re-run to validate the change. |
✅ Verification Complete - APPROVEDSummaryThis backport of Bitcoin Core PR bitcoin#29249 has been verified and approved. Verification ResultsBitcoin Commit: Changes:
Files Modified:
CI Status: ✅ All 52 checks passing Fix AppliedThe initial backport contained a shell quoting error in
This fix was correctly applied in commit ConclusionThe backport is complete, faithful to Bitcoin's intent, and all CI checks pass. The shell quoting issue has been resolved. |
Backports bitcoin#29249
Original commit: 0375244
Backported from Bitcoin Core v0.27
Note
Include
NMin depends build-id hashing by adding NM output todepends/gen_idand passingNMfromdepends/Makefile.depends/gen_id: AddNMto the hashed preimage (prints${NM} --versionandNM_env), update usage docs.depends/Makefile: PassNMfor both build and host toolsets intogen_idwhen computingbuild_idand$(host_arch)_$(host_os)_id.Written by Cursor Bugbot for commit e912491. This will update automatically on new commits. Configure here.
Summary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings.