Skip to content

Unset emsdk-related environment variables from inactive tools #801

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 1 commit into from
Apr 29, 2021

Conversation

sbc100
Copy link
Collaborator

@sbc100 sbc100 commented Apr 27, 2021

When we deactivate a tool we also want to remove its environment
variables. One driver for this is that modern sdks don't set
EM_CACHE whereas old ones did and we want to make sure that
EM_CACHE gets unset when folks upgrade (and then re-set if
they downgrade). See #797.

@sbc100 sbc100 requested a review from juj April 27, 2021 14:17
@sbc100
Copy link
Collaborator Author

sbc100 commented Apr 27, 2021

I should probably write test for this specifically around older SDKs and EM_CACHE...

@sbc100 sbc100 requested a review from dschuff April 28, 2021 16:18
@sbc100
Copy link
Collaborator Author

sbc100 commented Apr 28, 2021

In the long run, this change (combined with existing #797) should allow unity to move away from passing --cache to emcc since the point of doing that is to override the EM_CACHE environment variable set by emsdk.

@sbc100 sbc100 requested a review from kripken April 28, 2021 16:29
@sbc100 sbc100 changed the title Unset emsdk-related environment variable from inactive tools Unset emsdk-related environment variables from inactive tools Apr 28, 2021
@sbc100 sbc100 force-pushed the unset_old_env branch 2 times, most recently from 9771258 to 1e8edb0 Compare April 28, 2021 19:41
When we deactivate a tool we also want to remove its environment
variables.   One driver for this is that modern sdks don't set
`EM_CACHE` whereas old ones did and we want to make sure that
`EM_CACHE` gets unset when folks upgrade (and then re-set if
they downgrade).  See #797.
@sbc100 sbc100 enabled auto-merge (squash) April 28, 2021 22:03
@juj
Copy link
Collaborator

juj commented Apr 29, 2021

lgtm, though this maybe a good item to throw a notification to mailing list, since this will visibly change the semantics for users.

@sbc100 sbc100 disabled auto-merge April 29, 2021 15:43
@sbc100 sbc100 merged commit b4c9194 into master Apr 29, 2021
@sbc100 sbc100 deleted the unset_old_env branch April 29, 2021 15:43
radekdoulik referenced this pull request in dotnet/emsdk May 20, 2021
When we deactivate a tool we also want to remove its environment
variables.   One driver for this is that modern sdks don't set
`EM_CACHE` whereas old ones did and we want to make sure that
`EM_CACHE` gets unset when folks upgrade (and then re-set if
they downgrade).  See #797.
jmglogow added a commit to jmglogow/emsdk that referenced this pull request Nov 14, 2021
Introduced in commit b4c9194
("Unset emsdk-related environment variable from inactive tools
(emscripten-core#801)"), but no other match from "git log -GEMSDK_TTY".
jmglogow added a commit to jmglogow/emsdk that referenced this pull request Nov 14, 2021
Introduced in commit b4c9194
("Unset emsdk-related environment variable from inactive tools
(emscripten-core#801)"), but no other match from "git log -GEMSDK_TTY".
jmglogow added a commit to jmglogow/emsdk that referenced this pull request Nov 28, 2021
Introduced in commit b4c9194
("Unset emsdk-related environment variable from inactive tools
(emscripten-core#801)"), but no other match from "git log -GEMSDK_TTY".
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