From 40780130aa995872b777468c9ed60afe45af6601 Mon Sep 17 00:00:00 2001 From: Amin Yahyaabadi Date: Fri, 30 Oct 2020 22:57:15 -0500 Subject: [PATCH] fix: emsdk swallows/overwrites parts of PATH (#642) This PR has three changes: 1) Fixing --permanent flag: The previous algorithm looped over the values of PATH 4 times to check conditions while these conditions are the opposite of each other. Also, the result did not include all the parts in the whole_path. I have combined these loops and fixed the algorithm, so no path is lost. Now using --permanent the PATH and Env variables are correctly updated. 2) Fixing --system flag: Now, the system flag does not cause the PATH values to be overwritten/clobbered. The PATH/Env variables are correctly updated. 3) Fixing flagless flag: When no flag is provided the value of process/shell is correctly updated and preserved. This adds the tests for all the situations. Fixes #634 Fixes #645 --- .circleci/config.yml | 37 ++++++- emsdk.py | 70 ++++++------- scripts/test_activation.ps1 | 122 +++++++++++++++++++++++ scripts/test_path_preservation.ps1 | 151 +++++++++++++++++++++++++++++ scripts/test_permanent.ps1 | 56 ----------- 5 files changed, 345 insertions(+), 91 deletions(-) create mode 100644 scripts/test_activation.ps1 create mode 100644 scripts/test_path_preservation.ps1 delete mode 100644 scripts/test_permanent.ps1 diff --git a/.circleci/config.yml b/.circleci/config.yml index 7994d6d..d5ff7d6 100644 --- a/.circleci/config.yml +++ b/.circleci/config.yml @@ -87,11 +87,46 @@ jobs: command: | source emsdk_env.sh python scripts/test.py + + - run: + name: flagless (process/shell) test + shell: powershell.exe + command: | + scripts/test_activation.ps1 + - run: name: --permanent test shell: powershell.exe command: | - scripts/test_permanent.ps1 + $env:PERMANENT_FLAG="--permanent" + scripts/test_activation.ps1 + + - run: + name: --system test + shell: powershell.exe + command: | + $env:SYSTEM_FLAG="--system" + scripts/test_activation.ps1 + + - run: + name: Process/Shell PATH preservation test + shell: powershell.exe + command: | + scripts/test_path_preservation.ps1 + + - run: + name: User PATH preservation test + shell: powershell.exe + command: | + $env:PERMANENT_FLAG="--permanent" + scripts/test_path_preservation.ps1 + + - run: + name: System PATH preservation test + shell: powershell.exe + command: | + $env:SYSTEM_FLAG="--system" + scripts/test_path_preservation.ps1 build-docker-image: executor: bionic diff --git a/emsdk.py b/emsdk.py index 5032244..1e33b94 100644 --- a/emsdk.py +++ b/emsdk.py @@ -322,7 +322,10 @@ def win_set_environment_variable_direct(key, value, system=True): folder.Close() -def win_get_environment_variable(key, system=True, fallback=True): +def win_get_environment_variable(key, system=True, user=True, fallback=True): + if (not system and not user and fallback): + # if no --system or --permanent flag is provided use shell's value + return os.environ[key] try: folder = None try: @@ -355,21 +358,9 @@ def win_get_environment_variable(key, system=True, fallback=True): return value -def win_environment_variable_exists(key, system=True): - value = win_get_environment_variable(key, system) - return value is not None and len(value) > 0 - - -def win_get_active_environment_variable(key): - value = win_get_environment_variable(key, False) - if value is not None: - return value - return win_get_environment_variable(key, True) - - -def win_set_environment_variable(key, value, system=True): +def win_set_environment_variable(key, value, system, user): debug_print('set ' + str(key) + '=' + str(value) + ', in system=' + str(system), file=sys.stderr) - previous_value = win_get_environment_variable(key, system, fallback=False) + previous_value = win_get_environment_variable(key, system=system, user=user) 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. @@ -409,14 +400,14 @@ def win_set_environment_variable(key, value, system=True): return False -def win_set_environment_variables(env_vars_to_add, system): +def win_set_environment_variables(env_vars_to_add, system, user): 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 win_set_environment_variable(key, value, system, user): if not changed: changed = True print('Setting global environment variables:') @@ -443,9 +434,9 @@ def win_set_environment_variables(env_vars_to_add, system): errlog('SendMessageTimeout failed with error: ' + str(e)) -def win_delete_environment_variable(key, system=True): +def win_delete_environment_variable(key, system=True, user=True): debug_print('win_delete_environment_variable(key=' + key + ', system=' + str(system) + ')') - return win_set_environment_variable(key, None, system) + return win_set_environment_variable(key, None, system, user) # Returns the absolute pathname to the given path inside the Emscripten SDK. @@ -2449,12 +2440,12 @@ def set_active_tools(tools_to_activate, permanently_activate, system): if WINDOWS: # 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_vars_to_add = get_env_vars_to_add(tools_to_activate, system, user=permanently_activate) env_string = construct_env_with_vars(env_vars_to_add) write_set_env_script(env_string) if permanently_activate: - win_set_environment_variables(env_vars_to_add, system) + win_set_environment_variables(env_vars_to_add, system, user=permanently_activate) return tools_to_activate @@ -2500,23 +2491,34 @@ def to_msys_path(p): # Looks at the current PATH and adds and removes entries so that the PATH reflects # the set of given active tools. -def adjusted_path(tools_to_activate, system=False): +def adjusted_path(tools_to_activate, system=False, user=False): # These directories should be added to PATH path_add = get_required_path(tools_to_activate) # These already exist. if WINDOWS and not MSYS: - existing_path = win_get_environment_variable('PATH', system=system).split(ENVPATH_SEPARATOR) + existing_path = win_get_environment_variable('PATH', system=system, user=user, fallback=True).split(ENVPATH_SEPARATOR) else: existing_path = os.environ['PATH'].split(ENVPATH_SEPARATOR) emsdk_root_path = to_unix_path(emsdk_path()) - existing_emsdk_tools = [i for i in existing_path if to_unix_path(i).startswith(emsdk_root_path)] - new_emsdk_tools = [i for i in path_add if not normalized_contains(existing_emsdk_tools, i)] + existing_emsdk_tools = [] + existing_nonemsdk_path = [] + for entry in existing_path: + if to_unix_path(entry).startswith(emsdk_root_path): + existing_emsdk_tools.append(entry) + else: + existing_nonemsdk_path.append(entry) + + new_emsdk_tools = [] + kept_emsdk_tools = [] + for entry in path_add: + if not normalized_contains(existing_emsdk_tools, entry): + new_emsdk_tools.append(entry) + else: + kept_emsdk_tools.append(entry) + + whole_path = unique_items(new_emsdk_tools + kept_emsdk_tools + existing_nonemsdk_path) - # Existing non-emsdk tools - existing_path = [i for i in existing_path if not to_unix_path(i).startswith(emsdk_root_path)] - new_path = [i for i in path_add if not normalized_contains(existing_path, i)] - whole_path = unique_items(new_path + existing_path) if MSYS: # XXX Hack: If running native Windows Python in MSYS prompt where PATH # entries look like "/c/Windows/System32", os.environ['PATH'] @@ -2529,10 +2531,10 @@ def adjusted_path(tools_to_activate, system=False): return (separator.join(whole_path), new_emsdk_tools) -def get_env_vars_to_add(tools_to_activate): +def get_env_vars_to_add(tools_to_activate, system, user): env_vars_to_add = [] - newpath, added_path = adjusted_path(tools_to_activate) + newpath, added_path = adjusted_path(tools_to_activate, system, user) # Don't bother setting the path if there are no changes. if os.environ['PATH'] != newpath: @@ -2564,8 +2566,8 @@ def get_env_vars_to_add(tools_to_activate): 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(tools_to_activate, system, user): + return construct_env_with_vars(get_env_vars_to_add(tools_to_activate, system, user)) def construct_env_with_vars(env_vars_to_add): @@ -2978,7 +2980,7 @@ def main(): # to write out the new one. tools_to_activate = currently_active_tools() tools_to_activate = process_tool_list(tools_to_activate, log_errors=True) - env_string = construct_env(tools_to_activate) + env_string = construct_env(tools_to_activate, arg_system, arg_permanent) if WINDOWS and not BASH: write_set_env_script(env_string) else: diff --git a/scripts/test_activation.ps1 b/scripts/test_activation.ps1 new file mode 100644 index 0000000..98ebada --- /dev/null +++ b/scripts/test_activation.ps1 @@ -0,0 +1,122 @@ +# This test installs emsdk and activates the latest toolchain using `--system` or `--permanent` flags, +# and checks if the environment variables and PATH are correctly updated. Set $env:SYSTEM_FLAG and $env:PERMANENT_FLAG to test each. +# If no flag is provided the process/shell values are tested. See the CI file for an example. + +refreshenv + +$repo_root = [System.IO.Path]::GetDirectoryName((resolve-path "$PSScriptRoot")) + +$PATH_USER_BEFORE = [System.Environment]::GetEnvironmentVariable("PATH", "User") +$PATH_MACHINE_BEFORE = [System.Environment]::GetEnvironmentVariable("PATH", "Machine") +$PATH_Process_BEFORE = [System.Environment]::GetEnvironmentVariable("PATH", "Process") + + +try { + + & "$repo_root/emsdk.ps1" install latest + + $esc = '--%' + & "$repo_root/emsdk.ps1" activate latest $esc $env:PERMANENT_FLAG $env:SYSTEM_FLAG + + if ($env:SYSTEM_FLAG) { + $env_type = "Machine" + } + elseif ($env:PERMANENT_FLAG) { + $env_type = "User" + } else { + $env_type = "Process" + } + + $EMSDK = [System.Environment]::GetEnvironmentVariable("EMSDK", $env_type) + $EM_CONFIG = [System.Environment]::GetEnvironmentVariable("EM_CONFIG", $env_type) + $EMSDK_NODE = [System.Environment]::GetEnvironmentVariable("EMSDK_NODE", $env_type) + $EMSDK_PYTHON = [System.Environment]::GetEnvironmentVariable("EMSDK_PYTHON", $env_type) + $JAVA_HOME = [System.Environment]::GetEnvironmentVariable("JAVA_HOME", $env_type) + $EM_CACHE = [System.Environment]::GetEnvironmentVariable("EM_CACHE", $env_type) + $PATH = [System.Environment]::GetEnvironmentVariable("PATH", $env_type) + + if (!$EMSDK) { + throw "EMSDK is not set for the user" + } + if (!$EM_CONFIG) { + throw "EM_CONFIG is not set for the user" + } + if (!$EMSDK_NODE) { + throw "EMSDK_NODE is not set for the user" + } + if (!$JAVA_HOME) { + throw "JAVA_HOME is not set for the user" + } + if (!$EMSDK_PYTHON) { + throw "EMSDK_PYTHON is not set for the user" + } + if (!$EM_CACHE) { + throw "EM_CACHE is not set for the user" + } + + + $path_split = $PATH.Split(';') + + $EMSDK_Path = $path_split | Where-Object { $_ -like "$repo_root*" } + if (!$EMSDK_Path) { + throw "No path is added!" + } + $EMSDK_NODE_Path = $path_split | Where-Object { $_ -like "$repo_root\node*" } + if (!$EMSDK_NODE_Path) { + throw "$repo_root\\node is not added to path." + } + $EMSDK_PYTHON_Path = $path_split | Where-Object { $_ -like "$repo_root\python*" } + if (!$EMSDK_PYTHON_Path) { + throw "$repo_root\\python is not added to path." + } + $EMSDK_JAVA_Path = $path_split | Where-Object { $_ -like "$repo_root\java*" } + if (!$EMSDK_JAVA_Path) { + throw "$repo_root\\java is not added to path." + } + + $EMSDK_UPSTREAM_Path = $path_split | Where-Object { $_ -like "$repo_root\upstream\emscripten*" } + if (!$EMSDK_UPSTREAM_Path) { + throw "$repo_root\\upstream\emscripten is not added to path." + } + + +} +finally { + # Recover pre-split PATH + refreshenv + + [Environment]::SetEnvironmentVariable("Path", $PATH_USER_BEFORE, "User") + try { + [Environment]::SetEnvironmentVariable("Path", $PATH_MACHINE_BEFORE, "Machine") + } + catch {} + + [Environment]::SetEnvironmentVariable("Path", $PATH_Process_BEFORE, "Process") + + # Recover pre activation env variables + [Environment]::SetEnvironmentVariable("EMSDK", $null, "User") + [Environment]::SetEnvironmentVariable("EM_CONFIG", $null, "User") + [Environment]::SetEnvironmentVariable("EMSDK_NODE", $null, "User") + [Environment]::SetEnvironmentVariable("EMSDK_PYTHON", $null, "User") + [Environment]::SetEnvironmentVariable("JAVA_HOME", $null, "User") + [Environment]::SetEnvironmentVariable("EM_CACHE", $null, "User") + + try { + [Environment]::SetEnvironmentVariable("EMSDK", $null, "Machine") + [Environment]::SetEnvironmentVariable("EM_CONFIG", $null, "Machine") + [Environment]::SetEnvironmentVariable("EMSDK_NODE", $null, "Machine") + [Environment]::SetEnvironmentVariable("EMSDK_PYTHON", $null, "Machine") + [Environment]::SetEnvironmentVariable("JAVA_HOME", $null, "Machine") + [Environment]::SetEnvironmentVariable("EM_CACHE", $null, "Machine") + } catch {} + + + [Environment]::SetEnvironmentVariable("EMSDK", $null, "Process") + [Environment]::SetEnvironmentVariable("EM_CONFIG", $null, "Process") + [Environment]::SetEnvironmentVariable("EMSDK_NODE", $null, "Process") + [Environment]::SetEnvironmentVariable("EMSDK_PYTHON", $null, "Process") + [Environment]::SetEnvironmentVariable("JAVA_HOME", $null, "Process") + [Environment]::SetEnvironmentVariable("EM_CACHE", $null, "Process") + + refreshenv +} diff --git a/scripts/test_path_preservation.ps1 b/scripts/test_path_preservation.ps1 new file mode 100644 index 0000000..79a42d9 --- /dev/null +++ b/scripts/test_path_preservation.ps1 @@ -0,0 +1,151 @@ +# This test installs emsdk and activates the latest toolchain using `--system` or `--permanent` flags, +# and checks if parts of PATH are lost or overwritten. Set $env:SYSTEM_FLAG and $env:PERMANENT_FLAG to test each. +# If no flag is provided the process/shell values are tested. See the CI file for an example. + +refreshenv + +$repo_root = [System.IO.Path]::GetDirectoryName((resolve-path "$PSScriptRoot")) + +$PATH_USER_BEFORE = [System.Environment]::GetEnvironmentVariable("PATH", "User") +$PATH_MACHINE_BEFORE = [System.Environment]::GetEnvironmentVariable("PATH", "Machine") +$PATH_Process_BEFORE = [System.Environment]::GetEnvironmentVariable("PATH", "Process") + +try { + + + & "$repo_root/emsdk.ps1" install latest + + $esc = '--%' + & "$repo_root/emsdk.ps1" activate latest $esc $env:PERMANENT_FLAG $env:SYSTEM_FLAG + + $PATH_USER = [System.Environment]::GetEnvironmentVariable("PATH", "User") + $PATH_MACHINE = [System.Environment]::GetEnvironmentVariable("PATH", "Machine") + $PATH_Process = [System.Environment]::GetEnvironmentVariable("PATH", "Process") + + if ($env:SYSTEM_FLAG) { + echo "--system test............................." + $path_before_arr = $PATH_MACHINE_BEFORE.Split(';') + $path_arr = $PATH_MACHINE.Split(';') + } + elseif ($env:PERMANENT_FLAG) { + echo "--permanent test.........................." + $path_before_arr = $PATH_USER_BEFORE.Split(';') + $path_arr = $PATH_USER.Split(';') + } else { + echo "no flag test (shell/process).............." + $path_before_arr = $PATH_Process_BEFORE.Split(';') + $path_arr = $PATH_Process.Split(';') + } + + + $EMSDK_Path = $path_arr | Where-Object { $_ -like "$repo_root*" } + $EMSDK_NODE_Path = $path_arr | Where-Object { $_ -like "$repo_root\node*" } + $EMSDK_PYTHON_Path = $path_arr | Where-Object { $_ -like "$repo_root\python*" } + $EMSDK_JAVA_Path = $path_arr | Where-Object { $_ -like "$repo_root\java*" } + $EMSDK_UPSTREAM_Path = $path_arr | Where-Object { $_ -like "$repo_root\upstream\emscripten*" } + + $number_of_items = $path_arr.count + [System.Collections.ArrayList]$rest_of_path = @() + Foreach ($item in $path_arr) { + if ( + ($item -like "$repo_root*") -or + ($item -like "$repo_root\node*") -or + ($item -like "$repo_root\python*") -or + ($item -like "$repo_root\java*") -or + ($item -like "$repo_root\upstream\emscripten*") + ) { + echo "$item is on the PATH" + } + else { + $rest_of_path.add($item) | Out-Null + } + } + + # compare the PATHs before activation and after activation + if (Compare-Object -ReferenceObject $path_before_arr -DifferenceObject $rest_of_path ) { + echo "Old path is ............................." + echo $path_before_arr + echo "Current rest of path is ................." + echo $rest_of_path + throw "some parts of PATH are removed" + } + + # Compare the other untouched PATH + if ($env:SYSTEM_FLAG) { + if (Compare-Object -ReferenceObject $PATH_USER_BEFORE.Split(';') -DifferenceObject $PATH_USER.Split(';') ) { + echo "Old user path is ...................." + echo $PATH_USER_BEFORE + echo "Current user path is ................" + echo $PATH_USER + throw "User PATH are changed while --system had been provided" + } + } + elseif ($env:PERMANENT_FLAG) { + if (Compare-Object -ReferenceObject $PATH_MACHINE_BEFORE.Split(';') -DifferenceObject $PATH_MACHINE.Split(';') ) { + echo "Old machine path is.................." + echo $PATH_MACHINE_BEFORE + echo "Current machine path is.............." + echo $PATH_MACHINE + throw "MACHINE PATH are changed while --system was not provided" + } + } else { + if ( + (Compare-Object -ReferenceObject $PATH_MACHINE_BEFORE.Split(';') -DifferenceObject $PATH_MACHINE.Split(';')) -or + (Compare-Object -ReferenceObject $PATH_MACHINE_BEFORE.Split(';') -DifferenceObject $PATH_MACHINE.Split(';')) + ) { + echo "Old machine path is.................." + echo $PATH_MACHINE_BEFORE + echo "Current machine path is.............." + echo $PATH_MACHINE + echo "Old user path is ...................." + echo $PATH_USER_BEFORE + echo "Current user path is ................" + echo $PATH_USER + throw "MACHINE/USER PATH are changed while no flag was provided" + } + } + + + + +} +finally { + # Recover pre-split PATH + refreshenv + + [Environment]::SetEnvironmentVariable("Path", $PATH_USER_BEFORE, "User") + try { + [Environment]::SetEnvironmentVariable("Path", $PATH_MACHINE_BEFORE, "Machine") + } + catch {} + + [Environment]::SetEnvironmentVariable("Path", $PATH_Process_BEFORE, "Process") + + # Recover pre activation env variables + [Environment]::SetEnvironmentVariable("EMSDK", $null, "User") + [Environment]::SetEnvironmentVariable("EM_CONFIG", $null, "User") + [Environment]::SetEnvironmentVariable("EMSDK_NODE", $null, "User") + [Environment]::SetEnvironmentVariable("EMSDK_PYTHON", $null, "User") + [Environment]::SetEnvironmentVariable("JAVA_HOME", $null, "User") + [Environment]::SetEnvironmentVariable("EM_CACHE", $null, "User") + + try { + [Environment]::SetEnvironmentVariable("EMSDK", $null, "Machine") + [Environment]::SetEnvironmentVariable("EM_CONFIG", $null, "Machine") + [Environment]::SetEnvironmentVariable("EMSDK_NODE", $null, "Machine") + [Environment]::SetEnvironmentVariable("EMSDK_PYTHON", $null, "Machine") + [Environment]::SetEnvironmentVariable("JAVA_HOME", $null, "Machine") + [Environment]::SetEnvironmentVariable("EM_CACHE", $null, "Machine") + } catch {} + + + [Environment]::SetEnvironmentVariable("EMSDK", $null, "Process") + [Environment]::SetEnvironmentVariable("EM_CONFIG", $null, "Process") + [Environment]::SetEnvironmentVariable("EMSDK_NODE", $null, "Process") + [Environment]::SetEnvironmentVariable("EMSDK_PYTHON", $null, "Process") + [Environment]::SetEnvironmentVariable("JAVA_HOME", $null, "Process") + [Environment]::SetEnvironmentVariable("EM_CACHE", $null, "Process") + + refreshenv + +} diff --git a/scripts/test_permanent.ps1 b/scripts/test_permanent.ps1 deleted file mode 100644 index 4c9da19..0000000 --- a/scripts/test_permanent.ps1 +++ /dev/null @@ -1,56 +0,0 @@ -$repo_root = [System.IO.Path]::GetDirectoryName((resolve-path "$PSScriptRoot")) - -& "$repo_root/emsdk.ps1" install latest -& "$repo_root/emsdk.ps1" activate latest --permanent - -$EMSDK_USER = [System.Environment]::GetEnvironmentVariable("EMSDK", "User") -$EM_CONFIG_USER = [System.Environment]::GetEnvironmentVariable("EM_CONFIG", "User") -$EMSDK_NODE_USER = [System.Environment]::GetEnvironmentVariable("EMSDK_NODE", "User") -$EMSDK_PYTHON_USER = [System.Environment]::GetEnvironmentVariable("EMSDK_PYTHON", "User") -$JAVA_HOME_USER = [System.Environment]::GetEnvironmentVariable("JAVA_HOME", "User") -$EM_CACHE_USER = [System.Environment]::GetEnvironmentVariable("EM_CACHE", "User") -$PATH_USER = [System.Environment]::GetEnvironmentVariable("PATH", "User") - -if (!$EMSDK_USER) { - throw "EMSDK is not set for the user" -} -if (!$EM_CONFIG_USER) { - throw "EM_CONFIG_USER is not set for the user" -} -if (!$EMSDK_NODE_USER) { - throw "EMSDK_NODE is not set for the user" -} -if (!$JAVA_HOME_USER) { - throw "JAVA_HOME is not set for the user" -} -if (!$EMSDK_PYTHON_USER) { - throw "EMSDK_PYTHON is not set for the user" -} -if (!$EM_CACHE_USER) { - throw "EM_CACHE is not set for the user" -} - - -$path_split = $PATH_USER.Split(';') - -$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 { $_ -like "$repo_root\node*" } -if (!$EMSDK_NODE_Path_USER) { - throw "$repo_root\\node is not added to path." -} -$EMSDK_PYTHON_Path_USER = $path_split | Where-Object { $_ -like "$repo_root\python*" } -if (!$EMSDK_PYTHON_Path_USER) { - throw "$repo_root\\python is not added to path." -} -$EMSDK_JAVA_Path_USER = $path_split | Where-Object { $_ -like "$repo_root\java*" } -if (!$EMSDK_JAVA_Path_USER) { - throw "$repo_root\\java is not added to path." -} - -$EMSDK_UPSTREAM_Path_USER = $path_split | Where-Object { $_ -like "$repo_root\upstream\emscripten*" } -if (!$EMSDK_UPSTREAM_Path_USER) { - throw "$repo_root\\upstream\emscripten is not added to path." -}