Skip to content
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

containerized conda environment broken in 3.8.1 #274

Closed
GodloveD opened this issue Aug 25, 2021 · 5 comments · Fixed by #278
Closed

containerized conda environment broken in 3.8.1 #274

GodloveD opened this issue Aug 25, 2021 · 5 comments · Fixed by #278
Assignees
Labels
bug Something isn't working

Comments

@GodloveD
Copy link
Contributor

Version of Singularity

$ singularity version
singularity --version
singularity-ce version 3.8.1

Description of bug
We have an existing container with an installed conda environment that works with older versions of Singularity including v3.8.0, but does not work with v3.8.1.

To Reproduce
Use the following def file to create a container and run it with v3.8.0 and v3.8.1 like so:

$ cat >conda.def<<"EOF"
> Bootstrap: docker
> From: continuumio/miniconda3:latest
>
> %post
>         . /opt/conda/etc/profile.d/conda.sh
>         conda create -n env python=3
>
> %environment
>         source /opt/conda/etc/profile.d/conda.sh
>         conda activate env
>
> EOF

$ sudo singularity build conda.sif conda.def
INFO:    Starting build...
Getting image source signatures
Copying blob 33847f680f63 skipped: already exists
Copying blob f5a80bcd1413 skipped: already exists
Copying blob 8d0d14d1334a [--------------------------------------] 0.0b / 0.0b
Copying config d17c4c4005 done
[snip...]
INFO:    Build complete: conda.sif

$ /opt/singularity/3.8.0/bin/singularity --version
singularity-ce version 3.8.0

$ /opt/singularity/3.8.0/bin/singularity exec conda.sif true

$ /opt/singularity/3.8.1/bin/singularity --version
singularity-ce version 3.8.1

$ /opt/singularity/3.8.1/bin/singularity exec conda.sif true
/.singularity.d/actions/exec: 5: /.singularity.d/env/90-environment.sh: source: not found

CommandNotFoundError: Your shell has not been properly configured to use 'conda activate'.
To initialize your shell, run

    $ conda init <SHELL_NAME>

Currently supported shells are:
  - bash
  - fish
  - tcsh
  - xonsh
  - zsh
  - powershell

See 'conda init --help' for more information and options.

IMPORTANT: You may need to close and restart your shell after running 'conda init'.

Expected behavior
I expect the container to work the same in 2 different versions of Singularity.

OS / Linux Distribution
Which Linux distribution are you using?

$ cat /etc/os-release
NAME="Ubuntu"
VERSION="18.04.5 LTS (Bionic Beaver)"
ID=ubuntu
ID_LIKE=debian
PRETTY_NAME="Ubuntu 18.04.5 LTS"
VERSION_ID="18.04"
HOME_URL="https://www.ubuntu.com/"
SUPPORT_URL="https://help.ubuntu.com/"
BUG_REPORT_URL="https://bugs.launchpad.net/ubuntu/"
PRIVACY_POLICY_URL="https://www.ubuntu.com/legal/terms-and-policies/privacy-policy"
VERSION_CODENAME=bionic
UBUNTU_CODENAME=bionic

Installation Method
source

@dtrudg
Copy link
Member

dtrudg commented Aug 25, 2021

Thanks for the report @GodloveD

Confirmed on master also. This is related to the mvdan.cc/sh dependency update pulled in in CE 3.8.1. Downgrading to v3.2.4 of mvdan.cc/sh in go.mod without making any code changes appears to fix the issue:

diff --git a/go.mod b/go.mod
index b534aed3c..ef3ee8243 100644
--- a/go.mod
+++ b/go.mod
@@ -63,7 +63,7 @@ require (
        golang.org/x/sys v0.0.0-20210630005230-0f9fa26af87c
        gopkg.in/yaml.v2 v2.4.0
        gotest.tools/v3 v3.0.3
-       mvdan.cc/sh/v3 v3.3.1
+       mvdan.cc/sh/v3 v3.2.4
        oras.land/oras-go v0.4.0
        rsc.io/letsencrypt v0.0.3 // indirect
 )

... then go mod tidy and rebuild...

dtrudg@ythel:~
03:13 PM $ singularity exec conda.sif true
dtrudg@ythel:~
03:13 PM $ 

Need to have a regression test added for source within %environment, and investigate further what is going on here. Release notes for the module don't give much of a clue.

@dtrudg dtrudg self-assigned this Aug 25, 2021
@dtrudg
Copy link
Member

