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] Rewrite CMake build system to be more target-based #12698

Merged
merged 18 commits into from
Aug 10, 2020
Merged

[vcpkg] Rewrite CMake build system to be more target-based #12698

merged 18 commits into from
Aug 10, 2020

Conversation

strega-nil
Copy link
Contributor

@strega-nil strega-nil commented Aug 2, 2020 •
edited
Loading

This is now unrelated to the vcxproj build system. We're going to do a different solution for the vcxproj build system.

Additionally, this PR removes the "pch.h" at the front of every vcpkglib file, and allows building for VS2015 in the CMake build system.

@Neumann-A
Copy link
Contributor

Stupid question which I always wondered about:
Why do you keep those vcxproj files around and not just use a VS cmake generator and generate the solution for VS?
I mean if you need some order in the solution you can use:
https://cmake.org/cmake/help/latest/prop_gbl/USE_FOLDERS.html
https://cmake.org/cmake/help/latest/command/source_group.html?highlight=s
etc.

@strega-nil
Copy link
Contributor Author

strega-nil commented Aug 2, 2020 •
edited
Loading

@Neumann-A we don't require people to install cmake before they bootstrap vcpkg, and we can't generate it ahead of time because cmake-generated projects have dependencies on the system they're run on.

@JackBoosY JackBoosY added category:vcpkg-bug The issue is with the vcpkg system (including helper scripts in `scripts/cmake/`) 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. labels Aug 3, 2020
@strega-nil strega-nil changed the title [vcpkg] Auto-generate vcxproj build system from CMake [vcpkg] Rewrite CMake build system to be more target-based Aug 3, 2020
@strega-nil strega-nil marked this pull request as ready for review August 3, 2020 21:16
@cenit
Copy link
Contributor

cenit commented Aug 5, 2020 •
edited
Loading

we don't require people to install cmake before they bootstrap vcpkg

But if it’s not available, vcpkg is going to download it at the first install request. So why not right at the bootstrap (similar to osx/Linux)?

@strega-nil
Copy link
Contributor Author

@cenit that would require writing the logic in powershell 2, which is a pain in the butt. I'd rather wait for us to have user-local installers and just cut it out entirely

@strega-nil strega-nil removed the category:vcpkg-bug The issue is with the vcpkg system (including helper scripts in `scripts/cmake/`) label Aug 8, 2020
@BillyONeal BillyONeal self-assigned this Aug 8, 2020
BillyONeal
BillyONeal requested changes Aug 8, 2020
View reviewed changes
Copy link
Member

@BillyONeal BillyONeal left a comment

Choose a reason for hiding this comment

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

Waiting for the '# GCC and clang have different names for the same warning' comment

toolsrc/cmake/utilities.cmake Outdated Show resolved Hide resolved
toolsrc/cmake/utilities.cmake Show resolved Hide resolved
toolsrc/cmake/utilities.cmake Show resolved Hide resolved
toolsrc/cmake/utilities.cmake Show resolved Hide resolved
toolsrc/CMakeLists.txt Show resolved Hide resolved
toolsrc/CMakeLists.txt Show resolved Hide resolved
toolsrc/src/pch.cpp Show resolved Hide resolved
ras0219
ras0219 approved these changes Aug 9, 2020
View reviewed changes
Copy link
Contributor

@ras0219 ras0219 left a comment

Choose a reason for hiding this comment

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

Please ensure we want to link stdc++fs under libc++, since that's a change in behavior.

toolsrc/CMakeLists.txt Outdated Show resolved Hide resolved
toolsrc/CMakeLists.txt Show resolved Hide resolved
toolsrc/CMakeLists.txt Outdated Show resolved Hide resolved
ras0219
ras0219 approved these changes Aug 10, 2020
View reviewed changes
toolsrc/cmake/utilities.cmake Outdated Show resolved Hide resolved
toolsrc/cmake/utilities.cmake Outdated Show resolved Hide resolved
toolsrc/cmake/utilities.cmake Outdated Show resolved Hide resolved
toolsrc/cmake/utilities.cmake Outdated Show resolved Hide resolved
@strega-nil @ras0219
Apply suggestions from code review
6275686 
Thanks @ras0219!

Co-authored-by: ras0219 <533828+ras0219@users.noreply.github.com>
@strega-nil strega-nil merged commit 895678d into microsoft:master Aug 10, 2020
@strega-nil strega-nil deleted the vcxproj-fix branch September 10, 2020 21:21
strega-nil added a commit to strega-nil/vcpkg that referenced this pull request May 5, 2021
@strega-nil @ras0219
[vcpkg] Rewrite CMake build system to be more target-based (microsoft…
031e3b8 
…#12698)

* Change to using more target-focused cmake

* Add vcpkg_target_add_warning_options

* targetify the rest

* move the globs together

* Force-include pch.h on non-windows

* Rename VCPKGLIB_NON_PCH_* to VCPKGLIB_* in globs

* Remove `include "pch.h"`s

* missed a few lines

* fix build

* fix CMAKE_CURRENT_SOURCE_DIR

* try to fix VCPKG_REQUIRE_LINK_CXXFS

* change msvc-stl logic

* fix build

* CR

* clang-format

* Apply suggestions from code review

Thanks @ras0219!

Co-authored-by: JackBoosY <yuzaiyang@beyondsoft.com>
Co-authored-by: ras0219 <533828+ras0219@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@cenit cenit cenit left review comments

@ras0219 ras0219 ras0219 approved these changes

@BillyONeal BillyONeal BillyONeal approved these changes

@ras0219-msft ras0219-msft Awaiting requested review from ras0219-msft

Assignees

@BillyONeal BillyONeal

@PhoebeHui PhoebeHui

@JackBoosY JackBoosY

Labels
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.

None yet
7 participants
@strega-nil @Neumann-A @cenit @ras0219 @BillyONeal @PhoebeHui @JackBoosY