Skip to content

Conversation

maflcko
Copy link
Member

@maflcko maflcko commented May 4, 2023

CI_EXEC has many issues:

  • It is roughly equivalent to bash -c "$*", meaning that the full command will be treated as a single string, ignoring tokens.
  • It must be put in front of (almost) every command, making it easy to forget, hard to debug the resulting failure, and the code verbose.

Fix all issues in one script by removing it.

@DrahtBot
Copy link
Contributor

DrahtBot commented May 4, 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, 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:

  • #27571 (ci: Run iwyu on all src files by MarcoFalke)
  • #27530 (Remove now-unnecessary poll, fcntl includes from net(base).cpp by Empact)
  • #27425 (test: move remaining rand code from util/setup_common to util/random by jonatack)
  • #27385 (net, refactor: extract Network and BIP155Network logic to node/network by jonatack)
  • #25797 (build: Add CMake-based build system 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.

MarcoFalke added 3 commits May 5, 2023 08:46
Instead of enumerating each passed env var, just pass all. This avoids
the risk of missing to enumerate one. Also, it is less code.

The risk could be that an env var causes non-deterministic behavior, but
this can be fixed by explicitly excluding it once the issue is known.

Values with newlines can not be stored in the file and parsed by
docker/podman, so they are excluded.
This cleans up 06_script_b.sh to only contain code to be executed inside
the CI pod, which avoids confusion and is needed for the next commit.
@maflcko maflcko force-pushed the 2305-ci-exec-no- branch from fa394a5 to fa1dbd0 Compare May 5, 2023 06:52
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 fa1dbd0 - this conflicts with #27125, but that is going to be rebased soon, and this could be merged in the interim. cc TheCharlatan

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.

ACK fa1dbd0

@fanquake
Copy link
Member

Merging this now. #27125 is going to be re-rebased to deal with the (minor) conflict. That PR is also waiting on at least one followup comment.

@fanquake fanquake merged commit 883766f into bitcoin:master May 10, 2023
@maflcko maflcko deleted the 2305-ci-exec-no- branch May 10, 2023 11:02
sidhujag pushed a commit to syscoin/syscoin that referenced this pull request May 10, 2023
@bitcoin bitcoin locked and limited conversation to collaborators May 9, 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