| Message ID | 16603d40fdf96948580c04a7c2b791a97ec64fe7.1712555682.git.ps@pks.im (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | t: exercise Git/JGit reftable compatibility | expand |
> [PATCH v2 03/12] ci: allow skipping sudo on dockerized jobs
I think this title is somewhat misleading. While it's true, I don't
think it's limited to dockerized jobs. Something more along the lines of
"allow running install-dependencies.sh as root" would make more sense to
me.
That's the only tiny remark I have on this patch series.
Toon claes <toon@iotcl.com> writes: >> [PATCH v2 03/12] ci: allow skipping sudo on dockerized jobs > > I think this title is somewhat misleading. While it's true, I don't > think it's limited to dockerized jobs. Something more along the lines of > "allow running install-dependencies.sh as root" would make more sense to > me. It is true that dockerized is not the essential part of this change; and it is that we skip sudo when we are already root. So I agree with you that the original title is misleading. "allow running as root" is somehow a bit unsatisfactory, though. It sounds as if _we_ were not allowing it before---what was preventing us from doing so was a system without sudo. Stepping back a bit, I wonder if install-dependencies.sh will stay to be the only one among ci/ scripts that need to run things as root. If we moved the new logic in this patch to ci/lib.sh that is included by everybody, then the title of the patch can become a short and sweet ci: skip sudo when we are already root > That's the only tiny remark I have on this patch series. Thanks.
On Wed, Apr 10, 2024 at 10:21:48AM -0700, Junio C Hamano wrote: > Toon claes <toon@iotcl.com> writes: > > >> [PATCH v2 03/12] ci: allow skipping sudo on dockerized jobs > > > > I think this title is somewhat misleading. While it's true, I don't > > think it's limited to dockerized jobs. Something more along the lines of > > "allow running install-dependencies.sh as root" would make more sense to > > me. > > It is true that dockerized is not the essential part of this change; > and it is that we skip sudo when we are already root. So I agree > with you that the original title is misleading. > > "allow running as root" is somehow a bit unsatisfactory, though. It > sounds as if _we_ were not allowing it before---what was preventing > us from doing so was a system without sudo. > > Stepping back a bit, I wonder if install-dependencies.sh will stay > to be the only one among ci/ scripts that need to run things as > root. If we moved the new logic in this patch to ci/lib.sh that is > included by everybody, then the title of the patch can become a > short and sweet > > ci: skip sudo when we are already root Agreed, this title reads much better than what I had. > > That's the only tiny remark I have on this patch series. > > Thanks. Thanks to both of you! Patrick
diff --git a/ci/install-dependencies.sh b/ci/install-dependencies.sh index 7d247b5ef4..7dfd3e50ed 100755 --- a/ci/install-dependencies.sh +++ b/ci/install-dependencies.sh @@ -11,6 +11,17 @@ UBUNTU_COMMON_PKGS="make libssl-dev libcurl4-openssl-dev libexpat-dev tcl tk gettext zlib1g-dev perl-modules liberror-perl libauthen-sasl-perl libemail-valid-perl libio-socket-ssl-perl libnet-smtp-ssl-perl" +# Make sudo a no-op and execute the command directly when running as root. +# While using sudo would be fine on most platforms when we are root already, +# some platforms like e.g. Alpine Linux do not have sudo available by default +# and would thus break. +if test "$(id -u)" -eq 0 +then + sudo () { + "$@" + } +fi + case "$distro" in ubuntu-*) sudo apt-get -q update
Our "install-dependencies.sh" script is executed by non-dockerized jobs to install dependencies. These jobs don't run with "root" permissions, but with a separate user. Consequently, we need to use sudo(8) there to elevate permissions when installing packages. We're about to merge "install-docker-dependencies.sh" into that script though, and our Docker containers do run as "root". Using sudo(8) is thus unnecessary there, even though it would be harmless. On some images like Alpine Linux though there is no sudo(8) available by default, which would consequently break the build. Adapt the script to make "sudo" a no-op when running as "root" user. This allows us to easily reuse the script for our dockerized jobs. Signed-off-by: Patrick Steinhardt <ps@pks.im> --- ci/install-dependencies.sh | 11 +++++++++++ 1 file changed, 11 insertions(+)