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

[RFC] Options 🎉 #22216

Draft
wants to merge 5 commits into
base: master
Choose a base branch
from
Draft

Conversation

autoantwort
Copy link
Contributor

A RFC for options. Not final. Feel free to comment. I reused some sentences from https://github.com/edhebi/vcpkg/blob/master/docs/specifications/options.md. Thank you @edhebi for writing the initial RFC.

@JackBoosY JackBoosY added category:documentation To resolve the issue, documentation will need to be updated requires:vcpkg-team-review This PR or issue requires someone on the vcpkg team to take a further look. labels Dec 27, 2021
@JackBoosY JackBoosY changed the title Options RFC 🎉 [documentation] Options RFC 🎉 Dec 27, 2021
Comment on lines +137 to +140
1. `vcpkg install foobar[foo=dor]`
2. `vcpkg install foobar[foo=bar]`
3. `vcpkg install foobar[bar=QString]`
4. Install some dependency that needs `foobar[foo=dor]`
Copy link
Contributor

Choose a reason for hiding this comment

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

the same as selecting qtbase[core,gui] vs qtbase[core]. A rebuild if the selection cannot be fulfilled.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah, but features are additional.
If I run vcpkg install "qtbase[core,doubleconversion,zstd]"
and then vcpkg install "qtbase[core,doubleconversion]" qtbase[core,doubleconversion,zlib] and not qtbase[core,doubleconversion] is installed. With options you would "remove" the previously installed stuff because things are not additive.

Comment on lines 123 to 124
"options": {
"ssl-lib": "boringssl"
Copy link
Contributor

Choose a reason for hiding this comment

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

due to the fact that options are not additive they are not allowed to be specified in the manifest itself.
Otherwise conflicts within the tree might arise making stuff unbuildable.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Huh. Then you can't use options in manifest mode?

Copy link
Contributor

Choose a reason for hiding this comment

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

Then you can't use options in manifest mode?

to be more precise: you don't specify dependent options in the manifest.
Think about it: if the ssl/lapack or the qt version are hardcoded into the manifest of one port which sense does it make to offer the option? Furthermore if ports have conflicting hardcoding options those won't work together. There is a reason why features need to be additive.
Maybe there needs to be a distinction between manifest in the port folder and a manifest in the build folder. Or there needs to be a variable similar to VCPKG_MANIFEST_FEATURES just for the options of every port (VCPKG_<PORT>_MANIFEST_FEATURES ?).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Think about it: if the ssl/lapack or the qt version are hardcoded into the manifest of one port which sense does it make to offer the option?

If I have a port A with an option for Qt5/6 and a port B that depends on A[qt=qt5] because B only work with Qt5 I still can use port A with Qt6 (I don't care about port B). Currently A must stick at Qt5 or we remove port B.

Furthermore if ports have conflicting hardcoding options those won't work together.

Yes, I would accept this. We already have this situation with boringssl vs openssl. You can't install them at the same time.

Maybe there needs to be a distinction between manifest in the port folder and a manifest in the build folder.

One could think about it to only allow this at "end users project" manifest files and not in port manifest files. But I would vote against it and allow it everywhere (I know that you can then create uninstallable trees).

@JackBoosY
Copy link
Contributor

cc @strega-nil-ms for review this PR.

@strega-nil-ms strega-nil-ms changed the title [documentation] Options RFC 🎉 [RFC] Options 🎉 Jan 11, 2022
@strega-nil-ms
Copy link
Contributor

So, some thoughts from the team (I should've posted this last week and forgot):

This is our first RFC from a member of the community! (excited!) That's really cool, and I'm super excited about it.

There are some considerations we have to make, though. This is a Microsoft product and we need to be able to support any features that gets added. This means justifying to management that this is worth spending developer time on. "A community member showed up and wrote an RFC" is definitely a large piece of evidence for that the feature is worth it, but it still needs to go through internal review and the like.

What we think is the right way forward is first, to get some preliminary review from the vcpkg team; then, if you're interested, we'd love an experimental implementation, behind a feature flag, in vcpkg-tool. We can go from there.

I'll be championing this feature internally, so we should probably open communications on discord or something so we can talk about it, if you like.

Thanks so, so much!

@JackBoosY JackBoosY removed the requires:vcpkg-team-review This PR or issue requires someone on the vcpkg team to take a further look. label Mar 18, 2022
@Neumann-A
Copy link
Contributor

@autoantwort Do you already have an implementation in vcpkg-tool for this ?

@strega-nil-ms what is the progress?

@BillyONeal
Copy link
Member

@strega-nil-ms what is the progress?

https://github.com/microsoft/vcpkg/wiki/Roadmap <-- Our PM team just updated the roadmap here and it doesn't look like we'll be getting to this in the next... while. It's still something we want and I expect it to be a high priority next time additional capabilities for ports are under consideration.

@PhoebeHui PhoebeHui marked this pull request as draft June 7, 2022 00:40
@JackBoosY JackBoosY assigned Cheney-W and unassigned JackBoosY Oct 14, 2022
@vicroms
Copy link
Member

vicroms commented Oct 20, 2022

@autoantwort
Copy link
Contributor Author

@autoantwort Do you already have an implementation in vcpkg-tool for this ?

I am considering making one. Shouldn't be too hard™️

For this reason, ports should only require a value for an option of one of their dependencies if it is really necessary. One use case can be that a port can have an Qt5 or Qt6 compatible API interface, so it can depend on Qt5 or Qt6. The end user can then select between the used Qt version. But if we now have a Qt5 or Qt6 only port, this port can require the value Qt5 or Qt6 from the dependency.

## Possible extension: Options in features
**Disclaimer**: Since no use case is currently known, it will not be implemented for the time being.
Copy link
Contributor

Choose a reason for hiding this comment

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

I think this is the use case: #37450
I.e. need something like:

{
  "name": "curl",
  "features": {
    "http3": {
      "description": "HTTP3 support",
      "options": {
        "backend": { 
          "choices": [
            {
              "name": "ngtcp2",
              "dependencies": [,
                "nghttp3",
                "ngtcp2",
                "probably requires nested options to choose between quictls/gnutls/wolfssl?"
              ]
            },
            {
              "name": "openssl",
              "dependencies": [
                "nghttp3",
                {
                  "name": "curl",
                  "options": {
                    {
                      "name": "ssl-backends",
                      "value": "openssl",
                      "exclusive": true
                    }
                  }
                },
                {
                  "name": "openssl",
                  "version>=": "3.2.0"
                }
              ]
            }
          ],
          "default": "ngtcp2"
        }
      }      
    }
  }
}

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
category:documentation To resolve the issue, documentation will need to be updated
Projects
None yet
Development

Successfully merging this pull request may close these issues.

9 participants