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

[opengl] Use WindowsSdkDir and WindowsSdkVersion from vcvars to get the Windows SDK path correctly #11261

Closed
wants to merge 2 commits into from
Closed

[opengl] Use WindowsSdkDir and WindowsSdkVersion from vcvars to get the Windows SDK path correctly #11261

wants to merge 2 commits into from

Conversation

JackBoosY
Copy link
Contributor

@JackBoosY JackBoosY commented May 9, 2020 •
edited
Loading

Use environment variables instead of specified values to get the Windows SDK path.

Fix #11179 #8288

@JackBoosY JackBoosY added the info:internal This PR or Issue was filed by the vcpkg team. label May 9, 2020
@JackBoosY JackBoosY changed the title [opengl] Add a new function vcpkg_get_windows_sdk_path to get the Win… [opengl] Add a new function vcpkg_get_windows_sdk_path to get the Windows SDK path correctly May 9, 2020
cenit
cenit reviewed May 9, 2020
View reviewed changes
scripts/cmake/vcpkg_get_windows_sdk_path.cmake
@@ -0,0 +1,6 @@
# Returns Windows SDK path via out variable "ret"
function(vcpkg_get_windows_sdk_path ret)
set(WINDOWS_SDK_PATH $ENV{WindowsSdkDir})
Copy link
Contributor

Choose a reason for hiding this comment

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

Does it need a pass-through to be visible?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@cenit think we need to abstract these "concrete" functions, and there is no need to consider the specific acquisition process of these variables in portfile.cmake.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

vcpkg provides basic interfaces and variables, and portfile.cmake provides business processes.

Copy link
Contributor

Choose a reason for hiding this comment

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

Yes sure I just mean: is $ENV{WindowsSdkDir} visible when running cmake inside vcpkg, that masks almost all environment variables?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Maybe, I'm not sure.

Copy link
Member

Choose a reason for hiding this comment

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

@cenit It looks like that one is added by vcvarsall so we expose it.

Copy link
Contributor

Choose a reason for hiding this comment

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

vcvars is not loaded when chainload toolchain file is set, does it mean that any port using vcpkg_get_windows_sdk_path can't be build in that scenario?

@PhoebeHui
Copy link
Contributor

Related to #8329

@cenit cenit mentioned this pull request May 18, 2020
Closed
@JackBoosY
Copy link
Contributor Author

/azp run

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@JackBoosY JackBoosY marked this pull request as ready for review May 20, 2020 09:05
@JackBoosY JackBoosY added category:port-bug The issue is with a library, which is something the port should already support requires:discussion category:vcpkg-feature The issue is a new capability of the tool that doesn’t already exist and we haven’t committed labels May 22, 2020
vicroms
vicroms requested changes Jul 2, 2020
View reviewed changes
ports/opengl/portfile.cmake
vcpkg_get_program_files_32_bit(PROGRAM_FILES_32_BIT)
vcpkg_get_windows_sdk(WINDOWS_SDK)
vcpkg_get_windows_sdk_path(WINDOWS_SDK_PATH)
Copy link
Member

Choose a reason for hiding this comment

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

The port should be able to use the WindowsSdkDir environment variable directly, there is no need to have this function adding a level of indirection.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@vicroms @ras0219 @ras0219-msft
I think that for system or compiler variables, we should provide unified functions to contributors, because contributors do not need to pay attention to these things. And the Windows SDK PATH has been directly used in some ports, we have reason to provide a unified function to deal with it.

@BillyONeal BillyONeal changed the title [opengl] Add a new function vcpkg_get_windows_sdk_path to get the Windows SDK path correctly [opengl] Use WindowsSdkDir and WindowsSdkVersion from vcvars to get the Windows SDK path correctly Jul 2, 2020
@BillyONeal BillyONeal requested a review from vicroms July 2, 2020 22:27
@BillyONeal
Copy link
Member

Fixes #8288

BillyONeal added a commit to BillyONeal/vcpkg that referenced this pull request Jul 2, 2020
@BillyONeal
[opengl wincrypt winsock2] Use $ENV{WindowsSdkDir} instead of hard co…
846c135 
…ding Windows SDK paths.

Also delete vcpkg_get_program_files_32_bit because it was used in only one place.

Resolves microsoft#8288
Obsoletes microsoft#11421, microsoft#11261, microsoft#8329
@BillyONeal BillyONeal mentioned this pull request Jul 2, 2020
Merged
ras0219-msft pushed a commit that referenced this pull request Jul 6, 2020
@BillyONeal
[opengl wincrypt winsock2] Use $ENV{WindowsSdkDir} instead of hard co…
8eda81c 
…ding Windows SDK paths. (#12232)

Also delete vcpkg_get_program_files_32_bit because it was used in only one place.

Resolves #8288
Obsoletes #11421, #11261, #8329
@JackBoosY JackBoosY closed this Jul 7, 2020
@JackBoosY JackBoosY deleted the dev/jack/11179 branch July 7, 2020 01:36
strega-nil pushed a commit to strega-nil/vcpkg that referenced this pull request May 5, 2021
@BillyONeal
[opengl wincrypt winsock2] Use $ENV{WindowsSdkDir} instead of hard co…
a449d72 
…ding Windows SDK paths. (microsoft#12232)

Also delete vcpkg_get_program_files_32_bit because it was used in only one place.

Resolves microsoft#8288
Obsoletes microsoft#11421, microsoft#11261, microsoft#8329
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@BillyONeal BillyONeal BillyONeal left review comments

@cenit cenit cenit left review comments

@redbaron redbaron redbaron left review comments

@vicroms vicroms Awaiting requested review from vicroms

Assignees

@PhoebeHui PhoebeHui

Labels
category:port-bug The issue is with a library, which is something the port should already support category:vcpkg-feature The issue is a new capability of the tool that doesn’t already exist and we haven’t committed info:internal This PR or Issue was filed by the vcpkg team.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

[opengl] build failure
6 participants
@JackBoosY @PhoebeHui @BillyONeal @redbaron @cenit @vicroms