Skip to content

Conversation

maflcko
Copy link
Member

@maflcko maflcko commented Jul 24, 2023

This fixes a bug where the $PATH from the host is used inside the container. This will lead to bugs when the $PATH is different. For example on a host of Fedora 38, and a container of debian:bullseye.

This can be tested with the FILE_ENV=./ci/test/00_setup_env_arm.sh CI env. On master:

Error: crun: executable file `bash` not found in $PATH: No such file or directory: OCI runtime attempted to invoke a command that was not found

On this pull:

(everything passes)

@DrahtBot
Copy link
Contributor

DrahtBot commented Jul 24, 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 TheCharlatan

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:

  • #27976 (ci: Start with clean env by MarcoFalke)
  • #27793 (ci: label docker images and prune dangling images selectively by stickies-v)

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 DrahtBot changed the title ci: Keep system env vars as-is (bugfix) ci: Keep system env vars as-is (bugfix) Jul 24, 2023
@DrahtBot DrahtBot added the Tests label Jul 24, 2023
@maflcko maflcko force-pushed the 2307-ci-system-env- branch from fa9aeba to fad4f39 Compare July 24, 2023 14:05
MarcoFalke added 3 commits July 24, 2023 16:10
The --workdir setting to the docker run command is not needed. And
P_CI_DIR/PWD is equal to BASE_ROOT_DIR, so just use that directly.
This is needed for the next commit.

This also requires dropping CI_RETRY from the docker build step, which
is fine, because CI_RETRY should be called inside the build script, not
outside.

Also, fix a doc typo.
Copy link
Contributor

@TheCharlatan TheCharlatan left a comment

Choose a reason for hiding this comment

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

lgtm ACK fabc04a

@fanquake fanquake merged commit 4c57e53 into bitcoin:master Jul 28, 2023
@maflcko maflcko deleted the 2307-ci-system-env- branch July 28, 2023 15:09
sidhujag pushed a commit to syscoin/syscoin that referenced this pull request Aug 9, 2023
fabc04a ci: Keep system env vars as-is (MarcoFalke)
fa8dcdc ci: Set PATH inside the CI env (MarcoFalke)
fac229a ci: Remove P_CI_DIR and --workdir (MarcoFalke)

Pull request description:

  This fixes a bug where the `$PATH` from the host is used inside the container. This will lead to bugs when the `$PATH` is different. For example on a host of Fedora 38, and a container of `debian:bullseye`.

  This can be tested with the `FILE_ENV=./ci/test/00_setup_env_arm.sh` CI env. On master:

  ```
  Error: crun: executable file `bash` not found in $PATH: No such file or directory: OCI runtime attempted to invoke a command that was not found
  ```

  On this pull:

  (everything passes)

ACKs for top commit:
  TheCharlatan:
    lgtm ACK fabc04a

Tree-SHA512: 51d2affed91624d0e5b43a3ee1e696313f934e05fde6b5a19e8ac4e6666c1e7b667a444bf3de3f77190bcd00e81efb7576196afb0f6b5e788d1a50e7ddb28d7e
@bitcoin bitcoin locked and limited conversation to collaborators Jul 27, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants