From 4fe6e492c0a409e1070a5684e3de06d76baebd71 Mon Sep 17 00:00:00 2001 From: Chris Savoie Date: Fri, 9 Oct 2020 12:58:59 -0700 Subject: [PATCH] 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 | 7 +- emsdk.py | 180 +++++++++++++++++++++---------------- scripts/test_permanent.ps1 | 18 ++-- 3 files changed, 113 insertions(+), 92 deletions(-) diff --git a/.circleci/config.yml b/.circleci/config.yml index 4a2b452..6a5e16f 100644 --- a/.circleci/config.yml +++ b/.circleci/config.yml @@ -91,12 +91,7 @@ jobs: name: --permanent test shell: powershell.exe 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 - } + scripts/test_permanent.ps1 build-docker-image: executor: bionic diff --git a/emsdk.py b/emsdk.py index ee5452d..5032244 100644 --- a/emsdk.py +++ b/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 None - - 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') + return False + finally: + if folder is not None: + folder.Close() -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 + return False + return True try: - if system: - win_set_environment_variable_direct(key, value, system) - return + if win_set_environment_variable_direct(key, value, system): + return True # 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 WINDOWS and permanently_activate: - # 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) + if permanently_activate: + win_set_environment_variables(env_vars_to_add, 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): - env_string = '' +def get_env_vars_to_add(tools_to_activate): + 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_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 + env_vars_to_add += [('PATH', newpath)] 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()))] - - 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)] + env_vars_to_add += [('EMSDK', to_unix_path(emsdk_path()))] + env_vars_to_add += [('EM_CONFIG', os.path.normpath(dot_emscripten_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: - env_string += '$env:' + key + '="' + value + '"\n' - elif CMD: - env_string += 'SET ' + key + '=' + value + '\n' - elif CSH: - env_string += 'setenv ' + key + ' "' + value + '";\n' - elif BASH: - env_string += 'export ' + key + '="' + value + '";\n' - else: - assert False - if 'EMSDK_PYTHON' in env_vars: + + for key, value in env_vars_to_add: + # Don't set env vars which are already set to the correct value. + if key not in os.environ or to_unix_path(os.environ[key]) != to_unix_path(value): + errlog(key + ' = ' + value) + if POWERSHELL: + env_string += '$env:' + key + '="' + value + '"\n' + elif CMD: + env_string += 'SET ' + key + '=' + value + '\n' + elif CSH: + env_string += 'setenv ' + key + ' "' + value + '"\n' + 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 diff --git a/scripts/test_permanent.ps1 b/scripts/test_permanent.ps1 index 01ec1f1..4c9da19 100644 --- a/scripts/test_permanent.ps1 +++ b/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." }