Removing Bitcoin core text where unnecessary #33126
Conversation
|
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers. Code Coverage & BenchmarksFor details see: https://corecheck.dev/bitcoin/bitcoin/pulls/33126. ReviewsSee the guideline for information on the review process.
If your review is incorrectly listed, please react with 👎 to this comment and the bot will ignore it on the next update. ConflictsReviewers, this pull request conflicts with the following ones:
If you consider this pull request important, please also help to review the conflicting pull requests. Ideally, start with the one that should be merged first. |
|
Tested locally. Changes are minimal and improve flexibility for forks. Looks good. |
Concept NACK This is not something Bitcoin Core needs to support. |
|
What is the usecase for this ? forks would also fork the GUIX build script and if this is intended as multi client build script than move it to an independent repo. EDIT No longer NACK-ing this PR: see #33126 (comment) |
|
Concept NACK Unless this value was picked up from elsewhere, I don't see how this makes it easier to support "multi clients""? A two-line downstream patch... remains a two-line downstream patch. Only the lines being patched have changed? |
|
and ftr, Concept ACK. Core has always had a policy of using CLIENT_NAME correctly and not breaking needlessly when it's changed. PR title could be updated to appease naysayers. |
No I guess it isn't, but hopefully that means the spirit of FOSS that is invoked as an excuse when Bitcoin Core does something a large % of its users dislike is something we can stop pretending applies here. Making life harder for no good reason for people who fork this repo is lamentable behaviour. Otherwise, ACK. |
85cfa53 to
2c9aa1a
Compare
This objectively makes Core easier to fork and to maintain the fork. It doesn't need to support it, but it does make it a more accessible open source project. And its just cleaner. |
contrib/guix/libexec/build.sh
Outdated
There was a problem hiding this comment.
Potentially moving multiple files to a single file is not a good idea. Using a wildcard here makes this much more fragile and prone to error.
There was a problem hiding this comment.
Moving multiple files to a single file is an error, so it should cause the guix build to abort?
There was a problem hiding this comment.
I think this is fine here since it is in the deploy output folder. It should really never be giving out more than a single zip. It's safe to assume here that the deploy folder will just have the one
There was a problem hiding this comment.
Haven't explored this in depth, but my guess is that the zip filename comes from this line in add_macos_deploy_target:
https://github.com/bitcoin/bitcoin/blob/2c9aa1afa8dab8cf2c86b10010f1d3b342d93425/cmake/module/Maintenance.cmake#L87
An alternative approach might be to change that to something generic like CFBundleName = \"BundleName\", or make it have something closer to the final name in the first place.
There was a problem hiding this comment.
Changing it to CFBundleName = \"BundleName\" would be confusing or weird for people just running the raw deploy build, no?
There was a problem hiding this comment.
True, maybe one could make it possible for build.sh to override the bundle name when doing the deploy:
BUNDLE_NAME="${DISTNAME}-${HOST}-unsigned"
cmake --build build --target deploy ${V:+--verbose} "-DBUNDLE_NAME=${BUNDLE_NAME}"
mv "build/dist/${BUNDLE_NAME}.zip" "${OUTDIR}/"Then have Maintenance.cmake pick that up if set, otherwise fall back to CLIENT_NAME.
Guix builds (on x86_64) [untrusted test-only build, possibly unsafe, not for production use]
|
|
Can we merge this, the builds seem fine |
|
This has 3 nacks with no reasonable rebuttals, ready for close? |
This makes conducting a fork easier, and rebasing the fork/absorbing cores changes if required in a fork easier for a maintainer. The code is also a lot cleaner, given we check for bitcoin-utils in a better way, and don't have an unnecessary Bitcoin-Core based text in the build.sh |
This is addressed, and a fair callout. Now the downstream patch for a forked client is not a two line downstream patch |
Just addressed them. Feel like this is objectively more reusable code |
Rebuttal: claiming to be open source, but not acting in the spirit of open source, is bad for Core's already tarnished reputation. Sure, no one has to do anything, but ngaf about people outside of your immediate circle is no way to win kudos. |
|
Concept ACK - obviously. Simple change that simplifies forking. Core may not NEED to do this, but why would Core NOT do this? |
Isn't the missing links to the guix build outcomes not an indication that the build is indeed not fine ? would expect a result more like #32865 (comment) |
|
You are free to fork the code and change whatever you like, but the burden of maintaining your fork falls on you not the contributors of Bitcoin Core. |
No, there are no ACKs from regular contributors. No PR is merged just because it builds fine. |
Thanks for addressing my question. With a more descriptive PR title your intentions with the code changes (PR) are better understood. Although I'm not agreeing that the current state of the code in the PR is "a lot cleaner" I'm also not going to block the PR any longer. Have removed my NACK. |
jonatack
left a comment
There was a problem hiding this comment.
Concept ACK, provided this small patch is robust. It's a minor change to be sure. Nevertheless, it seems cleaner not to hardcode it and would convey a magnanimous FOSS spirit that I encourage.
|
I'm not sure why it would be our job to make the code more forkable - but we do need to make it maintainable, so concept ACK for less hard-coding - no opinion on the implementation details, will let others review the specifics. |
2c9aa1a to
0560c98
Compare
good call, there was a problem with using binary.name in the new python lief version. We should be fine now, just pushed a fix. However i cant run the drahbot build, if somebody could add the label for building, would be helpful |
The contributors and builders of the bitcoin network do have a responsibility towards helping decentralise the network, which leads to overall network health. In my humble opinion, helping support more diversity in client choice for node runners should be, if not already is, a core principle of the network. Also this leads to less hardcoding and code cleanliness, as agreed upon by other reviewers. |
No, it didn't. The build failed and you haven't tested this yourself at all?
A passing CI (or build) is not sufficient for a merge. Especially here, because a full guix build (with signing) isn't run by any CI. The burden to explain the changes (and why they are safe and correct, and how to test them) is on the pull request author. Instead of spamming the thread with comments about rushing a merge here without proper testing and review, it would be better to make testing and review easier. |
Hey apologies, my intent was not to rush, was just trying to address everyones concerns Did run tests, but had a bad local environment, unfortunately Anyways, everything should be fine now, should attach a drahbot guix build request to be sure |
Ideally you would do the full guix build yourself and post the build artifact hashes. Your dev environment should have no influence on that. Then reviewers can compare their results to yours. |
|
guix build for verification |
|
|
||
| # bitcoin-util does not currently contain any fortified functions | ||
| if 'Bitcoin Core bitcoin-util utility version ' in binary.strings: | ||
| if any(' bitcoin-util utility version ' in s for s in binary.strings): |
|
🐙 This pull request conflicts with the target branch and needs rebase. |
We do not need Bitcoin Core specific text on these guix-build flows. This makes forking the open source project easier.
These make the scripts more reusable.