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

[outcome] Replace Outcome single header based port with full fat cmake install port #15603

Merged
merged 26 commits into from Mar 30, 2021

Conversation

ned14
Copy link
Contributor

@ned14 ned14 commented Jan 12, 2021

  • What does your PR fix?

The last time vcpkg support for Outcome was attempted was in 2017, where a disagreement about how it ought to be implemented led to no support. This led to users submitting a single file include based edition of Outcome as a stand in. Given that four years have passed, and both vcpkg and Outcome have considerably matured and become far more popular with users, this PR tries again to make vcpkg's Outcome install based i.e. "full fat".

  • Which triplets are supported/not supported? Have you updated the CI baseline?

Every conceivable triplet should be supported. Outcome is very widely used in the C++ ecosystem across some very unusual platforms.

Almost. We do bundle an embedded library quickcpplib. This was already being silently bundled by the single file include edition, because the single file include contains quickcpplib, so in this sense nothing has changed. Quickcpplib is not intended for public usage outside this author's own libraries, and me not wanting it to be publicly usable was one of the primary reasons the 2017 submission of Outcome to vcpkg was not possible.

Related issues:

@JackBoosY JackBoosY self-assigned this Jan 12, 2021
@JackBoosY JackBoosY added the category:port-update The issue is with a library, which is requesting update new revision label Jan 12, 2021
@ned14
Copy link
Contributor Author

ned14 commented Jan 12, 2021

Now that all the CI tests have passed, @PhoebeHui @Pospelove both of you worked on bringing Outcome to vcpkg (thanks for that!). Can I ask that you review this PR, as obviously it'll be a breaking change for you, to make sure it's good?

@Pospelove
Copy link
Contributor

LGTM. Good job.

@JackBoosY JackBoosY linked an issue Jan 13, 2021 that may be closed by this pull request
@JackBoosY JackBoosY added the category:port-feature The issue is with a library, which is requesting new capabilities that didn’t exist label Jan 13, 2021
@JackBoosY JackBoosY changed the title Replace Outcome single header based port with full fat cmake install port [outcome] Replace Outcome single header based port with full fat cmake install port Jan 13, 2021
ports/outcome/portfile.cmake Outdated Show resolved Hide resolved
ports/outcome/portfile.cmake Outdated Show resolved Hide resolved
ports/outcome/portfile.cmake Outdated Show resolved Hide resolved
ports/outcome/portfile.cmake Outdated Show resolved Hide resolved
ports/outcome/portfile.cmake Outdated Show resolved Hide resolved
ports/outcome/portfile.cmake Outdated Show resolved Hide resolved
@JackBoosY
Copy link
Contributor

I will help you to improve these changes later.

@BurningEnlightenment
Copy link
Contributor

BurningEnlightenment commented Jan 14, 2021

I would love to see the ABI permutation disabled (either by default or by user request via library feature). Installing multiple library versions is prohibited under the vcpkg dependency model and so I think there is little (no?) gain from ABI permutation at the cost of more unreadable symbols. What do you think?

EDIT: For those unfamiliar with this library: The library namespace is suffixed with the git commit hash (e.g. outcome_v2_00000). So instead of spelling out the namespace symbol an indirection is used (the OUTCOME_V2_NAMESPACE macro). However, the library provides the OUTCOME_DISABLE_ABI_PERMUTATION option out of the box to disable suffixing (one should obviously still use the indirection macro).

@JackBoosY
Copy link
Contributor

I would love to see the ABI permutation disabled (either by default or by user request via library feature). Installing multiple library versions is prohibited under the vcpkg dependency model and so I think there is little (no?) gain from ABI permutation at the cost of more unreadable symbols. What do you think?

@BillyONeal what do you think about?

@JackBoosY
Copy link
Contributor

@ned14 Can you test if my changes is correct?

@ned14
Copy link
Contributor Author

ned14 commented Jan 14, 2021

I would love to see the ABI permutation disabled (either by default or by user request via library feature). Installing multiple library versions is prohibited under the vcpkg dependency model and so I think there is little (no?) gain from ABI permutation at the cost of more unreadable symbols. What do you think?

@BillyONeal what do you think about?

As current Outcome does not have a stable ABI until 2022, per-SHA ABI permutation is used to prevent collision between multiple copies of Outcome being brought into the same process. Some libraries may use Outcome from vcpkg, others may use Outcome standalone, still others may use a customised Outcome (there are quite a few forks going around because I refused certain changes, so various people stepped up with forks, and they haven't changed the namespace name).

In 2022 it is expected that we shall declare the ABI stable, and from thenceforth it shall be CI tested per commit to ensure it never, ever, changes again. At that point the ABI permutation will be dropped in the stable branch. If Outcome enters vcpkg, I should imagine that as part of the ordinary ports upgrade, vcpkg then gets the ABI stable Outcome.

Note that the ABI permute is per git SHA. As vcpkg chooses a specific git SHA, the ABI is not permuted for any given chosen git SHA.

@ned14
Copy link
Contributor Author

ned14 commented Jan 14, 2021

@ned14 Can you test if my changes is correct?

I've refactored your changes a bit further, see what you think. Some notes:

  • I think consolidating all the SHAs into a single file with instructions of how to upgrade them as a set is better UX for port maintainers.

  • usage appears to be ignored if the port is targets based. Right now it prints:

    The package outcome:x86-windows provides CMake targets:

    find_package(outcome CONFIG REQUIRED)
    target_link_libraries(main PRIVATE outcome::hl)

    find_package(quickcpplib CONFIG REQUIRED)
    target_link_libraries(main PRIVATE quickcpplib::hl)

This isn't useful, the latter find package is not necessary and is confusing, but usage does not appear to override.

  • I added vcpkg features so external users can choose for the internally chosen byte-lite and gsl-lite to be the vcpkg versions. I have no idea how to discover what to copy, and my attempts to get cmake to go search the vcpkg directories quickly became complex. Indeed, if we could even figure out the vcpkg port SHA, that could work too, and I didn't want cmake to go yanking port files from vcpkg's github and scanning the contents with a regex match. I don't think it wise to just use the vcpkg edition of those by default, in the past upstream changes have broken Outcome and it took some months to get upstream to change how they did things in a way that Outcome could return to trunk. Sticking with the versions published by Outcome's CI as "known good" is safer, in my opinion.

  • In other package managers they have the ability to run the test suite for a package in order to test if the chosen dependency versions from the package manager are compatible and non breaking. I don't see a facility for that here, but if there is one, I wouldn't object to Outcome using vcpkg's versions of the dependencies, if and only if the Outcome test suite passes with them.

@JackBoosY
Copy link
Contributor

@ned14

I think consolidating all the SHAs into a single file with instructions of how to upgrade them as a set is better UX for port maintainers.

The reason I separated these two dependencies into two files is to add them as separate ports in the future. So I think it is appropriate to put REF and HASH in the corresponding cmake file.

usage appears to be ignored if the port is targets based

Yes, I will take the usage file back.

I added vcpkg features so external users can choose for the internally chosen byte-lite and gsl-lite to be the vcpkg versions.

#11758 is adding the feature of the specified version of the dependency of the port. Once it is merged, we can specify the version of byte-lite and gsl-lite. I don't know whether this question can be solved completely.

In other package managers they have the ability to run the test suite for a package in order to test if the chosen dependency versions from the package manager are compatible and non breaking.

Yes, vcpkg currently lacks steps to test port validity and compatibility, but in the near future, I hope we can support them.
Each PR of the upgraded port version will test whether the port that depends on these ports is built correctly, if it fails, we will not merge into the master.

@ned14
Copy link
Contributor Author

ned14 commented Jan 15, 2021

The reason I separated these two dependencies into two files is to add them as separate ports in the future. So I think it is appropriate to put REF and HASH in the corresponding cmake file.

I'm not keen on exposing QuickCppLib as a public port. I don't want people to see it and have expectations that they can use it directly. This was one of the major reasons I would not support a vcpkg port of my libraries back in 2017, because it adds support costs to me. If that's vcpkg's intent, think the single header include based port, which you already have, is plenty sufficient as is.

I would support QuickCppLib as a vcpkg port if and only if:

  • Its port name is somehow adorned with "this is a port for internal use only". This could be as a simple as a prefix e.g. vcpkginternal_QuickCppLib.
  • If internal use only ports do not appear in the default list of ports (i.e. a flag must be passed to display all including internal ports).

Obviously all this is open source, so you guys can do what you want. However maintaining good relations between packagers and devs is important long term. I have good relations with most packagers of my libraries, I do my best to support them, it's a quid pro quo.

I added vcpkg features so external users can choose for the internally chosen byte-lite and gsl-lite to be the vcpkg versions.

#11758 is adding the feature of the specified version of the dependency of the port. Once it is merged, we can specify the version of byte-lite and gsl-lite. I don't know whether this question can be solved completely.

Versioning makes package management considerably more complex in general. People think they want versioning, and locally I suppose they do, but to be honest I don't think people generally appreciate the wider problems versioning brings into package management. Python's packaging story has become what it has due to versioning, and they're desperately trying to dig themselves out of that hole now.

For me personally, the whole point of bothering with package management is that for some given release X of all the packages, that every package within that release has been confirmed to work well when combined with every other package within that release. Ensuring that is a lot of tedious work, but it's most of the point of Debian or Fedora or any binary packages based Linux distro. I mean, anyone can go make their own Linux distro, but it's all that interoperability validation and testing is where the real value add occurs.

For me personally, same applies to a C++ package manager, and if a C++ package manager doesn't supply that, I don't see the point of buying into it for non-toy production software.

cpp-pm are currently transitioning to a matrix test configuration, so as each package get updated, they are tested by CI with all the other packages. If an upstream upgrades breaks something downstream, you now have to go fix downstream. This may kill off cpp-pm as they lack full time employees to go do all that donkey work (e.g. tons of stuff depends on OpenSSL, so an OpenSSL upgrade may break lots of other packages in tedious ways), but it's the right engineering choice, in my opinion.

In other package managers they have the ability to run the test suite for a package in order to test if the chosen dependency versions from the package manager are compatible and non breaking.

Yes, vcpkg currently lacks steps to test port validity and compatibility, but in the near future, I hope we can support them.
Each PR of the upgraded port version will test whether the port that depends on these ports is built correctly, if it fails, we will not merge into the master.

This isn't useful for header only libraries, obviously. Also, I find it frequent that my library source code compiles just fine after an upstream upgrade, but it's the test suite which fails because upstream broke some assumption in my code. I had a nasty recent experience with span<T> magically changing upstream, indeed I upcalled it to WG21 and it now looks they'll be undoing that change to span. Library code compiled fine, it was the test suite breaking that discovered the upstream change.

In other words, I really do think you need to support test suites, and I think you need to throw a ton of Azure pipelines at testing downstream dependencies every time something they depend upon gets PRed.

@JackBoosY
Copy link
Contributor

@ned14 So, I assume that the current changes are good, right?

@ned14
Copy link
Contributor Author

ned14 commented Jan 18, 2021

I've fixed the outcome package features so they actually work. If the current set of changes suit you, feel free to merge.

@JackBoosY
Copy link
Contributor

Already test all features succesfully on x86-windows and x64-windows-static.

@JackBoosY JackBoosY added info:reviewed Pull Request changes follow basic guidelines and removed requires:author-response labels Jan 19, 2021
@JackBoosY JackBoosY added info:reviewed Pull Request changes follow basic guidelines and removed info:reviewed Pull Request changes follow basic guidelines labels Jan 22, 2021
@ned14
Copy link
Contributor Author

ned14 commented Mar 2, 2021

I couldn't get UWP working, so I disabled it. All tests now pass.

@JackBoosY JackBoosY added info:reviewed Pull Request changes follow basic guidelines and removed depends:upstream-changes Waiting on a change to the upstream project requires:author-response labels Mar 3, 2021
@ned14
Copy link
Contributor Author

ned14 commented Mar 30, 2021

Any news on this? It's been a month now since last update.

@vicroms vicroms merged commit a434cc7 into microsoft:master Mar 30, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
category:port-feature The issue is with a library, which is requesting new capabilities that didn’t exist category:port-update The issue is with a library, which is requesting update new revision info:reviewed Pull Request changes follow basic guidelines
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Whether vcpkg will now accept LLFIO or not?
8 participants