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
  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/
    
  2. contrib: fix BUILDDIR in gen-bitcoin-conf script b3a96b3569
  3. 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.

  4. DrahtBot added the label Scripts and tools on Nov 20, 2024
  5. lucasbalieiro approved
  6. 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:
    image

    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:
    image

    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!

  7. BrandonOdiwuor commented at 7:05 pm on November 21, 2024: contributor
    tACK b3a96b35691f3fbf958958056cd905e119fb48fe
  8. 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.

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

  10. maflcko commented at 12:56 pm on December 2, 2024: member
    Are you still working on this?

github-metadata-mirror

This is a metadata mirror of the GitHub repository bitcoin/bitcoin. This site is not affiliated with GitHub. Content is generated from a GitHub metadata backup.
generated: 2024-12-03 21:12 UTC

This site is hosted by @0xB10C
More mirrored repositories can be found on mirror.b10c.me