-
Notifications
You must be signed in to change notification settings - Fork 201
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
migrate setup_environment.sh
to CMake
#752
Conversation
a6246d3
to
0596e47
Compare
0596e47
to
fb51e74
Compare
Thanks, @hebasto. Addressed #752 (comment). The solution is simpler than I thought. |
Not related to the CMake migration, but I'd suggest additionally this diff: --- a/test/setup_environment.sh
+++ b/test/setup_environment.sh
@@ -444,7 +444,7 @@ if [[ -n ${build_bitcoind} ]]; then
# Build bitcoind. This is super slow, but it is cached so it runs fairly quickly.
pushd depends
- make NO_QT=1 NO_QR=1 NO_ZMQ=1 NO_UPNP=1 NO_NATPMP=1
+ make NO_QT=1 NO_QR=1 NO_ZMQ=1 NO_UPNP=1 NO_NATPMP=1 NO_USDT=1
popd
# Do the build to skip needless build of the |
One more amendment is needed: --- a/.github/actions/build-bitcoind/action.yml
+++ b/.github/actions/build-bitcoind/action.yml
@@ -23,7 +23,7 @@ runs:
ccache --zero-stats
cd test; ./setup_environment.sh --bitcoind; cd ..
ccache --show-stats --verbose
- tar -czf bitcoind.tar.gz test/work/bitcoin/src/bitcoind
+ tar -czf bitcoind.tar.gz test/work/bitcoin/build/src/bitcoind
- uses: actions/cache/save@v4
if: github.event_name != 'pull_request' && steps.ccache-cache.outputs.cache-hit != 'true' |
fb51e74
to
37d520d
Compare
Addressed #752 (comment) and #752 (comment). Thanks, @hebasto. Just added you as co-author. |
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.
ACK 37d520d.
oops, another path issue (
CONFIG_SITE=$PWD/depends/x86_64-pc-linux-gnu/share/config.site ./configure --with-incompatible-bdb --with-miniupnpc=no --without-gui --disable-zmq --disable-tests --disable-bench --with-libs=no --with-utils=no | ||
make src/bitcoind | ||
cmake -B build --toolchain depends/x86_64-pc-linux-gnu/toolchain.cmake -DBUILD_TESTS=OFF -DBUILD_BENCH=OFF | ||
cmake --build build --target bitcoind |
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.
Are we interested in engaging in parallelism:
cmake --build build --target bitcoind | |
cmake --build build -j $(nproc) --target bitcoind |
?
If so, the same applies to building depends.
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.
I didn't address it because we did not have it previously, but I don't see why not engage parallelism.
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.
Addressed it in #755. I don't see why not parallelize it.
--- a/test/run_tests.py
+++ b/test/run_tests.py
@@ -60,7 +60,7 @@ parser.add_argument('--ledger-path', dest='ledger_path', help='Path to Ledger em
parser.add_argument('--jade-path', dest='jade_path', help='Path to Jade qemu emulator', default='work/jade/simulator')
parser.add_argument('--all', help='Run tests on all existing simulators', default=False, action='store_true')
-parser.add_argument('--bitcoind', help='Path to bitcoind', default='work/bitcoin/src/bitcoind')
+parser.add_argument('--bitcoind', help='Path to bitcoind', default='work/bitcoin/build/src/bitcoind')
parser.add_argument('--interface', help='Which interface to send commands over', choices=['library', 'cli', 'bindist', 'stdin'], default='library')
parser.add_argument("--device-only", help="Only run device tests", action="store_true") |
Co-authored-by: Hennadii Stepanov <[email protected]>
37d520d
to
937c1ae
Compare
Addressed #752 (comment) |
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.
ACK 937c1ae.
Not related to this PR changes, but it seems Coldcard/firmware@65b652a has to be considered to fix the CI. UPD. I suggest to leave it for a follow up PR. |
Just opened #754 as draft. |
ACK 937c1ae |
1dbc4fd setup_environment: parallelize bitcoind build (Bruno Garcia) Pull request description: See #752 (comment) ACKs for top commit: achow101: ACK 1dbc4fd hebasto: ACK 1dbc4fd. Tree-SHA512: 92dd01039b23ba291a9c97f7cd5508975913cbcb28b2f4507e6e83e0bcfd3780a1c60fef02793806aee55dbb6d077365be3ed64638a6a8102dfee2d164abb6ed
Fixes #750