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

[vcpkg] Use triplet file results in dependency logic expressions #8601

Merged
merged 94 commits into from
Feb 3, 2020

Conversation

Rastaban
Copy link
Contributor

Previously the logic expression evaluation would do a sub string search of the triplet name to determine if an identifier was 'true' or 'false'. With this PR it uses the actual values set inside the triplet file. Most of the changes to vcpkg.exe was refactoring the dependency logic to allow running the triplet file earlier so its results could be used in the qualified dependency resolution.

This PR also fixes the previously undocumented "Supports" CONTROL tag to also use the logic expression evaluation. Currently the only place the supports tag is used is in the ci command. This will help clean up the results in the CI pipeline - especially when things switch over to using the checked in baseline. The tag could also be used to provide better error messages on failure, but I will leave that for someone else to do in a later PR.

The unit tests are not complete.

@Rastaban Rastaban added the info:internal This PR or Issue was filed by the vcpkg team. label Oct 14, 2019
@Rastaban Rastaban changed the title Use triplet file contents [vcpkg] Use triplet file contents in dependency logic expressions Oct 14, 2019
@Rastaban Rastaban changed the title [vcpkg] Use triplet file contents in dependency logic expressions [vcpkg] Use triplet file results in dependency logic expressions Oct 14, 2019
@vicroms vicroms requested a review from ras0219-msft October 14, 2019 23:26
@vicroms vicroms self-assigned this Oct 14, 2019
@vicroms vicroms added the category:vcpkg-feature The issue is a new capability of the tool that doesn’t already exist and we haven’t committed label Oct 14, 2019
@vicroms
Copy link
Member

vicroms commented Oct 15, 2019

There's a bug in the dependency graph calculation, transitive dependencies are not being added to the install plan.

Examples:

PS D:\src\vcpkg> ./vcpkg install libzippp
The following packages will be built and installed:
  * libzip[core,bzip2,openssl]:x86-windows
    libzippp[core]:x86-windows
Additional packages (*) will be modified to complete this operation.
Starting package 1/2: libzip:x86-windows

# Missing bzip and openssl packages in install plan
PS D:\src\vcpkg> ./vcpkg install opencv
The following packages will be built and installed:
    opencv[dnn,jpeg,webp,opengl,tiff,png,core]:x86-windows
  * opencv4[core,dnn,jpeg,webp,opengl,tiff,png]:x86-windows
Additional packages (*) will be modified to complete this operation.
Starting package 1/2: opencv4:x86-windows

# Missing a lot of dependencies

@vicroms
Copy link
Member

vicroms commented Oct 15, 2019

It seems to be ignoring Build-Depends: completely.
E.g.:

ports/libzip/CONTROL

Source: libzip
Version: rel-1-5-2
Homepage: https://github.com/nih-at/libzip
Build-Depends: zlib
Default-Features: openssl, bzip2
Description: A library for reading, creating, and modifying zip archives.

Feature: bzip2
Build-Depends: bzip2
Description: Support bzip2-compressed zip archives

Feature: openssl
Build-Depends: openssl
Description: AES (encryption) support using OpenSSL
PS D:\src\viromer\vcpkg> ./vcpkg install libzip
The following packages will be built and installed:
    libzip[openssl,core,bzip2]:x86-windows
Starting package 1/1: libzip:x86-windows

Ignored direct dependency on zlib and dependencies from default features bzip2 and openssl (which in turn depends on openssl-windows).

spec.triplet(),
build_package_options,
var_provider,
std::move(*feature_dependencies),
Copy link
Contributor

Choose a reason for hiding this comment

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

These moves can get dropped, BuildPackageConfig takes const references to everything now.

@@ -713,6 +673,14 @@ namespace vcpkg::Build
Hash::Algorithm::Sha1));
Copy link
Contributor

Choose a reason for hiding this comment

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

Above we add the feature_list to the abi file. config.feature_list is no longer stable and should be sorted before being added to the abi file.

@JackBoosY
Copy link
Contributor

This PR is what we need!

@cbezault
Copy link
Contributor

@vicroms, sigh, I probably introduced a bug in one of my last minute changes.

PackageSpec::from_name_and_triplet(dep.depend.name, m_spec.triplet())
.value_or_exit(VCPKG_LINE_INFO),
dep_feature);
}
Copy link
Contributor

Choose a reason for hiding this comment

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

We need to add in the default FeatureSpec here. core is not in the feature list of dep.depend.features.

"parser.h",
"lexer.h",
};
static constexpr Span<const StringLiteral> restricted_lists[] = {
Copy link
Member

Choose a reason for hiding this comment

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

Shouldn't we add C++ Standard Library header names as well? At least one library (sfsexp) is known to install cstring.h directly into include/.

@dan-shaw
Copy link
Contributor

This looks like it has passed the CI. Before merging this, can we add more documentation as to what was changed (such as adding a new build policy)?

@ras0219-msft
Copy link
Contributor

ras0219-msft commented Jan 31, 2020

Also, @winsoft666, as part of this PR I discovered that teemo installing include/slice.h caused x264 to fail to build (and it's easy to see that #include "slice.h" is likely to conflict with other libraries' private files).

I resolved this by moving the includes into include/teemo/ -- CMake projects should be unaffected and can continue using the same include paths, but MSBuild projects will now need to use #include <teemo/teemo.h>. I'd strongly recommend considering making this change upstream and having all users always include teemo/teemo.h.

If you'd like me to open an issue, I'd be happy to do so.

Copy link
Member

@vicroms vicroms left a comment

Choose a reason for hiding this comment

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

Let's Get This Merged

@ras0219-msft ras0219-msft merged commit e62d136 into microsoft:master Feb 3, 2020
@cbezault
Copy link
Contributor

cbezault commented Feb 4, 2020

bless

@JackBoosY
Copy link
Contributor

With this PR, I think we can further improve the CI test to add feature tests.

strega-nil pushed a commit to strega-nil/vcpkg that referenced this pull request May 5, 2021
…s for dependency resolution. (microsoft#8601)

* remove unfinished "supports" tag

* extract "supports" from control files
But do nothing with the value

* Start `Supports` documentation

* Use Supports in a bunch of control files

I only tried matching the already existing logic in the portfile.cmake.

* Cmake var provider (microsoft#8)

* Cmake var provider (microsoft#9)

* fix windows build (microsoft#10)

* Add missing files to build

* Fix test (microsoft#11)

* adding hooks for cmake variables in expressions

* Adding hooks for 'supports' in CI test

* Fix test (microsoft#12)

* Add overrides to evaluation environment

* use "supported" tag in CI testing

* cleanup comment

* Fix issues with PR

* [var_provider] Get library linkage variables from triplet

* Fix compilation errors in tests

* Add unimplemented functions

* Fix unit tests part 1

* Fix issue when buildtrees dir does not exist

* Change binary output hash

* Fix handling of * feature

* Add core feature when using *

* Do not add Default-Features when installing 'core'

* [vcpkg] WIP. 6 failing tests.

* [vcpkg] WIP. 1 failing tests.

* [vcpkg] WIP. 0 failing tests.

* [vcpkg] Removed 'remove_graph'. 0 failing tests.

* [vcpkg] Removed 'install_graph'. 0 failing tests.

* [vcpkg] Remove AnyAction; replace with ActionPlan

* [vcpkg] Minor cleanup.

* [vcpkg][z3][qt5-connectivity][qt5-purchasing] Improve error messages while parsing. Fix a few trivial port issues.

* [vcpkg] Work around ICE with MSVC v140

* [vcpkg] Add purge on fail to decompress for CI

* [vcpkg] Fix parsing of nested parentheses in qualifiers

* [vcpkg] Fix Linux builds (explicit qualification in declaration)

* [vcpkg] Fix Build-Depends implying default features. Fix qualified dependencies regression.

* [mmx] Add to skip list and full rebuild -- mmx causes problems by installing 'sched.h'

* [libpqxx][mqtt-cpp] Prevent installing include/CMakeLists.txt

* [cppitertools] Fix installed include namespace (should be include/cppitertools)

* [libsoundio] Move headers into soundio/ subdirectory as per original cmake

* [ci.baseline] Temporarily skip charls due to conflict with dcmtk

* [vcpkg] Add restricted include files post build check -- bump global abi version

* [libsoundio] Hotfix stray line in portfile

* [vcpkg] Fix regression: CMake information was not being displayed for build-and-install actions

* [jsonnet] Fix installation of internal headers; use system nlohmann-json

* [grpc][upb] Teach grpc to use packaged upb. Add find_package(upb). Remove inappropriate upb features.

* [zfp] Move problematic 'include/bitstream.h' to 'include/zfp/bitstream.h'

* [x265] Bump control version to trigger rebuild after zfp conflict

* [akali] Disable parallel configure

* [dirent][dlfcn-win32][getopt-win32][pthreads] Grandfather into VCPKG_POLICY_ALLOW_RESTRICTED_HEADERS

* [ci.baseline] Update baseline for improved upb support

* [tgui] Disable parallel configure

* [libiconv] Enable VCPKG_POLICY_ALLOW_RESTRICTED_HEADERS

* [aws-sdk-cpp] Disable parallel configure

* [vcpkg] Implement policy VCPKG_POLICY_ALLOW_RESTRICTED_HEADERS

* [aws-sdk-cpp] Fix amount of escaping semicolons -- Note: I do not know the root cause requiring this change

* [libodb-sqlite] Fix configuring into source directory

* [gettext] Grandfather into VCPKG_POLICY_ALLOW_RESTRICTED_HEADERS

* [libodb] DISABLE_PARALLEL_CONFIGURE

* [vcpkg] Add 'config.h' and 'local.h' to restricted header list

* [mcpp] Remove unused and problematic include 'config.h' from installed files

* [teemo] Move installed headers into subdirectory to prevent conflicts with x265

* [ci.baseline] Update current OSX. Skip libmesh on all platforms due to heavy conflicts.

* [vcpkg] Add 'slice.h' as a restricted header

* [osg] Improve accuracy of dependencies (disable some, add some to Depends)

* [vcpkg] Skip invoking a subprocess for 0 specs in load_tag_vars

* [ci.baseline] Skip mongo-c-driver on osx due to flakiness

* [teemo] Fix incorrect include file read

* [osg] Fix dependency typo: glut -> freeglut

* [vcpkg] Recover some lost performance with the addition of vcpkg_get_tags.

A huge performance cost was loading the triplet files over and over; instead, we splice the sources into a macro and load it once, then just call that macro for each port.
Remove use of hashing because we aren't cross-process-safe anyway (global static will do instead).

* [vcpkg] Change Supports atom 'windows' to include UWP. Improve Supports field documentation.

* [vcpkg] Add docs for VCPKG_ENV_PASSTHROUGH and VCPKG_DEP_INFO_OVERRIDE_VARS

* Fix typo

Co-authored-by: Curtis J Bezault <curtbezault@gmail.com>
Co-authored-by: Victor Romero <romerosanchezv@gmail.com>
Co-authored-by: Robert Schumacher <roschuma@microsoft.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
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
Development

Successfully merging this pull request may close these issues.

7 participants