Skip to content
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

ci: Use hard-coded root path for CI containers (bugfix) #28185

Merged
merged 3 commits into from
Aug 15, 2023

Conversation

maflcko
Copy link
Member

@maflcko maflcko commented Jul 30, 2023

Currently the CI system will fail if the git folder that holds the Bitcoin Core source is moved from one location to another.

Fix this by using a single hard-coded root path inside the CI system containers.

Steps to test:

  • Run the CI system: MAKEJOBS="-j$(nproc)" FILE_ENV="./ci/test/00_setup_env_win64.sh" ./ci/test_run_all.sh
  • Move the git folder: pwd && cd .. && mv bitcoin_core_folder_1 bitcoin_core_folder_2 && cd ./bitcoin_core_folder_2 && pwd
  • Run the CI system again: (same cmd as above)

On master (error):

STRIPPROG="x86_64-w64-mingw32-strip" /bin/bash /bitcoin_core_folder_2/ci/scratch/build/bitcoin-x86_64-w64-mingw32/build-aux/install-sh -c -s ./src/qt/bitcoin-qt.exe ./release
/bitcoin_core_folder_2/ci/scratch/build/bitcoin-x86_64-w64-mingw32/build-aux/install-sh: ./src/qt/bitcoin-qt.exe does not exist.
make: *** [Makefile:1258: bitcoin-25.99.0-win64-setup.exe] Error 1

On this pull: (pass).

@DrahtBot
Copy link
Contributor

DrahtBot commented Jul 30, 2023

The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.

Reviews

See the guideline for information on the review process.

Type Reviewers
ACK fanquake
Concept ACK hebasto

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

Conflicts

No conflicts as of last run.

@DrahtBot DrahtBot changed the title ci: Use hard-coded root path for CI containers (bugfix) ci: Use hard-coded root path for CI containers (bugfix) Jul 30, 2023
@DrahtBot DrahtBot added the Tests label Jul 30, 2023
@maflcko maflcko marked this pull request as draft July 30, 2023 10:40
@maflcko
Copy link
Member Author

maflcko commented Jul 31, 2023

I rebased this on a commit by @fanquake to fix another bug, which I am not sure how it happened.

Cloning into '/ci_base_install/ci/scratch/iwyu/include-what-you-use'...
++ CI_EXEC rsync --archive --stats --human-readable /ci_base_install/ /tmp/cirrus-ci-build
sent 10.29K bytes  received 88 bytes  20.76K bytes/sec
python3: can't open file '/tmp/cirrus-ci-build/ci/scratch/iwyu/include-what-you-use/iwyu_tool.py': [Errno 2] No such file or directory

The same seemingly happened with msan, so instead of debugging further, I just did the same changes to msan in faa3186 to work around the bugs and clarify the code in any case.

@maflcko maflcko force-pushed the 2307-ci-hard-code- branch 3 times, most recently from 22fe27e to dddd8af Compare July 31, 2023 15:24
@maflcko maflcko marked this pull request as ready for review August 9, 2023 10:31
MarcoFalke added 3 commits August 9, 2023 12:32
Using a hard-coded path avoids non-determinism issues and improves CI
UX.
Now that container volumes are used, the folders are no longer mounted.
They are only needed when running without a container engine (docker,
podman).
@maflcko
Copy link
Member Author

maflcko commented Aug 9, 2023

Rebased on master to drop already merged bugfix commit d86a83d.

@DrahtBot DrahtBot removed the CI failed label Aug 9, 2023
@maflcko maflcko requested a review from hebasto August 14, 2023 08:29
@maflcko
Copy link
Member Author

maflcko commented Aug 14, 2023

I used the commands in the pull request description to re-test this is still a bugfix on current master.

@hebasto
Copy link
Member

hebasto commented Aug 14, 2023

Currently the CI system will fail if the git folder that holds the Bitcoin Core source is moved from one location to another.

Are there any real-life use cases for this scenario?

@maflcko
Copy link
Member Author

maflcko commented Aug 14, 2023

Yes, Cirrus CI persistent workers, and potentially GitHub (or other's) persistent workers (haven't checked).

Example: https://cirrus-ci.com/task/5714733521698816?logs=ci#L4307

@hebasto
Copy link
Member

hebasto commented Aug 14, 2023

Concept ACK.

@maflcko
Copy link
Member Author

maflcko commented Aug 14, 2023

Also, a developer may be moving the git folder at any time.

Copy link
Member

@fanquake fanquake left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ACK fafa17c - somewhat tested. MSAN changes are the same as what we did for tidy.

@fanquake fanquake merged commit 80d70cb into bitcoin:master Aug 15, 2023
15 checks passed
@maflcko maflcko deleted the 2307-ci-hard-code- branch August 15, 2023 10:28
sidhujag pushed a commit to syscoin/syscoin that referenced this pull request Aug 17, 2023
…bugfix)

fafa17c ci: Use hard-coded root path for CI containers (MarcoFalke)
fa084f5 ci: Only create folders when needed (MarcoFalke)
fab2712 ci: Drop BASE_SCRATCH_DIR from LIBCXX_DIR (MarcoFalke)

Pull request description:

  Currently the CI system will fail if the git folder that holds the Bitcoin Core source is moved from one location to another.

  Fix this by using a single hard-coded root path *inside* the CI system containers.

  Steps to test:

  * Run the CI system: `MAKEJOBS="-j$(nproc)" FILE_ENV="./ci/test/00_setup_env_win64.sh" ./ci/test_run_all.sh`
  * Move the git folder: `pwd && cd .. && mv bitcoin_core_folder_1 bitcoin_core_folder_2 && cd ./bitcoin_core_folder_2 && pwd`
  * Run the CI system again: (same cmd as above)

  On master (error):

  ```
  STRIPPROG="x86_64-w64-mingw32-strip" /bin/bash /bitcoin_core_folder_2/ci/scratch/build/bitcoin-x86_64-w64-mingw32/build-aux/install-sh -c -s ./src/qt/bitcoin-qt.exe ./release
  /bitcoin_core_folder_2/ci/scratch/build/bitcoin-x86_64-w64-mingw32/build-aux/install-sh: ./src/qt/bitcoin-qt.exe does not exist.
  make: *** [Makefile:1258: bitcoin-25.99.0-win64-setup.exe] Error 1
  ```

  On this pull: (pass).

ACKs for top commit:
  fanquake:
    ACK bitcoin@fafa17c - somewhat tested. MSAN changes are the same as what we did for tidy.

Tree-SHA512: 2ce693a3773c70fcfca062c2a6f0e5a16b94960b34a6145d10cee1a28f79154829d59d014465ccbb80e1cb9dcd5aa043729cee9afd2c4175b05e9bc945364b79
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants