Docker: avoid sourcing emsdk_env.sh in interactive shells

[kleisauke]

Instead, rely on the already set PATH environment variable.

---

This would ensure that a custom cache directory via EM_CACHE is honored rather than being ignored.

Current behavior:

$ docker run -it --rm -v $(pwd):/src -e EM_CACHE=/src/build/emcache emscripten/emsdk:3.1.39
...
Clearing existing environment variable: EM_CACHE
root@9c00b2231a23:/src# echo $EM_CACHE

New behavior:

$ docker run -it --rm -v $(pwd):/src -e EM_CACHE=/src/build/emcache emscripten/emsdk
root@7ed76e8b10f9:/src# echo $EM_CACHE
/src/build/emcache

Context: kleisauke/wasm-vips#49.

[sbc100]

Do we want to support this though? That point of emsdk_env is that it sets up the current env to point to emsdk tools (and emsdk cache). If we want to change this behaviour maybe we should change emsdk_env instead?

[sbc100]

Seeing this now I think we should remove EMSDK_NODE here.. its not used for anything that I know of.

[sbc100]

Also, EMSDK it only used locally within this file.. is there some way to declare it as local?

[kleisauke]

I removed the EMSDK_NODE env with commit f7c22d0.

The EMSDK env is probably still needed since it is also set by the emsdk_env.sh script, I clarified the origin of these env variables with commit 5d2b22c.

[kleisauke]

Do we want to support this though? That point of emsdk_env is that it sets up the current env to point to emsdk tools (and emsdk cache). If we want to change this behaviour maybe we should change emsdk_env instead?

The emsdk_env.sh script (and umask thing) was only run in interactive shells, see for example:

$ docker run -it --rm -v $(pwd):/src emscripten/emsdk:3.1.39 bash -c 'umask'
0022
$ docker run -it --rm -v $(pwd):/src emscripten/emsdk:3.1.39
Setting up EMSDK environment (suppress these messages with EMSDK_QUIET=1)
Setting environment variables:
PATH = /emsdk:/emsdk/upstream/emscripten:/usr/local/sbin:/usr/local/bin:/usr/sbin:/usr/bin:/sbin:/bin
root@8e8d456cdb1e:/src# umask
0000

So, it's therefore better to align the behavior of interactive and non-interactive shells (with the latter being much more common, I think).

[sbc100]

Do we want to support this though? That point of emsdk_env is that it sets up the current env to point to emsdk tools (and emsdk cache). If we want to change this behaviour maybe we should change emsdk_env instead?

The emsdk_env.sh script (and umask thing) was only run in interactive shells, see for example:

$ docker run -it --rm -v $(pwd):/src emscripten/emsdk:3.1.39 bash -c 'umask'
0022
$ docker run -it --rm -v $(pwd):/src emscripten/emsdk:3.1.39
Setting up EMSDK environment (suppress these messages with EMSDK_QUIET=1)
Setting environment variables:
PATH = /emsdk:/emsdk/upstream/emscripten:/usr/local/sbin:/usr/local/bin:/usr/sbin:/usr/bin:/sbin:/bin
root@8e8d456cdb1e:/src# umask
0000

So, it's therefore better to align the behavior of interactive and non-interactive shells (with the latter being much more common, I think).

The ideas is that emsdk_env is in charge to setting everything up.. but I guess we don't need that give that we are manually constructing that PATH.

However, emsdk_env is the official recommended way to setup the PATH so I guess I would prefer to have emsdk_env run in all cases.. and not manually inject upstream/bin into the PATH. Hardcoding those PATHs kind of goes against the idea of how emsdk is supposed to work.

[sbc100]

Can you split out the removal of fastcomp and EMSDK_NODE into its own separate PR?

[kleisauke]

The ideas is that emsdk_env is in charge to setting everything up.. but I guess we don't need that give that we are manually constructing that PATH.

However, emsdk_env is the official recommended way to setup the PATH so I guess I would prefer to have emsdk_env run in all cases.. and not manually inject upstream/bin into the PATH. Hardcoding those PATHs kind of goes against the idea of how emsdk is supposed to work.

I see. Unfortunately, you cannot set those ENV entries with the result of a command and RUN entries cannot modify environment variables (since they are executed in a new shell).

Another option would be to avoid the need for these environment variables altogether by installing the pre-built binaries in systems locations such as /usr or /usr/local; it will be isolated within the Docker image anyway. Though, I'm not sure how much work this would be.

Can you split out the removal of fastcomp and EMSDK_NODE into its own separate PR?

Sure, see PR #1225 and #1226.

[sbc100]

The ideas is that emsdk_env is in charge to setting everything up.. but I guess we don't need that give that we are manually constructing that PATH.
However, emsdk_env is the official recommended way to setup the PATH so I guess I would prefer to have emsdk_env run in all cases.. and not manually inject upstream/bin into the PATH. Hardcoding those PATHs kind of goes against the idea of how emsdk is supposed to work.

I see. Unfortunately, you cannot set those ENV entries with the result of a command and RUN entries cannot modify environment variables (since they are executed in a new shell).

Another option would be to avoid the need for these environment variables altogether by installing the pre-built binaries in systems locations such as /usr or /usr/local; it will be isolated within the Docker image anyway. Though, I'm not sure how much work this would be.

Can't we just add . /emsdk/.emsdk_env.sh to a place where it will always be run? Would ~/.bash_profile be better than ~/.back_profile?

Then we can remove the PATH additions above?

[kleisauke]

Closing in favor of #1227.

My only concern is that it would break compatibility for users that set the EM_CACHE env variable, which is useful for preserving cache across runs. However, one could use a custom entry point to fix that.