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

[curl] Add support of different features. #2862

Merged
merged 6 commits into from
Feb 27, 2018

Conversation

pravic
Copy link
Contributor

@pravic pravic commented Feb 22, 2018

This PR adds different features for the cURL library and, as an example, adds corresponding parts to the CPR library.

Default-Features: openssl, http2, ssh (as in previous builds)

Feature: curl
Description: Builds curl executable (placed in the /tools directory)

Feature: http-only
Description: Disables all protocols except HTTP/HTTPS/HTTP2

Feature: http2
Build-Depends: nghttp2, openssl
Description: HTTP2 support (requires openssl)

Feature: openssl
Build-Depends: openssl
Description: SSL support via OpenSSL

Feature: winssl
Description: SSL support via Schannel

Feature: ssh
Build-Depends: libssh2
Description: SSH support via libssh2

@pravic
Copy link
Contributor Author

pravic commented Feb 22, 2018

I have tested as many features as possible, however I found some dark corners of the Default-Features #2697 support.

For example,

Source: cpr
Build-Depends: curl[core]

Vcpkg still tries to install default features:

$ vcpkg install cpr --dry-run
curl[core,openssl,ssh,http2]:x86-windows

$ vcpkg install cpr[core] --dry-run
curl[core,openssl,ssh,http2]:x86-windows

The only way to avoid it is to install corresponding dependencies manually:

$ vcpkg install cpr[core] curl[core] --dry-run
curl[core]:x86-windows

However it still fails at the end:

Starting package 2/2: cpr:x86-windows
Building package cpr[core]:x86-windows...
The build command requires all dependencies to be already installed.
The following dependencies are missing:
curl[core]:x86-windows

So, I had to change Build-Depends: curl[core] to just Build-Depends: curl in CPR and, again, to install cURL manually: vcpkg install cpr[core] curl[core]. Now it works.

@pravic pravic changed the title [cpr] Add features of the curl library. [curl] Add support of different features. Feb 23, 2018
@pravic
Copy link
Contributor Author

pravic commented Feb 26, 2018

cc @ras0219-msft @alexkaratarakis

Could you guys have a look at this PR and give your opinion, please?

@ras0219-msft ras0219-msft self-assigned this Feb 26, 2018
@Squareys
Copy link
Contributor

Vcpkg still tries to install default features:
The only way to avoid it is to install corresponding dependencies manually:

Yes, that is intended, see discussion on #2697 :)
I am guessing the error message is the one you added with the diff in #2861 ? (Because it still says "build command", while this is the "install command").
Build-Depends: curl[core] should be valid and if specified explicitly and it doesn't work, it's a bug :/

@ras0219-msft
Copy link
Contributor

ras0219-msft commented Feb 26, 2018

Thanks for the PR!

I'll corroborate what @Squareys mentioned -- the "by design" behavior is that if you haven't explicitly told us that you know about the package (by pre-installing it or listing it on the command line), then we use the defaults. Note that if you currently have curl[core] installed, it should not install defaults (that would be a bug :)).

Starting package 2/2: cpr:x86-windows
Building package cpr[core]:x86-windows...
The build command requires all dependencies to be already installed.
The following dependencies are missing:
curl[core]:x86-windows

This is definitely a bug; you should never see "The build command requires all dependencies to be already installed." during an installation.

While trying to investigate that bug (I'm not able to repro it yet so further information would be awesome :)), I did find another serious bug in feature dependencies. If you had an installed package with a dependency like curl[core], the remove command did not consider that to be a dependency on curl so you could remove curl without removing cpr. I've pushed a fix to master for that, as well as rebased this PR onto that fix.

Finally, I pushed some changes (let me know if you think these are a problem):

  • I removed the transitive feature descriptions from cpr
    • Features aren't a good fit for this sort of "pass-through" because there isn't an observable difference between install curl[openssl] cpr[core] and install curl[openssl] cpr[openssl]. Instead, cpr should list cpr's capabilities and curl should list curl's capabilities :) If we want to have a command line experience like install cpr[curl:openssl] that does some amount of pass-through, then we can definitely discuss that separately
  • I inverted the logic of only-http to be non-http. Part of the design of features is that they must express positive relationships with code (i.e. adding a feature should add code, not remove it). Default features are a key part of making this work in the case of things like this -- non-http can be on by default, and removed to turn on the USE_HTTP_ONLY flag.
  • I renamed the "curl" feature to "tool" since it enables consistency between packages
  • Unfortunately, the winssl/openssl split is not expressed well with features. As mentioned above, Features are designed to map 1:1 with blocks of code/APIs/behavior. Turning on the feature turns on the APIs, turning off the feature removes those APIs. Turning on two features, should enable two sets of APIs. Put another way, it should always make sense to "turn on all the features!". In cases like this where it's two different backends for the same set of APIs, we'd ideally like to have one feature (say ssl) which represents the APIs that downstream code will consume with some other mechanism to choose the implementation. At the moment, the best mechanism is a setting in the triplet file, however that (unfortunately) isn't able to change the dependencies list. This is an area we need to improve upon, but it should work well enough for now.

Edit: I actually realized how it should be (w.r.t. the ssl feature) while writing this and had to go back and change my implementation :)

@Squareys
Copy link
Contributor

(
@ras0219-msft

Note that if you currently have curl[core] installed, it should not install defaults (that would be a bug :)).

Off the top of my head I cannot remember writing a test for that. I have a hunch that this may be an actual issue.
)

@ras0219-msft
Copy link
Contributor

@Squareys It worked that way for me already in my usages :) But I've added a test as well: 71d44ce

@pravic
Copy link
Contributor Author

pravic commented Feb 26, 2018

@ras0219-msft thanks for the great explanation and your effort to make this PR better.

However I can't go with a separate triplet since it will require to rebuild ALL my libraries and will break the vcpkg integration with projects. Essentially we went back again to the original status quo.

@ras0219-msft
Copy link
Contributor

ras0219-msft commented Feb 26, 2018

I found more bugs with feature packages (😦) but I've fixed all of them that I'm aware of (😄) so please try out c267f32 -- I'll rebase this PR onto that and push it here.

However I can't go with a separate triplet

Ideally, you can edit the existing triplets (x86-windows.cmake) locally to meet your needs. If that doesn't work well, we have several scenarios that need adjusting :)

Edit: rebased & pushed.

pravic and others added 6 commits February 26, 2018 14:50
Default-Features: openssl, http2, ssh (as in previous builds)

Feature: curl
Description: Builds curl executable (placed in the /tools directory)

Feature: http-only
Description: Disables all protocols except HTTP/HTTPS/HTTP2

Feature: http2
Build-Depends: nghttp2, openssl
Description: HTTP2 support (requires openssl)

Feature: openssl
Build-Depends: openssl
Description: SSL support via OpenSSL

Feature: winssl
Description: SSL support via Schannel

Feature: ssh
Build-Depends: libssh2
Description: SSH support via libssh2
@pravic
Copy link
Contributor Author

pravic commented Feb 27, 2018

Ideally, you can edit the existing triplets (x86-windows.cmake) locally to meet your needs.

And to force other people to do it? No, it does not make sense. Ideally, vcpkg install curl[winssl] would do the job.

Anyway, thanks.

@slodki
Copy link

slodki commented Nov 16, 2018

@ras0219-msft :

we'd ideally like to have one feature (say ssl) which represents the APIs that downstream code will consume with some other mechanism to choose the implementation. At the moment, the best mechanism is a setting in the triplet file, however that (unfortunately) isn't able to change the dependencies list. This is an area we need to improve upon, but it should work well enough for now.

Is there any progress with fixing broken openssl dependency with winssl selected?
OpenSSL is downloaded and build then not used at all.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants