Set user environment variables by default when giving --global in Windows

[saschanaz]

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]

This change looks great!

The only slight concern I have is changing the default. Do you know that history here? Why would setting system-wide have been desirable? Under what circumstances would it still be desirable? Do we need the option at all? Maybe we simply remove the option and always set user settings?

@juj do you remember why this was done this way in the first place?

[sbc100]

Looks like the log_additions param is never uses, or passed. Unrelated to this PR but since your are changing that line maybe delete it?

[juj]

The intent of calling emsdk activate has been to only enable the env. vars for current/calling user prompt, not do any permanent modifications to the system.

Then, emsdk activate --global is intended to apply permanent modifications to "All users" section of the registry.

(The doc piece at emsdk --help looks like up to date with this respect:

    if WINDOWS:
      print('''
   emsdk activate [--global] [--embedded] [--build=type] [--vs2013/--vs2015/--vs2017] <tool/sdk>

                                - Activates the given tool or SDK in the
                                  environment of the current shell. If the
                                  --global option is passed, the registration
                                  is done globally to all users in the system

There has never existed an option to perform "Current user" only registry activation. If that is desirable, perhaps it would be good to have that as a new option emsdk activate --local or emsdk activate --local_user, instead of changing the existing emsdk activate from "current prompt only" to "permanent local user"?

[saschanaz]

If that is desirable, perhaps it would be good to have that as a new option emsdk activate --local or emsdk activate --local_user, instead of changing the existing emsdk activate from "current prompt only" to "permanent local user"?

Sorry for misleading description, but no, this doesn't change emsdk activate to set the permanent user global environment, instead it modifies emsdk activate --global to be user scope by default.

I think defaulting to user global environment with --global is better as it does not require admin privilege.

[juj]

I think defaulting to user global environment with --global is better as it does not require admin privilege.

It feels like changing --global from meaning global to user local would be counterintuitive? A new option --local would be a better name?

[saschanaz]

IMO it's global user variables and global system variables, not "local", which sounds like non-permanent local shell variable. (Like in this page for example)

[sbc100]

I also feel like changing the meaning of an existing flag I a little confusing/misleading. How about simply removing the old --global flag which is ambiguous to something more specific such as --persistent or --registry?

Any users of the old --global flag could be asked to be more specific and switch to either --persistent or --persistent + --system.

[saschanaz]

--persistent sounds good to me, as --global can be an alias for --persistent --system then.

[juj]

--persistent sounds good, although I would recommend instead of --persistent --global to have --persistent_global, since the flags are not orthogonal: passing --global on its own would not be meaningful.

[saschanaz]

Does --persistent and --persistent-global --persistent-system sound good? Those should be mutually exclusive.

[sbc100]

I like --persistent-system and you could even keep the old --global flag as an alias to that or while/

[sbc100]

Any update on this? I think it would be good change overall.

[saschanaz]

I'll try get some time this weekend.

[sbc100]

Any update?

[aminya]

This is a very good change. What is blocking this from merging?

Can someone with write access update this branch?

[aminya]

If arg_system is provided but arg_global is not, it should automatically set that. In other words, system means permanent too!

Suggested change

	arg_system = extract_bool_arg('--system')
	arg_system = extract_bool_arg('--system')
	if arg_system
	arg_global=True

[aminya]

If there is anywhere to document the meanings of these options, we should.

global: set the environment variables permanently.
system: set the environment variables for all users of the system.

[sbc100]

emsdk --help is the main place we document this stuff.

[saschanaz]

I have to admit that I'm not actively using emsdk nowadays. I still intend to work on it but if anyone wants to take this over, please do.

[sbc100]

No problem. Hopefully someone can pick this up and get it landed.

[aminya]

@sbc100 this is almost ready. If you have write-access why don't you just finish it? Remember the more we wait, there will be more work to fix the conflicts

[sbc100]

I will get around to it at some point sure, but I've got a lot of other things I'm working so if anyone else want to pick this up in the mean time feel free.

[aminya]

Well, this is a critical bug on Windows, and we should fix it as soon as possible. I don't want emsdk to remove all the programs from my PATH!