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

[vcpkg/scripts] Add a way to get cmake compiler settings/flags #12936

Merged
merged 46 commits into from
Nov 10, 2020
Merged

[vcpkg/scripts] Add a way to get cmake compiler settings/flags #12936

merged 46 commits into from
Nov 10, 2020

Conversation

Neumann-A
Copy link
Contributor

Testing the extraction of compiler settings from cmake

Neumann-A
Neumann-A commented Aug 16, 2020
View reviewed changes
scripts/cmake/vcpkg_configure_make.cmake Outdated
Comment on lines 430 to 431
set(ENV{_CL_} "$ENV{_CL_} /DWINAPI_FAMILY=WINAPI_FAMILY_APP /D__WRL_NO_DEFAULT_LIB_ -FU\"${VCToolsInstallDir}/lib/x86/store/references/platform.winmd\"")
set(ENV{_LINK_} "$ENV{_LINK_} /MANIFEST /DYNAMICBASE WindowsApp.lib /WINMD:NO /APPCONTAINER")
endif()
if(VCPKG_TARGET_ARCHITECTURE STREQUAL x64)
set(ENV{_LINK_} "$ENV{_LINK_} -MACHINE:x64")
elseif(VCPKG_TARGET_ARCHITECTURE STREQUAL x86)
set(ENV{_LINK_} "$ENV{_LINK_} -MACHINE:x86")
elseif(VCPKG_TARGET_ARCHITECTURE STREQUAL arm)
set(ENV{_LINK_} "$ENV{_LINK_} -MACHINE:ARM")
elseif(VCPKG_TARGET_ARCHITECTURE STREQUAL arm64)
set(ENV{_LINK_} "$ENV{_LINK_} -MACHINE:ARM64")
set(ENV{_LINK_} "$ENV{_LINK_} /MANIFEST /DYNAMICBASE /WINMD:NO /APPCONTAINER")
Copy link
Contributor Author

Choose a reason for hiding this comment

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

CMake does not set these flags. I just took them from a default UWP VS project.

@Neumann-A Neumann-A mentioned this pull request Aug 16, 2020
Merged
1 task
@JackBoosY JackBoosY added the category:tool-update The issue is with build tool or build script, which requires update or should be executed correctly label Aug 17, 2020
@Neumann-A Neumann-A marked this pull request as ready for review August 17, 2020 08:54
@Neumann-A Neumann-A mentioned this pull request Aug 17, 2020
Merged
8 tasks
@JackBoosY JackBoosY requested a review from strega-nil August 18, 2020 02:27
strega-nil
strega-nil suggested changes Aug 19, 2020
View reviewed changes
Copy link
Contributor

@strega-nil strega-nil left a comment

Choose a reason for hiding this comment

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

Just some minor copy changes :)

scripts/cmake/vcpkg_get_cmake_vars.cmake Outdated Show resolved Hide resolved
scripts/cmake/vcpkg_get_cmake_vars.cmake Outdated Show resolved Hide resolved
scripts/get_cmake_vars/CMakeLists.txt Outdated Show resolved Hide resolved
scripts/get_cmake_vars/CMakeLists.txt Outdated Show resolved Hide resolved
scripts/get_cmake_vars/CMakeLists.txt Outdated Show resolved Hide resolved
scripts/get_cmake_vars/CMakeLists.txt Outdated Show resolved Hide resolved
scripts/get_cmake_vars/CMakeLists.txt Outdated Show resolved Hide resolved
scripts/get_cmake_vars/CMakeLists.txt Outdated Show resolved Hide resolved
@Neumann-A
Copy link
Contributor Author

Neumann-A commented Aug 19, 2020 •
edited
Loading

draft until run against #9966
works good locally.

@Neumann-A Neumann-A marked this pull request as draft August 19, 2020 23:44
@Neumann-A
Copy link
Contributor Author

@ras0219: about spaces. I literally tried hours to make it work. Even if you get it to through configure for some ports it is very likely that other ports requiring an external tool/script is not handling spaces in paths correctly and the build fails (even using \ , "", '', everything together or Linux itself). As such I don't bother testing with whitespaces anymore.

vcpkg/scripts/cmake/vcpkg_configure_make.cmake

Lines 218 to 221 in 70f380e

if(CURRENT_PACKAGES_DIR MATCHES " " OR CURRENT_INSTALLED_DIR MATCHES " ")
# Don't bother with whitespace. The tools will probably fail and I tried very hard trying to make it work (no success so far)!
message(WARNING "Detected whitespace in root directory. Please move the path to one without whitespaces! The required tools do not handle whitespaces correctly and the build will most likely fail")
endif()

Furthermore there are currently not that many make based ports for windows so testing is a bit difficult due to a high probability that the port is also using vcpkg_fixup_pkgconfig. So unless #13126 is finally merged I will not add an explicit test for it (If it is merged x264 could be used).

Since the compiler flags will always be a plain string and no list in cmake it is not required to change the script for meson since the flags need to be rearranged into a python list anyway which could be done in vcpkg_configure_meson. (For which I also already have an PR but we probably want to update meson first or wait until 0.56 since they now allow to pass a cmake toolchain for their find_dep stuff.)

@Neumann-A
Copy link
Contributor Author

@ras0219 The problem is setting LDFLAGS. If the -L<somepath> has a space autotools configure chokes because the configure check passes the path with spaces as separate parameters no matter the case. I mean we could try removing the setting of -L flags in LDFLAGS and just rely on compiler specific env flags but i also think that pkgconfig might break configure for pkg_search since it is adding -L flags.

vcpkg/scripts/cmake/vcpkg_configure_make.cmake

Line 488 in b7056e9

string(REGEX REPLACE "[ \t]+/" " -" LDFLAGS_${_VAR_SUFFIX} "-L${_VCPKG_INSTALLED}${PATH_SUFFIX_${_VAR_SUFFIX}}/lib ${LD_FLAGS_GLOBAL} ${VCPKG_LINKER_FLAGS_${_VAR_SUFFIX}}")

So how should I proceed with this PR?

@JackBoosY
Copy link
Contributor

Done?

@Neumann-A
Copy link
Contributor Author

@JackBoosY: Theoretically yes. There is nothing I can do to make spaces in paths work since after the last change configure is successful but the install step fails on paths with spaces.

@JackBoosY JackBoosY added category:port-update The issue is with a library, which is requesting update new revision info:reviewed Pull Request changes follow basic guidelines and removed category:port-update The issue is with a library, which is requesting update new revision labels Nov 10, 2020
@JackBoosY
Copy link
Contributor

@strega-nil Ping for review this PR again and merge it.

@strega-nil strega-nil merged commit 8de4ee8 into microsoft:master Nov 10, 2020
@Neumann-A Neumann-A deleted the add_get_cmake_vars branch November 10, 2020 18:02
@longnguyen2004 longnguyen2004 mentioned this pull request Nov 11, 2020
Closed
@ekilmer ekilmer mentioned this pull request Nov 15, 2020
Closed
@Neumann-A Neumann-A mentioned this pull request Apr 13, 2022
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ras0219 ras0219 ras0219 requested changes

@JackBoosY JackBoosY JackBoosY approved these changes

@strega-nil strega-nil strega-nil approved these changes

@BillyONeal BillyONeal Awaiting requested review from BillyONeal

Assignees

@JackBoosY JackBoosY

Labels
category:tool-update The issue is with build tool or build script, which requires update or should be executed correctly category:vcpkg-feature The issue is a new capability of the tool that doesn’t already exist and we haven’t committed info:reviewed Pull Request changes follow basic guidelines
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@Neumann-A @JackBoosY @strega-nil @ras0219 @BillyONeal