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_configure_cmake] Introduce REQUIRE_ALL_PACKAGES #15808

Closed
wants to merge 3 commits into from
Closed

[vcpkg_configure_cmake] Introduce REQUIRE_ALL_PACKAGES #15808

wants to merge 3 commits into from

Conversation

ras0219
Copy link
Contributor

@ras0219 ras0219 commented Jan 22, 2021 •
edited
Loading

A recurring problem with many builds are uses of autodetected dependencies; depending on the specific install order, different CI runs results in different results for detection which results in flaky, unrobust behavior.

This PR introduces new flags to vcpkg_configure_cmake() to control this: REQUIRE_ALL_PACKAGES, DISABLE_PACKAGES, and OPTIONAL_PACKAGES.

A deficiency in the current PR implementation is that disables are also applied to transitive find_package()/find_dependency() calls; this is problematic in that it (in the most general case) requires each port to calculate the transitive closure of packages used under the current configuration and compute a disable list using all that information. Ideally, we would only apply DISABLE_PACKAGES to the first level of find_package(), which allows these settings to only concern themselves with local information.

@JackBoosY JackBoosY self-assigned this Jan 22, 2021
@JackBoosY JackBoosY added category:tool-update The issue is with build tool or build script, which requires update or should be executed correctly info:internal This PR or Issue was filed by the vcpkg team. labels Jan 22, 2021
@cenit
Copy link
Contributor

cenit commented Jan 22, 2021

Very nice and very useful. Might help also reducing patchsets in some ports (and improve their maintainability too)

@cenit
Copy link
Contributor

cenit commented Jan 22, 2021 •
edited
Loading

Is it necessary to remove the QUIET keyword from ${ARGS}? Or putting REQUIRED at the end always wins?

Neumann-A
Neumann-A reviewed Jan 22, 2021
View reviewed changes
scripts/buildsystems/vcpkg.cmake Outdated
@@ -605,20 +623,20 @@ macro(${VCPKG_OVERRIDE_FIND_PACKAGE_NAME} name)
else()
set(Boost_COMPILER "-vc140")
endif()
_find_package(${ARGV})
_find_package(${ARGV} ${_REQUIRED})
elseif("${name}" STREQUAL "ICU" AND EXISTS "${_VCPKG_INSTALLED_DIR}/${VCPKG_TARGET_TRIPLET}/include/unicode/utf.h")
Copy link
Contributor

Choose a reason for hiding this comment

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

everything below should probably be installed within a port via a vcpkg-cmake-wrapper.cmake instead of being in the toolchain.

Copy link
Contributor

Choose a reason for hiding this comment

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

i agree. They belong to a time in which vcpkg-cmake-wrapper.cmake was not existing...

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yep, but I didn't want to make that change in this PR.

@cenit
Copy link
Contributor

cenit commented Jan 22, 2021

Just few years ago @Neumann-A (or me) would have triggered a full rebuild just after this PR was merged, enabling the REQUIRE_ALL_PACKAGES by default. And I would have really enjoyed the result of that hard work!
It would be a very reasonable default, to have more reproducible builds. But I understand that it would be very very difficult, with many broken ports at first. The outcome would be a better vcpkg too

@ras0219
Copy link
Contributor Author

ras0219 commented Jan 22, 2021

We unfortunately can no longer do such "break the world" operations with the new versioning and registries features -- we've lost "total" atomicity in exchange for stability.

However, we do intend to make this a requirement for all new ports and to eventually apply this policy to every current port.

@ras0219 ras0219 force-pushed the dev/roschuma/cmake-opt-deps branch 15 times, most recently from acc9cd5 to 56e849e Compare February 1, 2021 01:46
@ras0219 ras0219 force-pushed the dev/roschuma/cmake-opt-deps branch 2 times, most recently from 46de5b7 to 9dabfc5 Compare February 11, 2021 20:12
@ras0219-msft ras0219-msft mentioned this pull request Feb 11, 2021
Closed
@ras0219-msft
Copy link
Contributor

/azp run

@ras0219 ras0219 force-pushed the dev/roschuma/cmake-opt-deps branch 2 times, most recently from f93ee2d to 610f347 Compare February 21, 2021 21:57
@NancyLi1013 NancyLi1013 mentioned this pull request Feb 23, 2021
Merged
@JackBoosY JackBoosY mentioned this pull request Feb 25, 2021
Closed
@JackBoosY JackBoosY linked an issue Feb 25, 2021 that may be closed by this pull request
[gdal:x64-windows-static] missing geos dependencies #15526
Closed
@NancyLi1013 NancyLi1013 mentioned this pull request Mar 1, 2021
Merged
@JackBoosY JackBoosY mentioned this pull request Mar 4, 2021
Closed
@wrobelda
Copy link
Contributor

wrobelda commented Mar 15, 2021 •
edited
Loading

Don't want to sound needy or something, but it is a blocker for a number of PRs right now, which is a bummer. Has this stalled?

@NancyLi1013 NancyLi1013 mentioned this pull request Mar 25, 2021
Merged
@JackBoosY JackBoosY mentioned this pull request Mar 26, 2021
Merged
ras0219-msft
ras0219-msft reviewed Mar 26, 2021
View reviewed changes
ports/armadillo/vcpkg.json Show resolved Hide resolved
ports/armadillo/vcpkg.json Outdated Show resolved Hide resolved
ports/gdal/vcpkg.json Outdated Show resolved Hide resolved
ports/geos/vcpkg.json Show resolved Hide resolved
@ras0219-msft ras0219-msft force-pushed the dev/roschuma/cmake-opt-deps branch 6 times, most recently from 0f88793 to d121552 Compare April 2, 2021 00:34
@ras0219 @ras0219-msft
[docs/regenerate.ps1] Fix on Linux
52dfb19
@ras0219 @ras0219-msft
[vcpkg_configure_cmake] Add VCPKG_REQUIRE_ALL_PACKAGES
49402d1
@ras0219 @ras0219-msft
[many ports] Apply REQUIRE_ALL_PACKAGES
aaadf6f 
[opencv4] Disable vendored quirc library

[libsndfile] Disable Sndio dependency

[armadillo] Disable PkgConfig

WIP

[gdal] Specify options on linux [geos][libxml2][openjpeg] .pc fixes

[armadillo][gdal][geos] CR comments

[pangolin] Apply REQUIRE_ALL_PACKAGES
@ras0219-msft
Copy link
Contributor

Note #18393 and #18473 for examples of transitive dependency issues.

@ras0219-msft ras0219-msft mentioned this pull request Jun 16, 2021
Merged
@dg0yt
Copy link
Contributor

dg0yt commented Jun 19, 2021

Note #18393 and #18473 for examples of transitive dependency issues.

Or now simage on Windows: #17519, #17519

@dg0yt dg0yt mentioned this pull request Jun 19, 2021
Merged
@JackBoosY
Copy link
Contributor

This PR has been inactive for a long time, please reopen it if there is any progress.

@JackBoosY JackBoosY closed this Sep 10, 2021
@cenit
Copy link
Contributor

cenit commented Sep 10, 2021

😞 😢 😭

@JackBoosY
Copy link
Contributor

@cenit Don't worry, I believe this PR will continue.

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-msft ras0219-msft ras0219-msft left review comments

@Neumann-A Neumann-A Neumann-A left review comments

@JackBoosY JackBoosY JackBoosY requested changes

Assignees

@JackBoosY JackBoosY

Labels
category:tool-update The issue is with build tool or build script, which requires update or should be executed correctly 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.

[gdal:x64-windows-static] missing geos dependencies
7 participants
@ras0219 @cenit @ras0219-msft @wrobelda @dg0yt @JackBoosY @Neumann-A