support-expressions: Add support expressions to features. Error when supports expression evaluates to false in classic and manifest mode. by autoantwort · Pull Request #184 · microsoft/vcpkg-tool

autoantwort · 2021-09-05T00:43:27Z

strega-nil-ms

So, I love this change; this would make my experience using vcpkg so much better.

However, I think there needs to be an escape hatch; some kind of "please ignore supports expressions for this install". Additionally, I think it's very likely that we should start by printing warnings instead of errors, since otherwise we will cause breakage on people installing over-constrained packages.

So far, though, this looks amazing. Thank you so much for your work here @autoantwort!

autoantwort · 2021-09-08T11:23:11Z

Additionally, I think it's very likely that we should start by printing warnings instead of errors, since otherwise we will cause breakage on people installing over-constrained packages.

I have checked some random ports, and the supports expression was almost always back upped by a vcpkg_fail_port_install(...) call in the portfile.cmake. So instead of only printing a warning I would prefer to fix the support expressions of the few ports where it is only used to disable the ci build.

But an additional option is needed to disable the failure.

autoantwort · 2021-09-13T18:27:46Z

I have added such an option now

strega-nil-ms

Looking good!

include/vcpkg/dependencies.h

src/vcpkg/commands.upgrade.cpp

src/vcpkg/install.cpp

BillyONeal · 2021-09-17T15:06:48Z

/azp run

azure-pipelines · 2021-09-17T15:06:56Z

Azure Pipelines successfully started running 1 pipeline(s).

src/vcpkg/dependencies.cpp

BillyONeal

Only extreme nits here.

strega-nil-ms

Oops, I misclicked...

…supports expression evaluates to false in classic and manifest mode.

…ages

Co-authored-by: nicole mazzuca <83086508+strega-nil-ms@users.noreply.github.com>

autoantwort · 2021-09-25T11:59:59Z

I assume a approval from @ras0219-msft is also needed?

strega-nil-ms · 2021-09-27T17:33:43Z

Given two yesses, merging now. Thanks @autoantwort :D

wrobelda · 2021-10-29T10:31:44Z

@autoantwort Is this actually supposed to work? Cause I added a supports element to a feature and am getting:

$ ./vcpkg x-add-version --overwrite-version --all 
Error: The port `libical` is not properly formatted.

While ./vcpkg format-manifest ports/port/vcpkg.json simply removes the supports line.

It gets more explicit when using main:

$ ./vcpkg format-manifest ports/port/vcpkg.json
Failed to parse manifest file: /Users/cromo/Documents/Sourcecode/vcpkg/ports/libical/vcpkg.json
Errors occurred while parsing /Users/cromo/Documents/Sourcecode/vcpkg/ports/libical/vcpkg.json
    $.features.rscale (a feature): unexpected field 'supports', did you mean 'description'?

autoantwort · 2021-10-29T12:01:48Z

Yes, but this tool version is currently not in the vcpkg repo, only after microsoft/vcpkg#20838

wrobelda · 2021-10-29T13:40:10Z

Yes, but this tool version is currently not in the vcpkg repo, only after microsoft/vcpkg#20838

I compiled vcpkg tool myself, using both main and 2021-10-18 tag, which is when this was released. What am I missing?

autoantwort · 2021-10-29T19:47:38Z

Maybe you are missing a git pull. I can add supports expressions to features and it is definitively working

autoantwort · 2021-10-29T19:49:06Z

While ./vcpkg format-manifest ports/port/vcpkg.json simply removes the supports line.

But this seems to be a bug

autoantwort · 2021-10-29T20:08:21Z

While ./vcpkg format-manifest ports/port/vcpkg.json simply removes the supports line.

But this seems to be a bug

Would be fixed by #240

JackBoosY · 2021-11-01T09:59:47Z

src/vcpkg/dependencies.cpp

+                        if (!supports_expression.get()->evaluate(
+                                m_var_provider.get_dep_info_vars(spec.spec()).value_or_exit(VCPKG_LINE_INFO)))
+                        {
+                            const auto msg = Strings::format("%s[%s] is only supported on '%s'",


I strongly against this changes!
Since our document does not specify whether to use a whitelist or a blacklist for the supports field, this modification will cause many ports to fail to build on the community triplet.
Please note that this field is only used for pipeline test to judge whether the result is reasonable.

Support expressions supports black and whitelists. For example !windows is a blacklist while osx & linux is a whitelist. But yeah we should suggest using blacklists to not fail on community triplets unless we know that a port really only works the given set of triplets.

Please note that this field is only used for pipeline test to judge whether the result is reasonable.

Until this PR. Now it can replaces vcpkg_fail_port_install calls to get a better user experience. You don't have to build all dependencies to see that the port does not support your triplet.
If a port is generally supported by a triplet but our ci don't want to build it, you should use ci.baseline.txt because the port in general supports the triplet.

Please note that this field is only used for pipeline test to judge whether the result is reasonable.

The intent is that ci.baseline.txt is the 'specific to our pipelines' thing, while supports reflects behavior from the upstream sources.

Therefore, I suggest changes this error to warning message unless we convert all whitelist to blacklist.

This also breaks option --only-downloads and --keep-going.

And this doesn't exists in the usage info.

And this doesn't exists in the usage info.

I don't understand that point.

I don't understand that point.

This option should be printed in the usage info if we need to add it:

PS F:\vcpkg> .\vcpkg.exe depend-info vtk[*] --allow-unsupported Unknown option(s) for command 'depend-info': '--allow-unsupported' Example: vcpkg depend-info sqlite3 Options: --dot Creates graph on basis of dot --dgml Creates graph on basis of dgml --show-depth Show recursion depth in output --max-recurse=... Set max recursion depth, a value of -1 indicates no limit --sort=... Set sort order for the list of dependencies, accepted values are: lexicographical, topological (default), reverse --triplet=<t> Specify the target architecture triplet. See 'vcpkg help triplet' (default: %VCPKG_DEFAULT_TRIPLET%) --host-triplet=<t> Specify the host architecture triplet. See 'vcpkg help triplet' (default: %VCPKG_DEFAULT_HOST_TRIPLET%) --overlay-ports=<path> Specify directories to be used when searching for ports (also: %VCPKG_OVERLAY_PORTS%) --overlay-triplets=<path> Specify directories containing triplets files (also: %VCPKG_OVERLAY_TRIPLETS%) --binarysource=<path> Add sources for binary caching. See 'vcpkg help binarycaching' --x-asset-sources=<path> Add sources for asset caching. See 'vcpkg help assetcaching' --downloads-root=<path> Specify the downloads root directory (default: %VCPKG_DOWNLOADS%) --vcpkg-root=<path> Specify the vcpkg root directory (default: %VCPKG_ROOT%) --x-buildtrees-root=<path> (Experimental) Specify the buildtrees root directory --x-install-root=<path> (Experimental) Specify the install root directory --x-packages-root=<path> (Experimental) Specify the packages root directory --x-json (Experimental) Request JSON output

Fix this in #300.

Another issue: microsoft/vcpkg#22278

BillyONeal · 2021-11-01T17:08:09Z

s/blacklist/denylist/g
s/whitelist/allowlist/g

wrobelda · 2021-11-17T13:52:33Z

@autoantwort I think there's some issue with it in CI/CD. Specifically, in microsoft/vcpkg#21491 I added:

"features": {
     "libmount": {
       "description": "Used by the UDisks backend on Linux",
       "supports": "linux",
       "dependencies": [
         "libmount"
       ]
     }
   }

However, it appears that because of it the Windows CI/CD now skips the kf5solid build altogether, apparently since libmount is only available on Linux:
https://dev.azure.com/vcpkg/public/_build/results?buildId=63246&view=logs&jobId=7922e5c4-0103-5f8f-ad17-45ce9bb98e80&j=7922e5c4-0103-5f8f-ad17-45ce9bb98e80&t=491b9f02-7edc-5990-cda1-511e95a3768e

dg0yt · 2021-11-17T14:12:41Z

No e2e test for this?

wrobelda · 2021-11-17T16:01:08Z

IMHO dependencies' features should be enabled only on systems that support it.

dg0yt · 2021-11-17T16:14:46Z

However, it appears that because of it the Windows CI/CD now skips the kf5solid build altogether, apparently since libmount is only available on Linux:

Note that the skip is not because libmount is part of the default features of kf5solid, but because libmount is part of the features requested by depending port kf5io, unconditionally.

ras0219-msft · 2021-11-17T16:21:50Z

Which means the immediate fix is to add a supports clause to kf5io; but this is definitely tricky to detect reliably and we need some better diagnostics.

autoantwort force-pushed the check-supports-expressions branch 6 times, most recently from cace755 to 617ae0e Compare September 7, 2021 12:05

strega-nil-ms suggested changes Sep 7, 2021

View reviewed changes

autoantwort mentioned this pull request Sep 8, 2021

many-ports: Update supports field microsoft/vcpkg#20053

Merged

strega-nil-ms suggested changes Sep 14, 2021

View reviewed changes

autoantwort force-pushed the check-supports-expressions branch from d2b65d8 to a2d563f Compare September 14, 2021 19:30

autoantwort mentioned this pull request Sep 15, 2021

[vcpkg] ci.baseline.txt updates for 2021-09-13 microsoft/vcpkg#20144

Merged

autoantwort force-pushed the check-supports-expressions branch from a2d563f to 3c31af7 Compare September 15, 2021 09:32

BillyONeal reviewed Sep 17, 2021

View reviewed changes

src/vcpkg/dependencies.cpp Outdated Show resolved Hide resolved

BillyONeal reviewed Sep 17, 2021

View reviewed changes

src/vcpkg/dependencies.cpp Outdated Show resolved Hide resolved

BillyONeal approved these changes Sep 17, 2021

View reviewed changes

strega-nil-ms suggested changes Sep 22, 2021

View reviewed changes

strega-nil-ms approved these changes Sep 22, 2021

View reviewed changes

autoantwort and others added 4 commits September 24, 2021 11:16

support-expressions: Add support expressions to features. Error when …

a0cbf5c

…supports expression evaluates to false in classic and manifest mode.

vcpkg install: Add argument to get only warnings for unsupported pack…

b5d2ba7

…ages

Apply suggestions from code review

2f4c83b

Co-authored-by: nicole mazzuca <83086508+strega-nil-ms@users.noreply.github.com>

Apply suggestions from Billy

f1812a8

autoantwort force-pushed the check-supports-expressions branch from 39fde2c to f1812a8 Compare September 24, 2021 09:16

strega-nil-ms merged commit baf0eec into microsoft:main Sep 27, 2021

autoantwort mentioned this pull request Oct 29, 2021

Add serialization of supports expressions in features #240

Merged

JackBoosY reviewed Nov 1, 2021

View reviewed changes

autoantwort deleted the check-supports-expressions branch November 7, 2021 19:43

JackBoosY mentioned this pull request Nov 17, 2021

[vcpkg_fail_port_install] Deprecate function microsoft/vcpkg#21489

Merged

wrobelda mentioned this pull request Nov 17, 2021

[libmount] New port microsoft/vcpkg#21491

Merged

JackBoosY mentioned this pull request Dec 13, 2021

[depend-info] Turn errors into warnings when querying dependencies of unsupported triplet #300

Merged

Conversation

autoantwort commented Sep 5, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

strega-nil-ms left a comment

Choose a reason for hiding this comment

Uh oh!

autoantwort commented Sep 8, 2021

Uh oh!

autoantwort commented Sep 13, 2021

Uh oh!

strega-nil-ms left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

BillyONeal commented Sep 17, 2021

Uh oh!

azure-pipelines bot commented Sep 17, 2021

Uh oh!

Uh oh!

Uh oh!

BillyONeal left a comment

Choose a reason for hiding this comment

Uh oh!

strega-nil-ms left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

autoantwort commented Sep 25, 2021

Uh oh!

strega-nil-ms commented Sep 27, 2021

Uh oh!

wrobelda commented Oct 29, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

autoantwort commented Oct 29, 2021

Uh oh!

wrobelda commented Oct 29, 2021

Uh oh!

autoantwort commented Oct 29, 2021

Uh oh!

autoantwort commented Oct 29, 2021

Uh oh!

autoantwort commented Oct 29, 2021

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

JackBoosY Nov 2, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

BillyONeal commented Nov 1, 2021

Uh oh!

wrobelda commented Nov 17, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

dg0yt commented Nov 17, 2021

Uh oh!

wrobelda commented Nov 17, 2021

autoantwort commented Sep 5, 2021 •

edited

Loading

strega-nil-ms left a comment •

edited

Loading

wrobelda commented Oct 29, 2021 •

edited

Loading

JackBoosY Nov 2, 2021 •

edited

Loading

wrobelda commented Nov 17, 2021 •

edited

Loading