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

Add missing environment variables to MSVC_ENVIRONMENT_VARIABLE #2089

Merged
merged 8 commits into from
Sep 3, 2021

Conversation

tklajnscek
Copy link
Contributor

This change addresses item #1053

This changes visible behavior

The following changes are proposed:

  • add any missing environment variables to MSVC_ENVIRONMENT_VARIABLES so they get forwarded correctly

The purpose of this change

This also fixes Clang's ability to find the MSVC toolkit correctly as noted in my comment on the issue

…ABLES so they get forwarded correctly. Fixes CMake and Clang detecting wrong MSVC toolkit version, paths etc. Fixes microsoft#1053
src/kit.ts Outdated Show resolved Hide resolved
@bobbrow bobbrow added this to the 1.9.0 milestone Aug 30, 2021
… future users to know the context of it's removal
src/kit.ts Outdated
@@ -603,13 +603,19 @@ const MSVC_ENVIRONMENT_VARIABLES = [
'LIBPATH',
'NETFXSDKDir',
'Path',
//'Platform', - disabled at the request of @bobbrow as this causes some of MS internal projects to fail to build
Copy link
Member

@bobbrow bobbrow Sep 3, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We don't normally annotate our code with direct references to team members. I think it is sufficient to say that it is disabled because it is currently unnecessary and causes some projects to fail to build.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Changed as requested.

@tklajnscek
Copy link
Contributor Author

Not sure why the tests seem to have failed (C++ extension not installed etc.), let's see if merging latest develop fixes it :)

@bobbrow
Copy link
Member

bobbrow commented Sep 3, 2021

'C++ extension not installed' is not an error. I saw some flakiness on one of the test platforms recently. We need to investigate that.

@tklajnscek
Copy link
Contributor Author

Yeah, seems to have passed now!

@bobbrow bobbrow merged commit 064fcdf into microsoft:develop Sep 3, 2021
@github-actions github-actions bot locked and limited conversation to collaborators Jan 29, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants