Fix Path,EMSDK and EM_CACHE not being set permanently when using --global by kudaba · Pull Request #515 · emscripten-core/emsdk

kudaba · 2020-06-04T04:54:52Z

Going through the initial setup of emscripten the first time, the first thing I wanted to do was set it up globally to use in visual studio and found that some of the environment variables are not getting applied properly. There are still additional issues with environment variables however since EMSDK_NODE and EMSDK_PYTHON and PATH will get installed system wide while EM_CACHE and EMSDK will only get installed to user.

FYI: I also ran into some reported issues during my initial setup that didn't block me in the end:

pywin32 was missing, so I needed to pip install it, maybe add a not in the windows setup documentation.
ERROR: The system was unable to find the specified registry key or value. which are not actual issues but should probably be suppressed (trying to delete registry key that doesn't exist). Happens on every activate call.
(1159, 'PostMessage', 'The message can be used only with synchronous operations.') Only happens when system registry keys are not set, it doesn't cause them to fail though.

Edit:
So, a big update here. While the code remains to be able to set user environment variables in windows it really only runs in two modes: current session locals or system wide environment variables. I removed the code that was trying to delete the variables and causing errors since it doesn't really do anything.

I removed setx and the option to permanently activate from construct_env, but it's still used as a fallback to set registry variables where it already had the 1024 check in place.

I fixed the PostMessage error that was happening by using SendMessageWithTimeout instead. It only sends the message if a registry value actually changed. I also confirmed that the message is being received by other applications.

The one caveat that still exists is that you need to run emsdk activate before running emsdk activate --global so that it will use the correct python version.

sbc100 · 2020-06-04T16:02:57Z

Regarding pywin32. This should now be included in the version of python that is downloaded by emsdk. At least as of #487.. can you confirm?

kudaba · 2020-06-04T17:05:21Z

Python is mostly working as designed. The confusion happened because I went through the setup in a non-administrator console window, then when --global failed I switched to administrator to rerun only emsdk activate --global but since it was a new console window the environment variables were gone and it was trying to use my local python instead of the built-in one. I didn't realize (since it was my first time) that you should run emsdk_env before doing anything.

sbc100 · 2020-06-14T18:02:46Z

Seems like a good change to me. @juj?

sbc100 · 2020-06-17T19:25:20Z

This change is a lot bigger now :( Can it be simplified or broken down into smaller chunks? I'm having trouble groking all the moving parts there.

sbc100 · 2020-06-17T19:36:59Z

If there are part of this change that are cleanups or re-structuring only perhaps you could submit those separately?

Also, perhaps the PR description could be update to reflect exactly what this change is supposed to do? Or is it up-to-date already?

kudaba · 2020-06-17T19:56:09Z

Updated the PR with the info of all the changes. I could go through an restructure it, unfortunately I won't have the time to get back to this for a while.

sbc100 · 2020-06-23T00:22:09Z

How does this relate to #468 ?

kudaba · 2020-06-23T01:49:10Z

There will be merge conflicts but they should be able to co-exists.

aminya · 2020-09-30T05:39:43Z

Can we get this rebased? This will probably fix the windows tests.

FYI, --global is renamed to --system. Now, we have a separate --permanent flag.

sbc100 · 2020-09-30T17:10:45Z

          assert False

-      errlog(key + ' = ' + value)
+    errlog('')


Is this added an extra empty line to the output?

yeah, just to make formatting a bit nicer

But why do you say this is nicer? Right no I see the following output on my machine:

sbc.sbc-glaptop (~/dev/wasm/emsdk) ((53c184e...)) $ . ./emsdk_env.sh Adding directories to PATH: PATH += /home/sbc/dev/wasm/emsdk PATH += /home/sbc/dev/wasm/emsdk/upstream/emscripten PATH += /home/sbc/dev/wasm/emsdk/node/12.18.1_64bit/bin Setting environment variables: EMSDK = /home/sbc/dev/wasm/emsdk EM_CONFIG = /home/sbc/dev/wasm/emsdk/.emscripten EM_CACHE = /home/sbc/dev/wasm/emsdk/upstream/emscripten/cache EMSDK_NODE = /home/sbc/dev/wasm/emsdk/node/12.18.1_64bit/bin/node sbc.sbc-glaptop (~/dev/wasm/emsdk) ((53c184e...)) $

To me an extra empty line at the end would look pointless/messy.. but maybe that is just me?

aminya

I tested this. It works!

I recommend enabling the tests:
https://github.com/kudaba/emsdk/blob/ca1e15fb6a66c4d62e2a72a27cbff371b4e2a09f/.circleci/config.yml#L95

          command: |
-            try {
               scripts/test_permanent.ps1
-            } catch {
-             Write-Host "`n Error Message: [$($_.Exception.Message)"] -ForegroundColor Red
-              Write-Host "`n Line Number: "$_.InvocationInfo.ScriptLineNumber -ForegroundColor Red
-            }

aminya · 2020-10-07T06:02:03Z

I am happy to say that I tested this in combination with #635, and all the scripts/test_permanent.ps1 tests finally pass!

We should definitely enable them as I showed in #515 (review)

After #635 is merged there is a small conflict that needs to be fixed. I removed the following lines in that PR, so after accepting your changes here, just remove these lines:

prev_path = os.environ['PATH']

…or --permanent Switch PostMessage with SendMessageWithTimeout and only send one if environment variables actually changed Enable and fix permanent unit tests

kudaba · 2020-10-07T22:31:28Z

Sorry for all the force pushes, it took me a while to get local testing working.

I fixed up the exception handling with the registry and had to fix up the permanent test script a bit as well since it assumed emsdk was being installed to a folder called emsdk.

I did notice that the registry handling is a lot slower now, but since it's generally a one time thing it's probably not anything worth worrying about.

aminya · 2020-10-07T22:36:44Z

Awesome!

I did not notice any performance difference. Anyways, I think stability is more important. 😃

aminya · 2020-10-08T19:38:56Z

@sbc100 Can we roll this in?

sbc100

I think we still have the problem that we will clobber any local changes that the user has in their PATH with a fresh copy from the registry... but that is separate issue I think.

sbc100 · 2020-10-09T01:56:32Z

+      'Environment',     # lParam: Specifically environment variables changed
+      SMTO_BLOCK,        # fuFlags: Wait for message to be sent or timeout
+      100)               # uTimeout: 100ms
+  except Exception as e:


Can we be more specific?

@kudaba has added comments for these. The original code did not have any description
https://github.com/emscripten-core/emsdk/pull/515/files#diff-7ecf1f48f2952ff372972476842c8882L323

I mean more specific in the type of exceptions we catch here? Can it be just OSError perhaps?

I don't think it hurts to catch all the exceptions. We are not concerned with the performance of this program which should run on a powerful desktop processor. The stability is more important, IMO.

I'm not sure how error handling or exceptions are working with the interop between python and win32 so this is just me being a bit extra cautious.

Catching specific exceptions is not about performance but about code quality and maintainability. In general we want to minimize the size of the try block and be as specific as possible in the exceptions we catch.

From pep8: When catching exceptions, mention specific exceptions whenever possible instead of using a bare except: clause:

One problem with catching everything is that it can hide all kinds of programmer errors. e.g. simple things like accessing out of bounds which the except block is almost never supposed to handle.

Anyway, unrelated to this PR so we don't need to block on this.

sbc100 · 2020-10-09T02:01:28Z

          assert False

-      errlog(key + ' = ' + value)
+    errlog('')


But why do you say this is nicer? Right no I see the following output on my machine:

sbc.sbc-glaptop (~/dev/wasm/emsdk) ((53c184e...)) $ . ./emsdk_env.sh Adding directories to PATH: PATH += /home/sbc/dev/wasm/emsdk PATH += /home/sbc/dev/wasm/emsdk/upstream/emscripten PATH += /home/sbc/dev/wasm/emsdk/node/12.18.1_64bit/bin Setting environment variables: EMSDK = /home/sbc/dev/wasm/emsdk EM_CONFIG = /home/sbc/dev/wasm/emsdk/.emscripten EM_CACHE = /home/sbc/dev/wasm/emsdk/upstream/emscripten/cache EMSDK_NODE = /home/sbc/dev/wasm/emsdk/node/12.18.1_64bit/bin/node sbc.sbc-glaptop (~/dev/wasm/emsdk) ((53c184e...)) $

To me an extra empty line at the end would look pointless/messy.. but maybe that is just me?

* Fixed up comment * Removed redundant newline * Removed unneeded try/catch block in registry get variable

kudaba · 2020-10-09T17:14:02Z

I think we still have the problem that we will clobber any local changes that the user has in their PATH with a fresh copy from the registry... but that is separate issue I think.

Actually I think this will be fine. It adds emsdk paths to the local path environment variable and the registry setting separately so it should be additive in both cases. I had to do that since just setting the registry setting was not actually changing the currently set environment variable.

sbc100 · 2020-10-09T19:15:46Z

I think we still have the problem that we will clobber any local changes that the user has in their PATH with a fresh copy from the registry... but that is separate issue I think.

Actually I think this will be fine. It adds emsdk paths to the local path environment variable and the registry setting separately so it should be additive in both cases. I had to do that since just setting the registry setting was not actually changing the currently set environment variable.

The problem is not about addings to the current env. I believe the problem is that we always take the registry value as the initial PATH to add to (we've add the concept of fallback here but I think in practice it won't fire). This means that you always end up with "registry value + extras" rather than "current shell value + extras".

Basically we need to calculate two different paths which can be different:

What path should be added the registery.
What path should be set in the current shell

Again, its not something we need to fix as part of this PR, I guess its bug we have always had.

sbc100 · 2020-10-09T19:24:13Z

I think this change is ready to land now.

Can you craft as suitable message for the squashed commit? I would use the PR description but I'm not sure which parts are still relevant?

kudaba · 2020-10-09T19:49:08Z

The first commit has the most accurate messaging I think:

Fix environment variables to be consistently set when using --system or --permanent
Switch PostMessage with SendMessageWithTimeout and only send one if environment variables actually changed
Enable and fix permanent unit tests

Or did you mean you want me to squash the review feeback commit back down into the original (I had been doing it this whole time until the last bit of feedback).

sbc100 · 2020-10-09T19:59:08Z

Thanks!

… 20230922.1 (emscripten-core#515) runtime.linux-arm64.Microsoft.NETCore.Runtime.JIT.Tools , runtime.linux-arm64.Microsoft.NETCore.Runtime.Mono.LLVM.Sdk , runtime.linux-arm64.Microsoft.NETCore.Runtime.Mono.LLVM.Tools , runtime.linux-arm64.Microsoft.NETCore.Runtime.ObjWriter , runtime.linux-arm64.Microsoft.NETCore.Runtime.Wasm.LLVM.Transport , runtime.linux-musl-arm64.Microsoft.NETCore.Runtime.JIT.Tools , runtime.linux-musl-arm64.Microsoft.NETCore.Runtime.Mono.LLVM.Sdk , runtime.linux-musl-arm64.Microsoft.NETCore.Runtime.Mono.LLVM.Tools , runtime.linux-musl-arm64.Microsoft.NETCore.Runtime.ObjWriter , runtime.linux-musl-arm64.Microsoft.NETCore.Runtime.Wasm.LLVM.Transport , runtime.linux-musl-x64.Microsoft.NETCore.Runtime.JIT.Tools , runtime.linux-musl-x64.Microsoft.NETCore.Runtime.Mono.LLVM.Sdk , runtime.linux-musl-x64.Microsoft.NETCore.Runtime.Mono.LLVM.Tools , runtime.linux-musl-x64.Microsoft.NETCore.Runtime.ObjWriter , runtime.linux-musl-x64.Microsoft.NETCore.Runtime.Wasm.LLVM.Transport , runtime.linux-x64.Microsoft.NETCore.Runtime.JIT.Tools , runtime.linux-x64.Microsoft.NETCore.Runtime.Mono.LLVM.Sdk , runtime.linux-x64.Microsoft.NETCore.Runtime.Mono.LLVM.Tools , runtime.linux-x64.Microsoft.NETCore.Runtime.ObjWriter , runtime.linux-x64.Microsoft.NETCore.Runtime.Wasm.LLVM.Transport , runtime.osx-arm64.Microsoft.NETCore.Runtime.JIT.Tools , runtime.osx-arm64.Microsoft.NETCore.Runtime.Mono.LLVM.Sdk , runtime.osx-arm64.Microsoft.NETCore.Runtime.Mono.LLVM.Tools , runtime.osx-arm64.Microsoft.NETCore.Runtime.ObjWriter , runtime.osx-arm64.Microsoft.NETCore.Runtime.Wasm.LLVM.Transport , runtime.osx-x64.Microsoft.NETCore.Runtime.JIT.Tools , runtime.osx-x64.Microsoft.NETCore.Runtime.Mono.LLVM.Sdk , runtime.osx-x64.Microsoft.NETCore.Runtime.Mono.LLVM.Tools , runtime.osx-x64.Microsoft.NETCore.Runtime.ObjWriter , runtime.osx-x64.Microsoft.NETCore.Runtime.Wasm.LLVM.Transport , runtime.win-arm64.Microsoft.NETCore.Runtime.JIT.Tools , runtime.win-arm64.Microsoft.NETCore.Runtime.ObjWriter , runtime.win-arm64.Microsoft.NETCore.Runtime.Wasm.LLVM.Transport , runtime.win-x64.Microsoft.NETCore.Runtime.JIT.Tools , runtime.win-x64.Microsoft.NETCore.Runtime.Mono.LLVM.Sdk , runtime.win-x64.Microsoft.NETCore.Runtime.Mono.LLVM.Tools , runtime.win-x64.Microsoft.NETCore.Runtime.ObjWriter , runtime.win-x64.Microsoft.NETCore.Runtime.Wasm.LLVM.Transport From Version 16.0.5-alpha.1.23452.1 -> To Version 16.0.5-alpha.1.23472.1 Co-authored-by: dotnet-maestro[bot] <dotnet-maestro[bot]@users.noreply.github.com>

juj reviewed Jun 15, 2020

View reviewed changes

Comment thread emsdk.py Outdated

kudaba force-pushed the fix_win_env_global branch from 307c527 to 8d4f4d0 Compare June 17, 2020 19:19

sbc100 reviewed Jun 17, 2020

View reviewed changes

Comment thread emsdk.py Outdated

sbc100 mentioned this pull request Jun 17, 2020

Remove unused permanent in construct_env #525

Merged

kudaba force-pushed the fix_win_env_global branch 4 times, most recently from 997806d to e8b9a36 Compare September 30, 2020 17:14

sbc100 reviewed Sep 30, 2020

View reviewed changes

kudaba force-pushed the fix_win_env_global branch from e8b9a36 to ca1e15f Compare September 30, 2020 18:46

aminya mentioned this pull request Oct 6, 2020

On Windows emsdk_env.bat removes most of the PATH environment variable #634

Closed

aminya approved these changes Oct 7, 2020

View reviewed changes

aminya mentioned this pull request Oct 7, 2020

No win32api #635

Merged

kudaba force-pushed the fix_win_env_global branch 7 times, most recently from fabf720 to 082581c Compare October 7, 2020 21:07

Fix environment variables to be consistently set when using --system …

08a4405

…or --permanent Switch PostMessage with SendMessageWithTimeout and only send one if environment variables actually changed Enable and fix permanent unit tests

kudaba force-pushed the fix_win_env_global branch from 082581c to 08a4405 Compare October 7, 2020 22:16

sbc100 reviewed Oct 9, 2020

View reviewed changes

Comment thread emsdk.py

sbc100 approved these changes Oct 9, 2020

View reviewed changes

Review feedback

8508d8e

* Fixed up comment * Removed redundant newline * Removed unneeded try/catch block in registry get variable

sbc100 merged commit 4fe6e49 into emscripten-core:master Oct 9, 2020

Conversation

kudaba commented Jun 4, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

sbc100 commented Jun 4, 2020

Uh oh!

kudaba commented Jun 4, 2020

Uh oh!

sbc100 commented Jun 14, 2020

Uh oh!

Uh oh!

sbc100 commented Jun 17, 2020

Uh oh!

Uh oh!

sbc100 commented Jun 17, 2020

Uh oh!

kudaba commented Jun 17, 2020

Uh oh!

sbc100 commented Jun 23, 2020

Uh oh!

kudaba commented Jun 23, 2020

Uh oh!

aminya commented Sep 30, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

aminya left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

aminya commented Oct 7, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

kudaba commented Oct 7, 2020

Uh oh!

aminya commented Oct 7, 2020

Uh oh!

aminya commented Oct 8, 2020

Uh oh!

sbc100 left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

aminya Oct 9, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

aminya Oct 9, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

kudaba commented Oct 9, 2020

Uh oh!

sbc100 commented Oct 9, 2020

Uh oh!

sbc100 commented Oct 9, 2020

Uh oh!

kudaba commented Jun 4, 2020 •

edited

Loading

aminya commented Sep 30, 2020 •

edited

Loading

aminya left a comment •

edited

Loading

aminya commented Oct 7, 2020 •

edited

Loading

aminya Oct 9, 2020 •

edited

Loading

aminya Oct 9, 2020 •

edited

Loading