microsoft
[outcome] Update to version 2.2.3
Describe the pull request
-
What does your PR fix?
Updates
outcometo version 2.2.3 and refactors out deprecated port behaviour like vendoringstatus-code. I had to patch the build system a bit and will submit those changes upstream. However, version 2.2.3 is obviously out the door and cannot profit from upstream changes anymore. Teaches theoutcomeport aboutquickcpplibspolyfill features. Update status-code to support vendoring (note that I couldn't update to any later version due to a breakingstatus-codeAPI change. -
Which triplets are supported/not supported? Have you updated the CI baseline?
unchanged
-
Does your PR follow the maintainer guide?
Yes
-
If you have added/updated a port: Have you run
./vcpkg x-add-version --alland committed the result?Yes
Already tested all features successfully on x86-windows and x64-windows-static.
@BillyONeal @ras0219-msft @JackBoosY did this PR slip under the radar or is there some issue I should address?
The polyfills are all part of quickcpplib which is an internal not to be directly used port. Which in turn is why I propagated the polyfill features to the two dependent ports llfio (in #25560) and outcome (this one).
This is kind of exactly why we created the policy prohibiting polyfills as features, with the only example we found being somewhat reasonable being abseil (since downstream customers just say absl::whatever and always get the std:: or polyfilled one without caring). Crucially, someone can do vcpkg install ned14-internal-quickcpplib[polyfill-cxx17] outcome[core] which would seemingly disagree with each other.
This is all fallout from the behavior that adding features to something is never considered 'bad' in that we give users the union of features in transitive dependencies.
Looking at what quickcpplib is doing, it seems messing with QUICKCPPLIB_REQUIRE_CXXNN just makes it install less stuff and imbue requirements into the CMake targets; it seems like we should just always install all the things and never imbue the targets which makes the feature unnecessary, unless I'm missing something?
Note that in a given link, it is almost never true that everything was compiled with the same standard version.
Tagging requires:vcpkg-team-review because I think we may want to discuss adding a specific triplet variable for this since it has come up but I remain weakly against adding a feature like this. That a feature is necessary in a downstream consumer for consistency says that the feature should never have been added to ned14-internal-quickcpplib in the first place.
Note that in a given link, it is almost never true that everything was compiled with the same standard version.
Yes and that easily leads to ODR violations (and lots of wasted time) if types are switched based on standard versions. Ironically I introduced the polyfill features because without them (the compiled part of) llfio becomes a prime candidate for this sort of thing due to quickcpplib switching on __cplusplus without this intervention (see below).
Crucially, someone can do
vcpkg install ned14-internal-quickcpplib[polyfill-cxx17] outcome[core]which would seemingly disagree with each other.
Which they shouldn't, because ned14-internal-quickcpplib is not meant to be referenced directly. However, if it were a normal port we wouldn't need the polyfill features for outcome and llfio, because they don't provide any polyfills themselves. Corollary to this, doing vcpkg install ned14-internal-quickcpplib[polyfill-cxx17] outcome[core] is not harmful, i.e. it doesn't cause ODR violations (even if outcome wouldn't consume the quickcpplib cmake config).
Looking at what quickcpplib is doing, it seems messing with QUICKCPPLIB_REQUIRE_CXXNN just makes it install less stuff and imbue requirements into the CMake targets; it seems like we should just always install all the things and never imbue the targets which makes the feature unnecessary, unless I'm missing something?
The polyfill configuration is burned into the source files here: https://github.com/microsoft/vcpkg/blob/030c53833b977f1580b2a4817bb22edbdde606d4/ports/ned14-internal-quickcpplib/portfile.cmake#L80-L92 and quickcpplib provides type aliases like abseil does (e.g. here).
That a feature is necessary in a downstream consumer for consistency says that the feature should never have been added to ned14-internal-quickcpplib in the first place.
To reiterate my previous point: It is not technically necessary. These features exist solely, because we don't want users to touch ned14-internal-quickcpplib directly. Ports depending on llfio or outcome but not quickcpplib wouldn't need to expose these features.
EDIT: Note that making quickcpplib an "internal not for user consumption" type port was a concession made in the initial PR by the vcpkg team (see https://github.com/microsoft/vcpkg/pull/15603#issuecomment-767942396).
@ras0219-msft, @dan-shaw, @JavierMatosD, @vicroms, and I discussed this in person today, and have the following to report:
We don't have solution for C++ standard version etc. right now. Ports need to tolerate that there is not a single value for this in a given link. We are spitballing 'best effort' features for bits like C++ standard, Windows SDK or target version, etc. that may help but aren't going to have something that lets this library do what it wants soon, for lots of reasons:
- Lots of ports are themselves opinionated about C++ standard version, and requiring global locks would make composition by and large impossible.
- Most ports support customers on C++ standard versions older than what they are built with.
- It isn't realistic to define a new part of the universe that must be held constant when there are already ~2000 libraries where it is not a constant.
Looking at these specific changes thing where features have to match between ports remains not OK. If the primary effect is to control polyfills in ned14-quicklib bits, then ned14-quicklib's value needs to be the value used here, and that needs to be the means of control, not separate other features elsewhere. Alternately, you could just pick one for the expression of these packages in vcpkg and just say that's what you support, in the same way that you would have to do deploying this to apt or whatever.
-> Additionally @ras0219-msft specifically thinks "entirely aliased" features could be OK, but then the "outer" port must not actually look at the feature (they must be allowed to be inconsistent in case the user uses 2 or more "outer" ports with inconsistent values etc.).
=================
Yes and that easily leads to ODR violations (and lots of wasted time) if types are switched based on standard versions.
The point is that libraries should not switch ODR sensitive things based on __cplusplus. In 99.9% of cases, it will not be a constant in an overall link and you can't expect it to be. (It's not even a constant that the same language is used for all things in a link, much less C++ standard version)
Which they shouldn't, because ned14-internal-quickcpplib is not meant to be referenced directly.
OK, but they can reference it directly. This concession:
EDIT: Note that making quickcpplib an "internal not for user consumption" type port was a concession made in the initial PR by the vcpkg team (see https://github.com/microsoft/vcpkg/pull/15603#issuecomment-767942396).
means 'we will put your helper thing in our index as long as you name it appropriately', not 'we will let you create semantic problems between other ports based on the assumption that that port is used in only these limited ways'. A not intended for user consumption port must still follow the rules of being a port, including constraints like "adding features must not break downstream ports/users". The polyfill exception we carved out for absl works because it is possible to write a large body of code that doesn't care about the polyfill setting; this change is trying require the user to care extremely deeply about the polyfill setting (by using a matching __cplusplus).
I think we are talking past each other. So I will try to outline how the features and polyfills interact and hopefully that will clear the confusion.
Vanilla quickcpplib (partially) selects polyfills based on __cplusplus (e.g. span.hpp:48) and exposes a type alias for it (e.g. span.hpp:56) in the QUICKCPPLIB namespace which is used "to write a large body of code that doesn't care about the polyfill setting". This first part of this is obviously bad, because that causes ODR violations if TUs which include quickcpplib are compiled with different standards.
In order to prevent this I reworked the quickcpplib port in #25560 to textually replace the polyfill selection preprocessor logic with #if 1/#if 0 based on which polyfill feature has been enabled (see quickcpplib/portfile.cmake:80-90). Because vcpkg features are additive in nature I decided to let them lower the required standard version. This means that if port foo requires quickcpplib[polyfill-cxx20] (or quickcpplib[core]) and bar requires quickcpplib[polyfill-cxx17] we will install quickcpplib[polyfill-cxx17,polyfill-cxx20]. In this mode quickcpplib would deploy all polyfills regardless of the standard version the TU is compiled with. I.e. foo would be using the polyfills even if its TUs were compiled against C++17 or C++20. To reiterate this: (Transitively) Dependent ports like outcome and llfio would still be perfectly fine if they required different polyfill levels (even if those disagreeing ports would be used/referenced by another port). So the example you gave previously:
Crucially, someone can do
vcpkg install ned14-internal-quickcpplib[polyfill-cxx17] outcome[core]which would seemingly disagree with each other.
It doesn't cause ODR violations nor does it prevent using outcome in a TU compiled against C++17 or C++20.
Additionally @ras0219-msft specifically thinks "entirely aliased" features could be OK, but then the "outer" port must not actually look at the feature (they must be allowed to be inconsistent in case the user uses 2 or more "outer" ports with inconsistent values etc.).
As explained above this is (and has been) the case with this port. To make this clear, I added a commit which removes the CMAKE_CXX_STANDARD configuration (which only applied to the unit tests of the outcome port anyway). If you want, I can also remove the polyfill feature forwarding to outcome by the llfio port as it isn't technically required; I just added it to make the common cases less confusing for the end user.
The point is that libraries should not switch ODR sensitive things based on
__cplusplus.
I fully agree with you. However, these are not my libraries. I just use and help maintaining them.
Because vcpkg features are additive in nature I decided to let them lower the required standard version.
👍
As explained above this is (and has been) the case with this port. To make this clear, I added a commit which removes the CMAKE_CXX_STANDARD configuration (which only applied to the unit tests of the outcome port anyway). If you want, I can also remove the polyfill feature forwarding to outcome by the llfio port as it isn't technically required; I just added it to make the common cases less confusing for the end user.
OK, it looked like it was only controlling the c++ standard version (and thus only __cplusplus), but you're saying it has nothing to do with that and these features are doing nothing but triggering corresponding features in ned14-internal-quickcpplib. In that case, I'm still not sure I'm in favor of creating such features but I don't think it's categorically wrong. Let me double check with some folks.
@JavierMatosD @ras0219-msft @dan-shaw @vicroms @markle11m and I discussed this today and ended with the following results:
We think that this feature-controlling-other-features arrangement is acceptable.
-
Needs to be relevant to consumers of the port people are expected to name.
-
The only things that should be exposed in this way should be 'durable' concerns about high level behavior. Libraries should NOT be re-exposing every feature of every transitive dependency they have. Example:
- asio[ssl] implying that its dependency curl will have some SSL backend is acceptable.
- asio should not be replicating all of curl's ssl backend selection stuff (schannel, openssl, wolfssl, etc.)
We know that this is still a case by case basis thing while we explore what we find acceptable here in the curated catalog.
