Repository navigation
Conversation
Tools/build/regen-configure.sh ran the autoconf container as root, so it left a root-owned autom4te.cache/ in the source tree. The invoking user could neither write into it (making a later `make autoconf` fail with "cannot open autom4te.cache/requests: Permission denied") nor delete it (making `git clean -xdf` warn for every file in it). Run the container as the invoking user, and purge autom4te.cache before and after the run from inside the container, so leftovers owned by root are also removed without needing sudo.
itamaro
left a comment
There was a problem hiding this comment.
thanks for the fix!
as is, this breaks rootless podman/docker. see inline comments.
|
|
||
| purge_autom4te_cache | ||
| trap purge_autom4te_cache EXIT | ||
| "$RUNTIME" run --rm -u "$(id -u):$(id -g)" -v "$abs_srcdir:$WORK_DIR$PATH_OPT" "$IMAGE" |
There was a problem hiding this comment.
this breaks rootless Podman/Docker. container root is already mapped to the host user, so it works without this PR. with -u $(id -u):$(id -g) the container runs as a uid that maps to a host subuid with no write access to the bind mount, and autoreconf fails (verified with rootless podman 4.9: mkdir: can't create directory '/src/autom4te.cache': Permission denied).
possible fix:
USER_OPT=""
case "$RUNTIME" in
podman) USER_OPT="--userns=keep-id" ;;
docker) docker info --format '{{.SecurityOptions}}' | grep -q rootless || USER_OPT="-u $(id -u):$(id -g)" ;;
esacThere was a problem hiding this comment.
Good catch — applied your suggested fix: USER_OPT now uses --userns=keep-id for Podman and -u $(id -u):$(id -g) only for rootful Docker (detected via docker info).
| purge_autom4te_cache() { | ||
| "$RUNTIME" run --rm -v "$abs_srcdir:$WORK_DIR$PATH_OPT" \ | ||
| --entrypoint sh "$IMAGE" -c "rm -rf $WORK_DIR/autom4te.cache" | ||
| } |
There was a problem hiding this comment.
nit: entry.sh in the image already does exec "$@", so the --entrypoint is unnecessary
nit 2: you could check if autom4te.cache exists, and skip the container run if it doesn't
There was a problem hiding this comment.
Done — dropped --entrypoint and added an early return that skips the container run when autom4te.cache doesn't exist.
| } | ||
|
|
||
| purge_autom4te_cache | ||
| trap purge_autom4te_cache EXIT |
There was a problem hiding this comment.
nit: trap 'purge_autom4te_cache || true' EXIT so a cleanup failure doesn't hide the exit code from autoreconf
There was a problem hiding this comment.
Done — changed to trap 'purge_autom4te_cache || true' EXIT.
|
A Python core developer has requested some changes be made to your pull request before we can consider merging it. If you could please address their requests along with any other requests in other reviews from core developers that would be appreciated. Once you have made the requested changes, please leave a comment on this pull request containing the phrase |
|
I have made the requested changes; please review again |
|
The |
Summary
Tools/build/regen-configure.sh ran the Autoconf container as root, so Autoconf left a root-owned autom4te.cache/ in the source tree. Being a directory owned by root and mode 0755, it cannot be written to or removed by the invoking user, which breaks make autoconf afterwards (autom4te: error: cannot open autom4te.cache/requests: Permission denied) and makes git clean -xdf fail with warning: failed to remove autom4te.cache/...: Permission denied. Recovering needs sudo, which not every developer has.
Changes
Run the container as the invoking user (-u "$(id -u):$(id -g)"), so the regenerated configure, aclocal.m4 and pyconfig.h.in are not left owned by root either.
Remove $WORK_DIR/autom4te.cache before the run and from an EXIT trap, running the cleanup inside the container so that a stale cache left by an earlier root run can be removed without sudo.
The cleanup reuses the same image via --entrypoint sh, so no extra dependency (e.g. busybox) is pulled in.
Why the cache cannot simply be rm'd from the script
The invoking user has no write permission on a root-owned autom4te.cache/, so it has to be deleted as root — i.e. from inside the container. Doing it before the run also matters: a stale root-owned cache makes even the corrected, non-root run fail immediately.
Why the cache cannot simply be rm'd from the script
The invoking user has no write permission on a root-owned autom4te.cache/, so it has to be deleted as root — i.e. from inside the container. Doing it before the run also matters: a stale root-owned cache makes even the corrected, non-root run fail immediately.
issue: #159452