dtrudg commented Aug 25, 2021

Noting that replacing source /opt/conda/etc/profile.d/conda.sh with . /opt/conda/etc/profile.d/conda.sh appears to work.

@dtrudg
Copy link
Member

dtrudg commented Aug 25, 2021

Guessing the default interpreter mode might be trying to emulate POSIX sh now, rather than bash-isms by default... I had forgotten that source is a bash-ism:

03:41 PM $ sh -c "source bob"
sh: 1: source: not found

dtrudg@ythel:~
03:41 PM $ bash -c "source bob"
HELLO!
03:43 PM $ shellcheck test.sh 

In test.sh line 3:
source bob
^--------^ SC2039: In POSIX sh, 'source' in place of '.' is undefined.
       ^-^ SC1091: Not following: bob was not specified as input (see shellcheck 

So this is technically correct if we are saying %environment is POSIX sh, but admittedly confusing where it worked before!

Will look at restoring previous behavior.

@dtrudg
Copy link
Member

dtrudg commented Aug 25, 2021

Did a bisection of the commits on mvdan.cc/sh and isolated it to this change:

mvdan/sh@fb5052e

Does not appear to be intended POSIX strictness. Doh.

bbb14b7598a3e9073ce85b4b2cf8be43b245aa6e WORKS (FEB 16)
d0b20e4809ed3afb0bbb186eae7a83932c7ad8a9 WORKS (FEB 22)
f4c774aa15046ef006508e182fde10c4b56876fa WORKS (FEB 22)
c65af2c31c09630473081dd7dedd810b5a359463 WORKS (FEB 24)
---
fb5052e7a0109c9ef5553a310c05f3b8c04cca5f FAILS (FEB 24)
ef9f6aa8b2a1b9a46cf5b9b09094d4667750563f FAILS (FEB 25)
4169a2c7edff6c77678b12d3dc2123e967b46a2e FAILS (MAR 22)

dtrudg added a commit to dtrudg/singularity that referenced this issue Aug 26, 2021
Changes to exit handling with the implementation of trap in mvdan.cc/sh
mean that we need to interpret indibidual Stmt, rather than the parse
File when we are handling built-ins prefixed with `\` in
`internalExecHandler`.

Fixes sylabs#274
@dtrudg
Copy link
Member

dtrudg commented Aug 26, 2021

Turns out to be a buried side-effect of the trap changes in mvdan.cc/sh, due to the fact the conda script has \ prefixed builtin statements e.g. \export that we handle via a custom ExecHandler.

We are parsing these statements via syntax.NewParser().Parse() which returns a syntax.File. We then interpret that syntax.File. With the changes in mvdan.cc/sh this infers an exit, and we are seeing a spurious error.

Ref: mvdan/sh@fb5052e#diff-67969f7525614604b02319c2d9559235365266e9e657fac2240700ab024c269bR503

The fix is to run the parsed statements, which avoids the implied exit. PR'd as #278

dtrudg added a commit to dtrudg/singularity that referenced this issue Aug 26, 2021
Changes to exit handling with the implementation of trap in mvdan.cc/sh
mean that we need to interpret individual `syntax.Stmt`, rather than a
full parsed `syntax.File` when we are handling built-ins prefixed with
`\` in `internalExecHandler`.

Fixes sylabs#274
dtrudg added a commit to dtrudg/singularity that referenced this issue Aug 26, 2021
Changes to exit handling with the implementation of trap in mvdan.cc/sh
mean that we need to interpret individual `syntax.Stmt`, rather than a
full parsed `syntax.File` when we are handling built-ins prefixed with
`\` in `internalExecHandler`.

Fixes sylabs#274
dtrudg added a commit to dtrudg/singularity that referenced this issue Aug 26, 2021
Changes to exit handling with the implementation of trap in mvdan.cc/sh
mean that we need to interpret individual `syntax.Stmt`, rather than a
full parsed `syntax.File` when we are handling built-ins prefixed with
`\` in `internalExecHandler`.

Fixes sylabs#274
DrDaveD pushed a commit to DrDaveD/singularity that referenced this issue Aug 30, 2021
Changes to exit handling with the implementation of trap in mvdan.cc/sh
mean that we need to interpret individual `syntax.Stmt`, rather than a
full parsed `syntax.File` when we are handling built-ins prefixed with
`\` in `internalExecHandler`.

Fixes sylabs/singularity#274
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants