Set user environment variables permanently by using --permanent + deprecate --global

[aminya]

Closes #468

Fixes #467

This makes ./emsdk activate --global to set user environment variables by default instead of previous system-wide-only activation.

Also removes some magic that implicitly nukes user envvars when system ones are shadowed, as a system admin should understand what's going on when setting envvars. Thus fixes #189 and closes #192.

[sbc100]

According to the comments in #468 we decided it was best not to change existing meaning of --global.

I think the best option was to add two new options --persistent (meaning set in the registry for the current user) and --persistent-system meaning set in the registry for all users.

--global can the becomes a legacy alias for --persistent-system.

[aminya]

I deprecated --global. Now, we have --permanent to set the environment variables permanently.

[aminya]

I added the tests, which shows that pywin32 code part of emsdk is broken.

#91

Nothing is wrong with this PR, so I suggest that you merge it and instead try to fix the tests (probably by merging #91). Emsdk did not have these tests and this has caused many issues on Windows.

Otherwise, you can comment this line to bypass the broken tests: https://github.com/emscripten-core/emsdk/pull/621/files#diff-1d37e48f9ceff6d8030570cd36286a61R93 or I can make the tests a separate PR.

[saschanaz]

Yay, thanks for taking over!

[sbc100]

I've not had a chance to look at the new test script but generally this change lgtm!

[kripken]

Is the CI error the last issue here? (maybe also the permanent/persistent naming question)

[sbc100]

I think we need a tie breaker :) @kripken @juj how to do you feel about --persistent vs --permanent .

As a non-windows-user I don't care too much either way.

[aminya]

Is the CI error the last issue here?

This is not an issue of this PR. I added the tests, so we know that environment variables are not set on Windows (the behavior is broken). We need to fix the tests in other pull requests such as in #91.

If you run the same tests using the deprecated --global option on the master branch, they will still fail.

(maybe also the permanent/persistent naming question)

I don't feel strongly about this. I think --permanent is more descriptive. --persistent is a little fuzzy since it does not explicitly say persisting between what.

[sbc100]

Is the CI error the last issue here?

This is not an issue of this PR. I added the tests, so we know that environment variables are not set on Windows (the behavior is broken). We need to fix the tests in other pull requests such as in #91.

Can you remove the tests from this PR then? somehow disable them or mark them as currently known/expected to fail? We don't want to land a change with the CI being red.

[aminya]

Is the CI error the last issue here?

This is not an issue of this PR. I added the tests, so we know that environment variables are not set on Windows (the behavior is broken). We need to fix the tests in other pull requests such as in #91.

Can you remove the tests from this PR then? somehow disable them or mark them as currently known/expected to fail? We don't want to land a change with the CI being red.

I caught the failures in 2d7ccb2

This PR is ready to go.

[kripken]

I don't have a preference between --persistent vs --permanent .

I think if the code looks good to you @sbc100 , let's land it!

[sbc100]

lgtm with a couple more comments. Ready to land once they are addressed.

[sbc100]

Looks like the removal of the second call to causes the issue:

current_user_path = win_get_environment_variable('PATH', system=False)

Previously it was adding the user and system PATHs together I think.

I guess we should probably just ignore the registry when running in normal (non-persistent) mode and just look at the current os.environ['PATH']?