Basically it removes the above-mentioned env-vars as per MarcoFalke’s instructions. The only deviation from the plan laid out there was that I double-quoted the last instance of $ANDROID_HOME for the sake of consistency and future-proofing and the rest of the non-quoted vars due to lint failing the build.
Fixes #27321.
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.
See the guideline for information on the review process.
| Type | Reviewers |
|---|---|
| ACK | josibake, hernanmarino |
| Concept ACK | Ayush170-Future |
| Stale ACK | MarcoFalke |
If your review is incorrectly listed, please react with 👎 to this comment and the bot will ignore it on the next update.
No conflicts as of last run.
10-# replaced by a bash -c "command" directly, where needed.
11-# For now, they are named aliases to aid review with --color-moved=dimmed-zebra.
12-CI_EXEC_ROOT () { bash -c "$*"; }
13-CI_EXEC() { bash -c "$*"; }
14-export -f CI_EXEC_ROOT
15-export -f CI_EXEC
Yes, but the next time they are used in ci/test/04_install.sh, they are already redefined on lines 79-84:
CI_EXEC () {
$CI_EXEC_CMD_PREFIX bash -c "export PATH=${BINS_SCRATCH_DIR}:\$PATH && cd \"$P_CI_DIR\" && $*"
}
CI_EXEC_ROOT () {
$CI_EXEC_CMD_PREFIX_ROOT bash -c "export PATH=${BINS_SCRATCH_DIR}:\$PATH && cd \"$P_CI_DIR\" && $*"
}
This PR only relates to their usage in base ci/test/01_base_install.sh and potentially the env-var definitions in ci/test/00_setup_env_*.sh
./depends/ being owned by root) no longer applies as ./depends/ is a docker/podman volume.
However, this is probably for a follow-up.
./depends was the only folder affected, so removing the second user account and running as root seems fine.
21
22 if [[ $CI_IMAGE_NAME_TAG == *centos* ]]; then
23- ${CI_RETRY_EXE} CI_EXEC_ROOT dnf -y install epel-release
24- ${CI_RETRY_EXE} CI_EXEC_ROOT dnf -y --allowerasing install "$CI_BASE_PACKAGES" "$PACKAGES"
25+ ${CI_RETRY_EXE} bash -c "dnf -y install epel-release"
26+ ${CI_RETRY_EXE} bash -c "dnf -y --allowerasing install $CI_BASE_PACKAGES $PACKAGES"
0 ${CI_RETRY_EXE} dnf -y --allowerasing install $CI_BASE_PACKAGES $PACKAGES
I guess I know nothing about bash, but putting it this way may also work at no downside?
It is basically the same trick described in #27321 to be used for apt, dnf is basically the Redhat package manager
The other option would be to arrayify PACKAGES and CI_BASE_PACKAGES. I have that in my git stash too, but even though it is a cleaner way of doing it, it actually adds a couple extra lines when initialising the defaults. I value both cleanness and simplicity, so I cannot choose myself which way is better.
The interesting question is whether I should do the same for pip3 - otherwise the guy who adds a second python package would have to do the same research as me.
whether I should do the same for pip3
You are already doing what I suggested for pip3, which is why I left the comment here. I think it makes sense to use the same approach for all three pkg managers (dnf, apt, pip3)
Concept ACK
lgtm (although, CI is failing). happy to re-review once passing. also happy to review any follow-ups to remove the extra user account, since that was only introduced to solve the problem of CI changing file permissions on depends when running CI locally (which seems to no longer be an issue per #27333 (review))
ACK on the concept.
I’ve been following this since the start and have reviewed the changes once again. The approach followed looks good to me. If the checks fail again, I’d be happy to re-review (they are running currently).