Fix environment variables to be consistently set when using --system or --permanent (#515)

Also switch PostMessage to SendMessageWithTimeout and only send one if
environment variables actually changed.

Also, enable and fix --permanent unit tests.

.circleci/config.yml

@@ -91,12 +91,7 @@ jobs:
           name: --permanent test
           shell: powershell.exe
           command: |
-            try {
+            scripts/test_permanent.ps1
-              scripts/test_permanent.ps1
-            } catch {
-              Write-Host "`n Error Message: [$($_.Exception.Message)"] -ForegroundColor Red
-              Write-Host "`n Line Number: "$_.InvocationInfo.ScriptLineNumber -ForegroundColor Red
-            }
 
   build-docker-image:
     executor: bionic

emsdk.py

@@ -299,6 +299,7 @@ def remove_tree(d):
 
 
 def win_set_environment_variable_direct(key, value, system=True):
+  folder = None
   try:
     if system:
       # Read globally from ALL USERS section.
@@ -308,23 +309,22 @@ def win_set_environment_variable_direct(key, value, system=True):
       folder = winreg.OpenKeyEx(winreg.HKEY_CURRENT_USER, 'Environment', 0, winreg.KEY_ALL_ACCESS)
     winreg.SetValueEx(folder, key, 0, winreg.REG_EXPAND_SZ, value)
     debug_print('Set key=' + key + ' with value ' + value + ' in registry.')
+    return True
   except Exception as e:
     # 'Access is denied.'
-    if e.args[0] == 5:
+    if e.args[3] == 5:
       exit_with_error('Error! Failed to set the environment variable \'' + key + '\'! Setting environment variables permanently requires administrator access. Please rerun this command with administrative privileges. This can be done for example by holding down the Ctrl and Shift keys while opening a command prompt in start menu.')
     errlog('Failed to write environment variable ' + key + ':')
     errlog(str(e))
-    folder.Close()
+    return False
-    return None
+  finally:
-
+    if folder is not None:
-  folder.Close()
+      folder.Close()
-  HWND_BROADCAST = ctypes.wintypes.HWND(0xFFFF)  # win32con.HWND_BROADCAST == 65535
-  WM_SETTINGCHANGE = 0x001A  # win32con.WM_SETTINGCHANGE == 26
-  ctypes.windll.user32.SendMessageA(HWND_BROADCAST, WM_SETTINGCHANGE, 0, 'Environment')
 
 
-def win_get_environment_variable(key, system=True):
+def win_get_environment_variable(key, system=True, fallback=True):
   try:
+    folder = None
     try:
       if system:
         # Read globally from ALL USERS section.
@@ -334,22 +334,24 @@ def win_get_environment_variable(key, system=True):
         folder = winreg.OpenKey(winreg.HKEY_CURRENT_USER, 'Environment')
       value = str(winreg.QueryValueEx(folder, key)[0])
     except Exception:
-      # PyWin32 is not available - read via os.environ. This has the drawback
+      # If reading registry fails for some reason - read via os.environ. This has the drawback
       # that expansion items such as %PROGRAMFILES% will have been expanded, so
       # need to be precise not to set these back to system registry, or
       # expansion items would be lost.
-      return os.environ[key]
+      if fallback:
+        return os.environ[key]
+      return None
+    finally:
+      if folder is not None:
+        folder.Close()
+
   except Exception as e:
+    # this catch is if both the registry key threw an exception and the key is not in os.environ
     if e.args[0] != 2:
       # 'The system cannot find the file specified.'
       errlog('Failed to read environment variable ' + key + ':')
       errlog(str(e))
-    try:
-      folder.Close()
-    except Exception:
-      pass
     return None
-  folder.Close()
   return value
 
 
@@ -367,11 +369,11 @@ def win_get_active_environment_variable(key):
 
 def win_set_environment_variable(key, value, system=True):
   debug_print('set ' + str(key) + '=' + str(value) + ', in system=' + str(system), file=sys.stderr)
-  previous_value = win_get_environment_variable(key, system)
+  previous_value = win_get_environment_variable(key, system, fallback=False)
   if previous_value == value:
     debug_print('  no need to set, since same value already exists.')
     # No need to elevate UAC for nothing to set the same value, skip.
-    return
+    return False
 
   if not value:
     try:
@@ -382,13 +384,12 @@ def win_set_environment_variable(key, value, system=True):
       debug_print(str(cmd))
       value = subprocess.call(cmd, stdout=subprocess.PIPE)
     except Exception:
-      return
+      return False
-    return
+    return True
 
   try:
-    if system:
+    if win_set_environment_variable_direct(key, value, system):
-      win_set_environment_variable_direct(key, value, system)
+      return True
-      return
     # Escape % signs so that we don't expand references to environment variables.
     value = value.replace('%', '^%')
     if len(value) >= 1024:
@@ -398,15 +399,53 @@ def win_set_environment_variable(key, value, system=True):
     retcode = subprocess.call(cmd, stdout=subprocess.PIPE)
     if retcode != 0:
       errlog('ERROR! Failed to set environment variable ' + key + '=' + value + '. You may need to set it manually.')
+    else:
+      return True
   except Exception as e:
     errlog('ERROR! Failed to set environment variable ' + key + '=' + value + ':')
     errlog(str(e))
     errlog('You may need to set it manually.')
 
+  return False
+
+
+def win_set_environment_variables(env_vars_to_add, system):
+  if not env_vars_to_add:
+    return
+
+  changed = False
+
+  for key, value in env_vars_to_add:
+    if win_set_environment_variable(key, value, system):
+      if not changed:
+        changed = True
+        print('Setting global environment variables:')
+
+      print(key + ' = ' + value)
+
+  if not changed:
+    print('Global environment variables up to date')
+    return
+
+  # if changes were made then we need to notify other processes
+  try:
+    HWND_BROADCAST = ctypes.wintypes.HWND(0xFFFF)  # win32con.HWND_BROADCAST == 65535
+    WM_SETTINGCHANGE = 0x001A  # win32con.WM_SETTINGCHANGE == 26
+    SMTO_BLOCK = 0x0001  # win32con.SMTO_BLOCK == 1
+    ctypes.windll.user32.SendMessageTimeoutA(
+      HWND_BROADCAST,    # hWnd: notify everyone
+      WM_SETTINGCHANGE,  # Msg: registry changed
+      0,                 # wParam: Must be 0 when setting changed is sent by users
+      'Environment',     # lParam: Specifically environment variables changed
+      SMTO_BLOCK,        # fuFlags: Wait for message to be sent or timeout
+      100)               # uTimeout: 100ms
+  except Exception as e:
+    errlog('SendMessageTimeout failed with error: ' + str(e))
+
 
 def win_delete_environment_variable(key, system=True):
   debug_print('win_delete_environment_variable(key=' + key + ', system=' + str(system) + ')')
-  win_set_environment_variable(key, None, system)
+  return win_set_environment_variable(key, None, system)
 
 
 # Returns the absolute pathname to the given path inside the Emscripten SDK.
@@ -1770,14 +1809,6 @@ class Tool(object):
           return False
     return True
 
-  def win_activate_env_vars(self, system):
-    if WINDOWS:
-      envs = self.activated_environment()
-      for env in envs:
-        key, value = parse_key_value(env)
-
-        win_set_environment_variable(key, value, system)
-
   # If this tool can be installed on this system, this function returns True.
   # Otherwise, this function returns a string that describes the reason why this
   # tool is not available.
@@ -2416,20 +2447,14 @@ def set_active_tools(tools_to_activate, permanently_activate, system):
   # calling shell environment.  On other platform `source emsdk_env.sh` is
   # required.
   if WINDOWS:
-    env_string = construct_env(tools_to_activate)
+    # always set local environment variables since permanently activating will only set the registry settings and
+    # will not affect the current session
+    env_vars_to_add = get_env_vars_to_add(tools_to_activate)
+    env_string = construct_env_with_vars(env_vars_to_add)
     write_set_env_script(env_string)
 
-  # Apply environment variables to global all users section.
+    if permanently_activate:
-  if WINDOWS and permanently_activate:
+      win_set_environment_variables(env_vars_to_add, system)
-    # Individual env. vars
-    for tool in tools_to_activate:
-      tool.win_activate_env_vars(system=system)
-
-    # PATH variable
-    newpath, added_items = adjusted_path(tools_to_activate, system=system)
-    # Are there any actual changes?
-    if newpath != os.environ['PATH']:
-      win_set_environment_variable('PATH', newpath, system=system)
 
   return tools_to_activate
 
@@ -2504,22 +2529,14 @@ def adjusted_path(tools_to_activate, system=False):
   return (separator.join(whole_path), new_emsdk_tools)
 
 
-def construct_env(tools_to_activate):
+def get_env_vars_to_add(tools_to_activate):
-  env_string = ''
+  env_vars_to_add = []
+
   newpath, added_path = adjusted_path(tools_to_activate)
 
   # Don't bother setting the path if there are no changes.
   if os.environ['PATH'] != newpath:
-    if POWERSHELL:
+    env_vars_to_add += [('PATH', newpath)]
-      env_string += '$env:PATH="' + newpath + '"\n'
-    elif CMD:
-      env_string += 'SET PATH=' + newpath + '\n'
-    elif CSH:
-      env_string += 'setenv PATH "' + newpath + '";\n'
-    elif BASH:
-      env_string += 'export PATH="' + newpath + '";\n'
-    else:
-      assert False
 
     if added_path:
       errlog('Adding directories to PATH:')
@@ -2528,11 +2545,8 @@ def construct_env(tools_to_activate):
       errlog('')
 
   # A core variable EMSDK points to the root of Emscripten SDK directory.
-  env_vars = [('EMSDK', to_unix_path(emsdk_path()))]
+  env_vars_to_add += [('EMSDK', to_unix_path(emsdk_path()))]
-
+  env_vars_to_add += [('EM_CONFIG', os.path.normpath(dot_emscripten_path()))]
-  em_config_path = os.path.normpath(dot_emscripten_path())
-  if to_unix_path(os.environ.get('EM_CONFIG', '')) != to_unix_path(em_config_path):
-    env_vars += [('EM_CONFIG', em_config_path)]
 
   for tool in tools_to_activate:
     config = tool.activated_config()
@@ -2540,27 +2554,40 @@ def construct_env(tools_to_activate):
       # For older emscripten versions that don't use this default we export
       # EM_CACHE.
       em_cache_dir = os.path.join(config['EMSCRIPTEN_ROOT'], 'cache')
-      env_vars += [('EM_CACHE', em_cache_dir)]
+      env_vars_to_add += [('EM_CACHE', em_cache_dir)]
     envs = tool.activated_environment()
     for env in envs:
       key, value = parse_key_value(env)
       value = to_native_path(tool.expand_vars(value))
-      env_vars += [(key, value)]
+      env_vars_to_add += [(key, value)]
 
-  if env_vars:
+  return env_vars_to_add
+
+
+def construct_env(tools_to_activate):
+  return construct_env_with_vars(get_env_vars_to_add(tools_to_activate))
+
+
+def construct_env_with_vars(env_vars_to_add):
+  env_string = ''
+  if env_vars_to_add:
     errlog('Setting environment variables:')
-    for key, value in env_vars:
+
-      if POWERSHELL:
+    for key, value in env_vars_to_add:
-        env_string += '$env:' + key + '="' + value + '"\n'
+      # Don't set env vars which are already set to the correct value.
-      elif CMD:
+      if key not in os.environ or to_unix_path(os.environ[key]) != to_unix_path(value):
-        env_string += 'SET ' + key + '=' + value + '\n'
+        errlog(key + ' = ' + value)
-      elif CSH:
+        if POWERSHELL:
-        env_string += 'setenv ' + key + ' "' + value + '";\n'
+          env_string += '$env:' + key + '="' + value + '"\n'
-      elif BASH:
+        elif CMD:
-        env_string += 'export ' + key + '="' + value + '";\n'
+          env_string += 'SET ' + key + '=' + value + '\n'
-      else:
+        elif CSH:
-        assert False
+          env_string += 'setenv ' + key + ' "' + value + '"\n'
-      if 'EMSDK_PYTHON' in env_vars:
+        elif BASH:
+          env_string += 'export ' + key + '="' + value + '"\n'
+        else:
+          assert False
+      if 'EMSDK_PYTHON' in env_vars_to_add:
         # When using our bundled python we never want the user's
         # PYTHONHOME or PYTHONPATH
         # See https://github.com/emscripten-core/emsdk/issues/598
@@ -2579,7 +2606,6 @@ def construct_env(tools_to_activate):
         else:
           assert False
 
-      errlog(key + ' = ' + value)
   return env_string
 
 

scripts/test_permanent.ps1

@@ -33,24 +33,24 @@ if (!$EM_CACHE_USER) {
 
 $path_split = $PATH_USER.Split(';')
 
-$EMSDK_Path_USER = $path_split | Where-Object { $_ -match 'emsdk' }
+$EMSDK_Path_USER = $path_split | Where-Object { $_ -like "$repo_root*" }
 if (!$EMSDK_Path_USER) {
     throw "No path is added!"
 }
-$EMSDK_NODE_Path_USER = $path_split | Where-Object { $_ -match 'emsdk\\node' }
+$EMSDK_NODE_Path_USER = $path_split | Where-Object { $_ -like "$repo_root\node*" }
 if (!$EMSDK_NODE_Path_USER) {
-    throw "emsdk\node is not added to path."
+    throw "$repo_root\\node is not added to path."
 }
-$EMSDK_PYTHON_Path_USER = $path_split | Where-Object { $_ -match 'emsdk\\python' }
+$EMSDK_PYTHON_Path_USER = $path_split | Where-Object { $_ -like "$repo_root\python*" }
 if (!$EMSDK_PYTHON_Path_USER) {
-    throw "emsdk\python is not added to path."
+    throw "$repo_root\\python is not added to path."
 }
-$EMSDK_JAVA_Path_USER = $path_split | Where-Object { $_ -match 'emsdk\\java' }
+$EMSDK_JAVA_Path_USER = $path_split | Where-Object { $_ -like "$repo_root\java*" }
 if (!$EMSDK_JAVA_Path_USER) {
-    throw "emsdk\java is not added to path."
+    throw "$repo_root\\java is not added to path."
 }
 
-$EMSDK_UPSTREAM_Path_USER = $path_split | Where-Object { $_ -match 'emsdk\\upstream\\emscripten' }
+$EMSDK_UPSTREAM_Path_USER = $path_split | Where-Object { $_ -like "$repo_root\upstream\emscripten*" }
 if (!$EMSDK_UPSTREAM_Path_USER) {
-    throw "emsdk\upstream\emscripten is not added to path."
+    throw "$repo_root\\upstream\emscripten is not added to path."
 }