Skip to content

Avoid writing to file in emsdk_env.csh / emsdk_env.sh #544

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

Merged
merged 2 commits into from
Jul 11, 2020
Merged

Conversation

sbc100
Copy link
Collaborator

@sbc100 sbc100 commented Jul 9, 2020

This is an alternative fix for
emscripten-core/emscripten#9090 which recently
came up again after #539.

Tested with bash, tcsh and fish.

@sbc100 sbc100 requested a review from tlively July 9, 2020 22:20
This is an alternative fix for
emscripten-core/emscripten#9090 which recently
came up again after #539.

Tested with bash, tcsh and fish.
Copy link
Member

@tlively tlively left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Neat! Am I correct that this does not change the behavior on Windows?

@sbc100
Copy link
Collaborator Author

sbc100 commented Jul 10, 2020

Yeah.. the race condition still exist on windows... but its always be there and nobody seems to complain. i imagine if you had emsdk_env.bat in your shell startup and up opened a lot of shells at the same time you might see this issue.

Honestly I don't know enough about windows .bat files to know how to the same thing there..

@sbc100 sbc100 merged commit d37abed into master Jul 11, 2020
@sbc100 sbc100 deleted the avoid_env_file branch July 11, 2020 00:49
kichikuou added a commit to kichikuou/setup-emsdk that referenced this pull request Jul 11, 2020
emscripten-core/emsdk#544 changed `emsdk construct_env` to print
messages to stderr.
sbc100 added a commit that referenced this pull request Jul 14, 2020
This wasa temporary file that `construct_env` used to create.  Now
it just writes to stdout (as of #544).
sbc100 added a commit that referenced this pull request Jul 14, 2020
This wasa temporary file that `construct_env` used to create.  Now
it just writes to stdout (as of #544).
sbc100 added a commit that referenced this pull request Jul 14, 2020
This was an internal script that we used to generate up
until #544.   Now we avoid writing that file at all and
emsdk_env.sh is instead sourced directly.

This should fix the docker CI issues we have been having.
sbc100 added a commit that referenced this pull request Jul 14, 2020
This was an internal script that we used to generate up
until #544.   Now we avoid writing that file at all and
emsdk_env.sh is instead sourced directly.

This should fix the docker CI issues we have been having.
sbc100 added a commit that referenced this pull request Jul 14, 2020
This was an internal script that we used to generate up
until #544.   Now we avoid writing that file at all and
emsdk_env.sh is instead sourced directly.

This should fix the docker CI issues we have been having.
sbc100 added a commit that referenced this pull request Jul 14, 2020
This was an internal script that we used to generate up
until #544.   Now we avoid writing that file at all and
emsdk_env.sh is instead sourced directly.

This should fix the docker CI issues we have been having.
@fibbo
Copy link

fibbo commented Aug 4, 2020

Is there a reason why

emsdk/emsdk.py

Line 2588 in e88a3c5

log_stderr('Adding directories to PATH:')
and the following loop logs now to stderr?

@sbc100
Copy link
Collaborator Author

sbc100 commented Aug 4, 2020

Is there a reason why

emsdk/emsdk.py

Line 2588 in e88a3c5

log_stderr('Adding directories to PATH:')

and the following loop logs now to stderr?

Yes, it is so that the stdout of construct_env can be directly evaluated by the shell.

eval `emsdk construct_env`

This is technique that I borrowed from ssh-agent:

eval `ssh-agent`

And also from homebrew (https://docs.brew.sh/Homebrew-on-Linux):

eval $($(brew --prefix)/bin/brew shellenv)

Is it causing trouble? We could consider simply removing the logging.

@fibbo
Copy link

fibbo commented Aug 5, 2020

Running

emsdk activate <version> and
emcc

in a Python script via the subprocess module the emcc command fails since the environment variables set in the first command are not set in the second command since it uses a new shell. And chaining them with && also due to the way the windows command prompt works (https://superuser.com/questions/1481929/why-is-set-path-0path-echo-path-printing-the-old-environment). So my solution was to store the output created by the emsdk activate command and parse it (I'm interested in the PATH += lines) and add the environment variables inside my python script to the environment.
I was only reading stdout and since the change it fails again since the path variables are sent to stderr. I read now both from stdout and stderr and my script works again. I was mostly just wondering.

But please don't remove the logging :)

@sbc100
Copy link
Collaborator Author

sbc100 commented Aug 5, 2020

That doesn't seem like a very sustainable solution. I agree we should work on finding one, but currently the output of these commands is not designed to machine parse-able like that and as far as I know the output logged during these commands is not guaranteed not to change.

But you pose an interesting problem here. Normally the answer would be that you should install and activate the SDK before running the python script, or re-start python to pick up the new environment variable, but I can see how that is not a very satisfying answer.

Most other SDKs that I know of have the same problem (e.g. homebrew, or fuschia). In emsdk as with those other SDKs the environment variables would need to be set in the calling shell (the shell that loads python).

Is there any way you can change your setup to work this way around? If not, I can try to work on a more long-term solution for you that doesn't involve parsing log messages.

@fibbo
Copy link

fibbo commented Aug 6, 2020

I'm not sure how else to setup the emscripten environment.

The Python script drives our wasm build (which itself is using CMake). Probably a way around it would be using a batch or powershell script which takes over the activation of the sdk and then call the python script from within that batch/ps script. Adding another script file is not something I would like to do.

But I also understand that there can be no guarantee that I can rely on parsing the emsdk output.

@sbc100
Copy link
Collaborator Author

sbc100 commented Aug 6, 2020

I think the best solution is for you to run emsdk construct_env then parse stdout, and discard stderr.

The stdout of construct_env is guaranteed to be in shell form and comes in the form of export XXX=YYY.

e.g on my machine I get:

export PATH="....";
export EMSDK="/usr/local/google/home/sbc/dev/wasm/emsdk";
export EM_CONFIG="/usr/local/google/home/sbc/dev/wasm/emsdk/.emscripten";
export EMSDK_NODE="/usr/local/google/home/sbc/dev/wasm/emsdk/node/12.18.1_64bit/bin/node";

So you it should be pretty easy to parse and inject into your env.

@fibbo
Copy link

fibbo commented Aug 7, 2020

The output from construct_env seems to differ on windows:

image

Which is identical to the emsdk activate output and it seems that everything is written to stderr

emsdk construct_env > env.txt leaves env.txt empty. Only emsdk construct_env 2> env.txt writes something to the file (the output from above).

@sbc100
Copy link
Collaborator Author

sbc100 commented Aug 7, 2020

Yes emsdk construct_env on windows doesn't write to stdout, because AFAIK there is no way to do something like eval on windows. Instead it writes to a file called emsdk_set_env.bat which you can then execute.

@sbc100
Copy link
Collaborator Author

sbc100 commented Aug 7, 2020

(Actually the emsdk.bat file will execute emsdk_set_env.bat and then delete it before it returns so the file will not persist).

If you want to see the contents of emsdk_set_env.bat you can run python esmdk.py construct_env (avoiding the emsdk.bat launcher that reads and deletes the file), or you can remove the del command from the end of emsdk.bat.

@fibbo
Copy link

fibbo commented Aug 10, 2020

This is very helpful, thanks!

One more thing for the future to consider is when more than one version can be installed at the same time that the construct_env command probably should also support a version as argument.

@sbc100
Copy link
Collaborator Author

sbc100 commented Aug 10, 2020

That is why we have activate. Only one set of tools can be active at a given time, even if more than one can be installed.

@fibbo
Copy link

fibbo commented Aug 11, 2020

I think I don't understand when to use construct_env and when to use activate. Is activate just taking it a step further while construct_env by itself doesn't do anything (you need eval ... to actually do something)?

Because as I understand construct_env also adds the location of emcc to the path - and this is actually dependent on the version.

@sbc100
Copy link
Collaborator Author

sbc100 commented Aug 11, 2020

activate is something you just run once. It creates the config file that points to the active tools.

source emsdk_env.sh (which uses construct_env internally) is something you need to run each time you start a shell (can can add it to your .bashrc for example). It injects PATH and other environment variables into the current environment.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants