contrib: fix BUILDDIR in gen-bitcoin-conf script #31332

pull MarnixCroes wants to merge 1 commits into bitcoin:master from MarnixCroes:fix-builddir changing 1 files +1 −1

MarnixCroes commented at 2:13 pm on November 20, 2024: contributor
On master the gen-bitcoin-conf script doesn’t work as it cannot find bitcoind. This is because of the build dir that is now being used since cmake.

This PR fixes it.

master
0./gen-bitcoin-conf.sh 1/home/marnix/projects/bitcoin/src/bitcoind not found or not executable.
PR
0./gen-bitcoin-conf.sh 1Generating example bitcoin.conf file in share/examples/
contrib: fix BUILDDIR in gen-bitcoin-conf script b3a96b3569
DrahtBot commented at 2:13 pm on November 20, 2024: contributor
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.

Code Coverage & Benchmarks

For details see: https://corecheck.dev/bitcoin/bitcoin/pulls/31332.

Reviews

See the guideline for information on the review process.

Type Reviewers

ACK lucasbalieiro, BrandonOdiwuor, jonatack

If your review is incorrectly listed, please react with 👎 to this comment and the bot will ignore it on the next update.

Conflicts

Reviewers, this pull request conflicts with the following ones:
- #31161 (cmake: Set top-level target output locations by hebasto)
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.
DrahtBot added the label Scripts and tools on Nov 20, 2024
lucasbalieiro approved
lucasbalieiro commented at 11:55 pm on November 20, 2024: none
Hi @MarnixCroes!

tACK b3a96b3

Summary of the Changes

This PR addresses an issue introduced by the new cmake build process, which places the build output in the ./build directory. Specifically, it updates the BUILDDIR path in the script to point to this directory, ensuring compatibility with in-tree builds.

This behavior is consistent with the instructions outlined in contrib/devtools/README.md:
With in-tree builds this tool can be run from any directory within the
repository. To use this tool with out-of-tree builds set BUILDDIR. For
example:

0BUILDDIR=$PWD/build contrib/devtools/gen-bitcoin-conf.sh
Potential Conflict

It’s worth noting that this PR conflicts with #31161 by @hebasto, which proposes a more holistic solution to this issue.

If Hebasto’s PR is merged first, this PR could potentially break those changes. It might be beneficial to coordinate between these two PRs to avoid introducing unnecessary conflicts or redundant solutions.

Testing Results

Platform
- Distributor ID: Ubuntu
- Description: Ubuntu 22.04.5 LTS
- Release: 22.04
- Codename: jammy
Master Branch

I was able to reproduce the issue on the master branch. The script fails to locate bitcoind due to the directory discrepancy caused by the cmake build process. Evidence of the issue can be seen in the screenshot below:

PR Branch

Testing the PR resolved the issue, and the script successfully generated the bitcoin.conf file in the correct location. Evidence of the fix is shown in the screenshot below:

Further Testing

To ensure stability, I rebuilt the entire Bitcoin Core project with the changes from this PR and encountered no issues. Everything worked as expected.

Closing Thoughts

This PR effectively resolves the issue in its current context, but careful coordination with #31161 is necessary to avoid conflicts or redundant fixes. Great work on the implementation!
BrandonOdiwuor commented at 7:05 pm on November 21, 2024: contributor

tACK b3a96b35691f3fbf958958056cd905e119fb48fe
jonatack commented at 7:37 pm on November 21, 2024: member

Tested ACK b3a96b35691f3fbf958958056cd905e119fb48fe

With respect to #31161, the bug can be fixed now with this change and that pull easily updated if it moves forward.
achow101 commented at 11:14 pm on November 21, 2024: member

contrib/devtools/README.md needs to be updated to mention this as then the script would no longer work for in-tree builds (if we ever do that with cmake, but I have asked for it before).

Furthermore, that documentation is pretty clear that out of tree builds should set BUILDDIR, and with cmake, all builds will be out of tree, so I’m not sure that this change is actually useful.
maflcko commented at 12:56 pm on December 2, 2024: member

Are you still working on this?
maflcko added the label Up for grabs on Dec 9, 2024
i-am-yuvi commented at 2:00 pm on January 4, 2025: none

Can I take this up if no one is working?
i-am-yuvi commented at 2:13 pm on January 4, 2025: none

contrib/devtools/README.md needs to be updated to mention this as then the script would no longer work for in-tree builds (if we ever do that with cmake, but I have asked for it before).

Furthermore, that documentation is pretty clear that out of tree builds should set BUILDDIR, and with cmake, all builds will be out of tree, so I’m not sure that this change is actually useful.

Agree, I think we can close this.
MarnixCroes commented at 3:40 pm on January 4, 2025: contributor

sorry forgot to reply.

while BUILDDIR can be set, it’s odd that simply executing the script doesn’t work imo

It’s not clear for me what is the desired way forward.
maflcko commented at 8:53 am on January 6, 2025: member
#31332 (comment) is pretty clear that the current docs and script is correct. If the script is changed, the docs will have to be adjusted as well. Otherwise there will be confusion and bugs. So any of the following can be done:
- Close this pull and leave it as-is
- Change the docs and the script
- A third alternative could be to make the script a template (.in file) and have cmake inject the source and build dir, putting the resulting script in the build dir.
fanquake added this to the milestone 29.0 on Jan 6, 2025
achow101 commented at 9:53 pm on January 6, 2025: member
It’s not clear for me what is the desired way forward.

I have no preference either way, but I think the only reasonable options:
1. Close the PR
2. Update both the docs and script
As it is now, the docs and script are consistent with each other in master, but conflicting in this PR.
maflcko removed the label Up for grabs on Jan 7, 2025
maflcko removed this from the milestone 29.0 on Jan 7, 2025