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

MarnixCroes · 2024-11-20T14:13:55Z

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

./gen-bitcoin-conf.sh 
/home/marnix/projects/bitcoin/src/bitcoind not found or not executable.

PR

./gen-bitcoin-conf.sh 
Generating example bitcoin.conf file in share/examples/

DrahtBot · 2024-11-20T14:13:59Z

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.

lucasbalieiro

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:
BUILDDIR=$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

tACK b3a96b3

jonatack

Tested ACK b3a96b3

With respect to #31161, the bug can be fixed now with this change and that pull easily updated if it moves forward.

achow101 · 2024-11-21T23:14:14Z

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 · 2024-12-02T12:56:55Z

Are you still working on this?

yuvicc · 2025-01-04T14:00:34Z

Can I take this up if no one is working?

yuvicc · 2025-01-04T14:13:47Z

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 · 2025-01-04T15:40:42Z

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 · 2025-01-06T08:53:02Z

#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.

achow101 · 2025-01-06T21:53:13Z

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:

Close the PR
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.

jurraca · 2025-01-27T20:38:14Z

I picked this up and fixed both script and docs #31742. The change was also needed for the gen-manpages.py script.

maflcko · 2025-02-05T19:27:35Z

Ok, closing for #31742

…n-manpages.py 63a8791 contrib: fix BUILDDIR in gen-bitcoin-conf script and gen-manpages.py (jurraca) Pull request description: The `gen-bitcoin-conf.sh` and `gen-manpages.py` scripts assume a top level `src/` build dir, but in-tree builds are no longer allowed, nor recommended in the build steps. If a user builds `bitcoind` as recommended, these scripts fail. To fix it, we update the `BUILDDIR` env var and update the README accordingly. Follows up on initial work and discussion in #31332 . ACKs for top commit: fjahr: Code review ACK 63a8791 achow101: ACK 63a8791 Tree-SHA512: cf4d5b0d2e8b1f5db759bec01e131d8a0c511a2fd183389d2a0488d5fe4a906db2579d944f408b5c966f619edc6b2534023c3521f1fa5f8edd0216d29f3e48db

contrib: fix BUILDDIR in gen-bitcoin-conf script

b3a96b3

DrahtBot added the Scripts and tools label Nov 20, 2024

DrahtBot mentioned this pull request Nov 20, 2024

cmake: Set top-level target output locations #31161

Merged

lucasbalieiro approved these changes Nov 20, 2024

View reviewed changes

BrandonOdiwuor reviewed Nov 21, 2024

View reviewed changes

jonatack reviewed Nov 21, 2024

View reviewed changes

fanquake mentioned this pull request Dec 8, 2024

Update gen-bitcoin-conf.sh #31443

Closed

maflcko added the Up for grabs label Dec 9, 2024

fanquake added this to the 29.0 milestone Jan 6, 2025

maflcko removed the Up for grabs label Jan 7, 2025

maflcko removed this from the 29.0 milestone Jan 7, 2025

jurraca mentioned this pull request Jan 27, 2025

contrib: fix BUILDDIR in gen-bitcoin-conf script and gen-manpages.py #31742

Merged

maflcko closed this Feb 5, 2025

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

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

Uh oh!

Conversation

MarnixCroes commented Nov 20, 2024

Uh oh!

DrahtBot commented Nov 20, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Code Coverage & Benchmarks

Reviews

Conflicts

Uh oh!

lucasbalieiro left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Summary of the Changes

Potential Conflict

Testing Results

Platform

Master Branch

PR Branch

Further Testing

Closing Thoughts

Uh oh!

BrandonOdiwuor left a comment

Choose a reason for hiding this comment

Uh oh!

jonatack left a comment

Choose a reason for hiding this comment

Uh oh!

achow101 commented Nov 21, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

maflcko commented Dec 2, 2024

Uh oh!

yuvicc commented Jan 4, 2025

Uh oh!

yuvicc commented Jan 4, 2025

Uh oh!

MarnixCroes commented Jan 4, 2025

Uh oh!

maflcko commented Jan 6, 2025

Uh oh!

achow101 commented Jan 6, 2025

Uh oh!

jurraca commented Jan 27, 2025

Uh oh!

maflcko commented Feb 5, 2025

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

10 participants

DrahtBot commented Nov 20, 2024 •

edited

Loading

lucasbalieiro left a comment •

edited

Loading

achow101 commented Nov 21, 2024 •

edited

Loading