-
Notifications
You must be signed in to change notification settings - Fork 6
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Replace /bin/sh with a wrapper to /bin/bash #77
Conversation
08433be
to
d886a46
Compare
0ad7831
to
79df204
Compare
The Debian image test fails here - bioconda-containers/images/base-glibc-debian-bash/Dockerfile.test Lines 9 to 13 in ecfb6b5
It prints
i.e. there is a double activation ... |
This is needed to be able to set umask=022 for the Linux aarch64 images that use umask=027 by default. /bin/sh does not load /etc/profile nor any other rcfile. See bioconda/bioconda-recipes#46177 for full details and galaxyproject/galaxy#17631 for a summary. Signed-off-by: Martin Tzvetanov Grigorov <mgrigorov@apache.org>
79df204
to
f60cce5
Compare
Signed-off-by: Martin Tzvetanov Grigorov <mgrigorov@apache.org>
@@ -0,0 +1,3 @@ | |||
#!/usr/bin/env bash | |||
|
|||
bash "$@" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This one does not use BASH_ENV=/etc/profile
because the current tests in Dockerfile.test use sh -lc ...
and -l
/--login
causes double activation.
IMO all Dockerfile
s should use BASH_ENV=/etc/profile
and the tests should be adapted to not use -l
.
@daler The PR is ready for review! |
I'm not sure of all of the implications of this -- @mbargull and @bgruening can you weigh in on this? I can't think of cases where changing the umask would be problematic. But is it OK to overwrite |
I wonder why I face this issue with umask only with the bioconductor-**
recipes …
Why/how others mulled tests pass ?!
…On Tue, 12 Mar 2024 at 15:54, Ryan Dale ***@***.***> wrote:
I'm not sure of all of the implications of this -- @mbargull
<https://github.com/mbargull> and @bgruening
<https://github.com/bgruening> can you weigh in on this?
I can't think of cases where changing the umask would be problematic.
But is it OK to overwrite /bin/sh with a wrapper that uses bash?
—
Reply to this email directly, view it on GitHub
<#77 (comment)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/AABYUQS6VJTIU4XKIMYI4E3YX4CIXAVCNFSM6AAAAABEP53HVOVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMYTSOJRG4YDOMJYG4>
.
You are receiving this because you authored the thread.Message ID:
***@***.***>
|
No, not OK at all.
Sorry, I'm aware this is not helpful to resolve you problems. |
Thanks for the comments, @mbargull !
This is true! But the problem is that
And because of this One more (minor?!) issue:
/etc/profile.d/conda.sh instead, so any global settings in /etc/profile are missed again.
I will close this issue because as you said the proposed changes may cause big differences for others! |
Hi @martin-g, sorry for not getting back to you earlier.
Right, I completely forgot that I kept the larger one mostly "vanilla Debian" in that regard.
Setting Generally, we don't want to run interactive/login scripts for these cases. FWIW, I cannot reproduce the behavior you described locally. The discussion around this seem to be scatter around multiple issues/PRs; I don't have an overview about this -- do we have some central issue to discuss this? |
Hi Marcel,
Did you test on Linux ARM64 machine/VM or using —platform=linux-arm64 and
QEMU ?
Please check
bioconda/bioconda-recipes#46177
<bioconda/bioconda-recipes#46177 (comment)>
It has all the history of my investigation.
My latest solution uses galaxy-until-tool from their dev branch and it
solves the problem by using “-l”
I’m on vacation until Wednesday so I cannot test anything but I can answer
questions here!
…On Sat, 30 Mar 2024 at 11:23, Marcel Bargull ***@***.***> wrote:
Hi @martin-g <https://github.com/martin-g>, sorry for not getting back to
you earlier.
base-glibc-debian-bash points to /bin/dash !
Right, I completely forgot that I kept the larger one mostly "vanilla
Debian" in that regard.
Thanks for the correction!
One more (minor?!) issue: /bin/sh (without -l) does not read any rcfile
(like /etc/profile). With -l it would read it but create-env Docker image
specifies a custom ENV env var (
https://github.com/bioconda/bioconda-containers/blob/ecfb6b5e9bbd5944890e92e6587fabc00153c96d/images/create-env/Dockerfile#L41)
that tells it to read /etc/profile.d/conda.sh instead, so any global
settings in /etc/profile are missed again.
Setting ENV is for interactive use and does not prevent sourcing login
shell specific files (e.g, /etc/profile).
The differences between
non-login-non-interactive/non-login-interactive/login-non-interactive/login-interactive
shell invocations and between POSIX-y
shell/Bash-specifics/other-shell-specifics can be confusing/daunting.
There are many different things that can come into play under different
circumstances (/etc/profile, ~/.profile, ~/.bash_profile, ~/.bash_login,
/etc/bash.bashrc, ~/.bashrc, ENV, BASH_ENV, and more env vars/arguments
-- and they all have different meanings/implications).
We should take care about understanding implications of build setup
changes around this since it can (negatively) affect the products of such
setups (e.g., our packages/containers).
The aforementioned
https://manpages.debian.org/bookworm/bash/bash.1.en.html#INVOCATION gives
a reasonably good overview about this, IMO.
(I'm somewhat familiar with the differences, but also have to refresh my
memory on this regularly with all this differences/niche uses.)
Generally, we don't want to run interactive/login scripts for these cases.
Meaning, the shell invocations you mentioned should remain sh -c without
any -i/-l.
(And sh -c is supposed to behave the same as sh some-script.sh in regards
to not sourcing any custom scripts; if they were to allow running anything
from /etc/profile or elsewhere, then a simple echo exit 1 > /etc/profile
would pretty much break your system.)
------------------------------
FWIW, I *cannot* reproduce the behavior you described locally.
For me, all container images I tested for amd64/arm64 with or without
running through their entry points, yield umask of 0022 for any sh/bash
combination of -c/-i/-l (tested with mainly with podman but also checked
with the latest docker in case something fishy would happen there).
(This is locally on an x86-64 machine with the ARM64 bits running via
QEMU, which should not matter, but noting for completeness' sake.)
So, to me it is not clear at all where the observed 0027 stems from.
------------------------------
The discussion around this seem to be scatter around multiple issues/PRs;
I don't have an overview about this -- do we have some central issue to
discuss this?
—
Reply to this email directly, view it on GitHub
<#77 (comment)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/AABYUQQUQV4DORZUX2GVKRTY2ZZAZAVCNFSM6AAAAABEP53HVOVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDAMRXHE4DQMZXGE>
.
You are receiving this because you were mentioned.Message ID:
***@***.***>
|
Hi Martin,
QEMU (as noted above); i.e., only user space via
Hm... I don't think that's a good idea, TBH. That's rather a workaround for whatever underlying issue and might mask (no pun intended..) other problems. I've let this run on CI in bioconda/bioconda-recipes#46900 and we get the same output for So, still no sign of the I still haven't looked into what/how happens for the CI build failures (meaning, not looked at the logs you pointed out at all).
I'll try to see if I can understand/reproduce things on CI. |
On Sat, 30 Mar 2024 at 13:12, Marcel Bargull ***@***.***> wrote:
Hi Martin,
Did you test on Linux ARM64 machine/VM or using —platform=linux-arm64 and
QEMU ?
QEMU (as noted above); i.e., only user space via binfmt_misc.
My latest solution uses galaxy-until-tool from their dev branch and it
solves the problem by using “-l”
Hm... I don't think that's a good idea, TBH. That's rather a workaround
for whatever underlying issue and might mask (no pun intended..) other
problems.
Probably you are right!
The mask issue is easily reproducible on the ARM64 VM I use but it might be
issue with Docker, as you said.
Using “-l” solved the issue locally and at CircleCI so I didn’t pursue
other solutions.
I've let this run on CI in bioconda/bioconda-recipes#46900
<bioconda/bioconda-recipes#46900> and we get the
same output for amd64/arm64 images there, i.e., umask of 0022 for root
and 0002 for non-root login shells (also non-root non-login interactive
Bash which I didn't expect; not that it matters, but it surprised me).
So, still no sign of the 0027 you observed. (Really no idea there; do you
run some old Docker that does things differently?)
I still haven't looked into what/how happens for the CI build failures
(meaning, not looked at the logs you pointed out at all).
One thing I could imagine, would be package building happening with root
user (i.e., overriding the entrypoint which I think we did for some reason
at some point -- I'll have to check if that's still the case) and then
reusing those package files as the non-root ("conda") user when creating
the environment for Biocontainers (i.e., umask 0022 from root preventing
writes from conda user).
^-- But that should've also happen for x86-64 builds since the
setup/umasks seem to be the same on CI...
Actually it have seen it for x86_64 too.
If an error happens (like this one) then it falls back to “remote”
execution of involucro, i.e. it tries to use a Docker image
“Involucro/tool” that’s amd64 only and this hides the earlier problem.
I’m on vacation until Wednesday so I cannot test anything but I can answer
questions here!
I'll try to see if I can understand/reproduce things on CI.
Enjoy your vacation!
Thank you for your help!
—
… Reply to this email directly, view it on GitHub
<#77 (comment)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/AABYUQSXNBWPGVMLWRMVUQTY22FZ3AVCNFSM6AAAAABEP53HVOVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDAMRYGAYTKOJSGU>
.
You are receiving this because you were mentioned.Message ID:
***@***.***>
|
Oh, wow, that's ugly! Thanks for making me aware of that! |
On Sat, 30 Mar 2024 at 14:06, Marcel Bargull ***@***.***> wrote:
Actually it have seen it for x86_64 too. If an error happens (like this
one) then it falls back to “remote” execution of involucro, i.e. it tries
to use a Docker image “Involucro/tool” that’s amd64 only and this hides the
earlier problem.
Oh, wow, that's ugly! Thanks for making me aware of that!
The logic is in mulled-build tool (galaxy-util-tool).
On ARM64 it fails with format exec error.
—
… Reply to this email directly, view it on GitHub
<#77 (comment)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/AABYUQXUKSHASY472PCYNNLY22MDXAVCNFSM6AAAAABEP53HVOVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDAMRYGAZDSNBZGE>
.
You are receiving this because you were mentioned.Message ID:
***@***.***>
|
@mbargull Here is an fresh example of the problem on x86_64 - bioconda/bioconda-recipes#47373
Similar logs but for aarch64 fail with:
|
@mbargull Here is the simplest reproducer I could do:
It fails for me on both Linux aarch64 and x86_64! x86_64 logs:
aarch64 logs:
|
|
I think you were right about the cause. |
Hi @martin-g, I'm trying to get up to speed on this issue in case I can help. Thanks for all the work you've put into it so far. This seems really tricky to track down. I did notice that the example you made above actually seems to be a different error. It's in mulled-build where it's cleaning up any leftover build directory while the CircleCI error is in the packing step. In your example, packing succeeds. Also on my linux (amd64) local test, if I do Have you seen the error in CircleCI for any other files except this particular LICENCE.txt? I noticed that if I install font-ttf-ubuntu from conda-forge and look in my pkgs, the file shows I'll let you know if I figure anything out. |
Hi @aliciaaevans ! The error in Bioconda recipes build is always the same - this licence from font-ttf-ubuntu. |
I made a comment on the original PR. It's probably better to continue the discussion there. bioconda/bioconda-recipes#46177 (comment) |
This is needed to be able to set umask=022 for the Linux aarch64 images that use umask=027 by default.
/bin/sh does not load /etc/profile nor any other rcfile.
See bioconda/bioconda-recipes#46177 for full details and galaxyproject/galaxy#17631 for a summary.