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.

Sign up for GitHub

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

Jump to bottom

Don't hardcode the path to v141 platform toolset #3434

Merged
merged 27 commits into from
Oct 18, 2019
Merged

Don't hardcode the path to v141 platform toolset #3434

merged 27 commits into from
Oct 18, 2019

Conversation

asklar
Copy link
Member

@asklar asklar commented Oct 16, 2019 •
edited
Loading

This change removes the hardcoded path to C:\Program Files (x86)\ ... where we go look for v141 tool. This addresses #3430 and #3435

Microsoft Reviewers: Open in CodeFlow

@asklar asklar requested a review from a team as a code owner October 16, 2019 00:44
@ghost ghost added the vnext label Oct 16, 2019
kmelmon
kmelmon approved these changes Oct 17, 2019
View reviewed changes
Copy link
Contributor

@kmelmon kmelmon left a comment

Choose a reason for hiding this comment

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

:shipit:

@asklar asklar removed the request for review from marlenecota October 17, 2019 01:43
@JunielKatarn
Copy link
Contributor

(Bumping to main conversation for visibility).
Please note. The VCTargetsPath variable is still partially hard-coded. The v150 suffix refers to "Visual Studio 2017" tools.

This will break run-windows if we switch to the v142 platform toolset of MSVC.

@asklar
Copy link
Member Author

asklar commented Oct 17, 2019

(Bumping to main conversation for visibility).
Please note. The VCTargetsPath variable is still partially hard-coded. The v150 suffix refers to "Visual Studio 2017" tools.

This will break run-windows if we switch to the v142 platform toolset of MSVC.

agree but since Office requires v141 for now, we can address that, move to default (v142) and remove the forced target path altogether in the same change

JunielKatarn
JunielKatarn reviewed Oct 17, 2019
View reviewed changes
Copy link
Contributor

@JunielKatarn JunielKatarn left a comment

Choose a reason for hiding this comment

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

Please double-check whether to include the ARM VC tools.

vnext/local-cli/runWindows/utils/msbuildtools.js Show resolved Hide resolved
vnext/local-cli/runWindows/utils/msbuildtools.js Outdated Show resolved Hide resolved
@asklar asklar merged commit 50f9233 into microsoft:master Oct 18, 2019
@asklar asklar self-assigned this Oct 18, 2019
This was referenced Oct 18, 2019
Closed
Closed
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@JunielKatarn JunielKatarn JunielKatarn left review comments

@chrisglein chrisglein chrisglein left review comments

@kmelmon kmelmon kmelmon approved these changes

Assignees

@asklar asklar

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@asklar @JunielKatarn @chrisglein @kmelmon @marlenecota