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

Fix systemversion("latest") on VS2017 #1424

Merged
merged 1 commit into from
Apr 15, 2020

Conversation

starkos
Copy link
Member

@starkos starkos commented Apr 15, 2020

What does this PR do?

My take on a cleaned-up version of #1317. Implements the workaround described here to enable VS 2017 to discover the correct version of the Windows 10 SDK.

h/t to @jeaiii to discovering the fix, and putting together the original PR.

How does this PR change Premake's behavior?

Should not be any breaking changes. Does add this markup to every VS 2017 project, regardless of whether systemversion("latest") is used or not:

<LatestTargetPlatformVersion>$([Microsoft.Build.Utilities.ToolLocationHelper]::GetLatestSDKTargetPlatformVersion('Windows', '10.0'))</LatestTargetPlatformVersion>

The alternative is to scan through all of the project configurations to see if the value has been set or not before writing, but the markup is ignored if not used, and querying all of the configurations can be relatively expensive. If no one complains, this is a more efficient approach.

Anything else we should know?

No.

Did you check all the boxes?

  • Focus on a single fix or feature; remove any unrelated formatting or code changes
  • Add unit tests showing fix or feature works; all tests pass
  • Mention any related issues N/A
  • Follow our coding conventions
  • Minimize the number of commits

You can now support Premake on our OpenCollective. Your contributions help us spend more time responding to requests like these!

@samsinsane samsinsane merged commit 07ab4cc into premake:master Apr 15, 2020
@starkos starkos deleted the fix/vs2017-system-latest branch April 16, 2020 18:23
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.

2 participants