Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

MSBuild/CL.exe fails to launch when called from a nodeJS script via Yarn #1566

Merged
merged 1 commit into from
Sep 13, 2020

Conversation

asklar
Copy link
Contributor

@asklar asklar commented Sep 9, 2020

Description

New tool, Bug fixing, or Improvement? Bug

Some tools like Yarn will spawn nodeJS via CreateProcess and pass an environment block that contains duplicated environment variables, e.g:
image

Windows doesn't do any checking of environment variables being duplicated in CreateProcess so the node.exe process gets created with the two names. Then in our case, MSBuild gets launched from within node.exe, which later launches CL.exe and other build tools.

The Azure DevOps CI pipeline sets NPM_CONFIG_CACHE (all caps), while yarn will add the lowercase npm_config_cache to the environment block when a process is launched via child_process.exec()

As a result of this, we are hitting an error in our CI because MultiTaskTool is probably putting variables in a case-insensitive dictionary and doesn't expect to find the same variable twice:
C:\Program Files (x86)\Microsoft Visual Studio\2019\Enterprise\MSBuild\Microsoft\VC\v160\Microsoft.CppCommon.targets(375,5): error MSB6001: Invalid command line switch for "CL.exe". System.ArgumentException: Item has already been added. Key in dictionary: 'NPM_CONFIG_CACHE' Key being added: 'npm_config_cache' [D:\a\1\s\vnext\Common\Common.vcxproj]

See example run: https://dev.azure.com/ms/react-native-windows/_build/results?buildId=107982&view=logs&j=5435db8c-d05a-5d7b-87ca-9953c4461652&t=397ce80a-0141-5d29-7c56-c592461a0308&l=4585

Related issue:

dotnet/msbuild#5726

Check list

  • Related issue / work item is attached
  • Tests are written (if applicable)
  • Documentation is updated (if applicable)
  • Changes are tested and related VM images are successfully generated

@asklar
Copy link
Contributor Author

asklar commented Sep 9, 2020

@AlenaSviridenko
Copy link
Contributor

/azp run windows2016, windows2019

@azure-pipelines
Copy link

Azure Pipelines successfully started running 2 pipeline(s).

@asklar
Copy link
Contributor Author

asklar commented Sep 13, 2020

@maxim-lobanov how do we get this merged?

@maxim-lobanov
Copy link
Contributor

@asklar , sorry for delay. I believe it is ready for merge.
Just want to get one more review from @alepauly , could you please take a look?

@asklar
Copy link
Contributor Author

asklar commented Sep 13, 2020

@maxim-lobanov thanks. On a related note I'm seeing something weird I'm hoping you might know the answer to.
If I delete the env var regkey:
reg delete "HKLM\SYSTEM\CurrentControlSet\Control\Session Manager\Environment" /v NPM_CONFIG_CACHE /f
then set the lowercase one (that I saved in $npm_config_prefix in powershell:
$env:npm_config_prefix = $npm_config_prefix
setx npm_config_cache $npm_config_cache /M
Then that task sees the lowercase variable: link

But the next task, prints out the environment, and that one again has the uppercase version. link
Any idea what might be going on?

@maxim-lobanov
Copy link
Contributor

@maxim-lobanov thanks. On a related note I'm seeing something weird I'm hoping you might know the answer to.
If I delete the env var regkey:
reg delete "HKLM\SYSTEM\CurrentControlSet\Control\Session Manager\Environment" /v NPM_CONFIG_CACHE /f
then set the lowercase one (that I saved in $npm_config_prefix in powershell:
$env:npm_config_prefix = $npm_config_prefix
setx npm_config_cache $npm_config_cache /M
Then that task sees the lowercase variable: link

But the next task, prints out the environment, and that one again has the uppercase version. link
Any idea what might be going on?

I think this command doesn't work at all because registry changes are applied only after reboot.

reg delete "HKLM\SYSTEM\CurrentControlSet\Control\Session Manager\Environment" /v NPM_CONFIG_CACHE /f

The same about setx npm_config_cache $npm_config_cache /M

$env:npm_config_prefix = $npm_config_prefix works as expected but this way, you set variable only within single PowerShell session.
The single way to pass variable to next steps is task.setvariable as far as I know.

@asklar
Copy link
Contributor Author

asklar commented Sep 13, 2020

@maxim-lobanov thanks. On a related note I'm seeing something weird I'm hoping you might know the answer to.
If I delete the env var regkey:
reg delete "HKLM\SYSTEM\CurrentControlSet\Control\Session Manager\Environment" /v NPM_CONFIG_CACHE /f
then set the lowercase one (that I saved in $npm_config_prefix in powershell:
$env:npm_config_prefix = $npm_config_prefix
setx npm_config_cache $npm_config_cache /M
Then that task sees the lowercase variable: link
But the next task, prints out the environment, and that one again has the uppercase version. link
Any idea what might be going on?

I think this command doesn't work at all because registry changes are applied only after reboot.

reg delete "HKLM\SYSTEM\CurrentControlSet\Control\Session Manager\Environment" /v NPM_CONFIG_CACHE /f

The same about setx npm_config_cache $npm_config_cache /M

$env:npm_config_prefix = $npm_config_prefix works as expected but this way, you set variable only within single PowerShell session.
The single way to pass variable to next steps is task.setvariable as far as I know.

@maxim-lobanov I'm not sure about the reboot requirement, it seems to work ok for me locally if I create a new cmd window (it doesn't work if the cmd process is a child of the original one):
image

Regarding task.setvariable, that would work if it preserved the casing, but it doesn't, which takes me back to square 0 :(

@maxim-lobanov maxim-lobanov merged commit db9d42d into actions:main Sep 13, 2020
@maxim-lobanov
Copy link
Contributor

@asklar merged. This change should be deployed in 1-2 weeks and should unblock you fully

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants