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

Add cobalt library #194

Merged
merged 12 commits into from
Mar 25, 2024
Merged

Conversation

sdebionne
Copy link
Contributor

@sdebionne sdebionne commented Mar 5, 2024

Fixes #193

Checklist

  • Used a personal fork of the feedstock to propose changes
  • Bumped the build number (if the version is unchanged)
  • Reset the build number to 0 (if the version changed)
  • Re-rendered with the latest conda-smithy (Use the phrase @conda-forge-admin, please rerender in a comment in this PR for automated rerendering)
  • Ensured the license file is being packaged.

@conda-forge-webservices
Copy link

Hi! This is the friendly automated conda-forge-linting service.

I just wanted to let you know that I linted all conda-recipes in your PR (recipe) and found it was in an excellent condition.

@sdebionne sdebionne force-pushed the add-cobalt-library branch from 0f08672 to c4e79a2 Compare March 5, 2024 17:55
@sdebionne
Copy link
Contributor Author

@conda-forge-admin, please rerender

@jakirkham
Copy link
Member

Thanks Samuel! 🙏

Are we missing any others or was this the only one?

Is there a good test we can add to verify that we have the expected components in install?

@sdebionne
Copy link
Contributor Author

The main issue is that Cobalt requires c++20 and a compiler with adequate support of coroutine: Clang 14, GCC 10 and MSVC 19.30 (Visual Studio 2022).

Are we missing any others or was this the only one?

The other one Boost.Redis is header only (or rather the user needs to include a src.hpp in one of its compilation unit)

Is there a good test we can add to verify that we have the expected components in install?

The typical file exist tests with libboost_cobal.so/dll should be good enough.

@h-vetinari
Copy link
Member

The main issue is that Cobalt requires c++20 and a compiler with adequate support of coroutine: Clang 14, GCC 10 and MSVC 19.30 (Visual Studio 2022).

We're already on Clang 16 + GCC 12 (and could use 17 resp. 13; soon also 18 resp. 14), so unix should be fine. I'm wondering about the MSVC requirement though; the last VS2019 releases essentially had 100% support for C++20, and was only kept from declaring /std:c++20 due to ABI changes in std::format. In particular, coroutines should be fully supported (modulo bugs) as of 19.28 (VS 16.8) - we're on VS 16.11 for the VS2019 line.

Moving to VS2022 is a bigger pain, and probably not yet in the cards for something as deep as boost, hence my interest in finding out where that MSVC requirement comes from.

@sdebionne
Copy link
Contributor Author

@sdebionne
Copy link
Contributor Author

The regressions tests are not setup for MSVC less than 2022.

I'll ask the author for details since 2019 is supposed to support coroutine.

There are other errors in the CI that are purely Conda issues...

@h-vetinari
Copy link
Member

h-vetinari commented Mar 5, 2024

OK, so no details on why requiring 19.30 vs. 19.29. Also very reassuring:

Some if not all MSVC versions have a broken coroutine implementation, that this library needs to workaround. This may cause non-deterministic behaviour and overhead.

Given that, I think the sanest approach then would be to have a separate output for libboost-cobalt, which we build with VS2022, while the rest of the libs are built with VS2019.

The alternative is redoing conda-forge/conda-forge.github.io#1732 for VS2022, or at least accepting that boost-lib-consuming feedstocks (and all projects linking to them) most likely have to move to VS2022. Both of these would require discussion in @conda-forge/core I think. OTOH, vs2019 will reach EOL this April, so if we assume a similar timeline1, we could move to VS2022 sometime this year.

Doing the separate output is a kind of stopgap measure, but at least it would enable that we build the library for anyone who needs it. If so, I'd propose to re-absorb it into libboost once we've moved to VS2022.

Footnotes

  1. for VS2019, it was driven by real-world requirements due to CI providers abandoning VS2017, and the ensuing bitrot resp. raised requirements in key projects.

@h-vetinari
Copy link
Member

I'll ask the author for details since 2019 is supposed to support coroutine.

Ah cool, thanks (your post arrived after I wrote mine)

There are other errors in the CI that are purely Conda issues...

Ugh, another instance of conda/conda#11612 it seems. Not sure where this is coming from now, as the recipe hasn't changed?

@sdebionne
Copy link
Contributor Author

sdebionne commented Mar 6, 2024

The alternative is redoing conda-forge/conda-forge.github.io#1732 for VS2022, or at least accepting that boost-lib-consuming feedstocks (and all projects linking to them) most likely have to move to VS2022.

I think Microsoft preserves ABI compatibility over versions since MSVC 2015 and that a library built with a given MSVC version can be consumed (e.g. linked) by a MSVC version that's the same or newer.

@h-vetinari
Copy link
Member

a library built with a given MSVC version can be consumed (e.g. linked) by a MSVC version that's the same or newer.

Yes, that's the point, because this creates a viral requirement to update to vs2022 for all boost consumers and all their transitive consumers. If we build only libboost-cobalt with vs2022, then the blast radius of that is much smaller.

@sdebionne
Copy link
Contributor Author

I spend some time compiling with MSVC 2019, and while the library itself compiles fine, one of the test fails to build, for a reason unrelated to coroutine.

So the CI should pass if we add cxxflags=/std:c++20 to the b2 command line (on Windows) and -std:c++20 for the Unix platforms.

@sdebionne
Copy link
Contributor Author

The win64 and linux64 builds now passed.

The osx build fails because conda-forge targets MacOS 10.9.

The other builds (ppc64le and aarch64) require the attention of the @conda-forge/core:

    ValueError: Incompatible component merge:
      - '*_cpython'
      - '*cpython'

@h-vetinari
Copy link
Member

The win64 and linux64 builds now passed.

Thanks, that's great!

The osx build fails because conda-forge targets MacOS 10.9.

I'd be okay with raising the MACOSX_DEPLOYMENT_TARGET to 10.13 here. Could you try if that makes the build pass? Needs to be set in CBC (for # [osx and x86_64]) and recipe rerendered.

The other builds (ppc64le and aarch64) require the attention of the @conda-forge/core:

As long as conda doesn't merge the PR I linked above, we'll have to figure out which feedstock sets the incompatible variant of that build constraint, and change that regex to match the predominant one...

@sdebionne
Copy link
Contributor Author

@conda-forge-admin, please rerender

@sdebionne sdebionne force-pushed the add-cobalt-library branch from c6d0faa to dba38f1 Compare March 8, 2024 10:02
@sdebionne
Copy link
Contributor Author

@conda-forge-admin, please rerender

Copy link
Contributor

Hi! This is the friendly automated conda-forge-webservice.

I tried to rerender for you, but it looks like there was nothing to do.

This message was generated by GitHub actions workflow run https://github.com/conda-forge/boost-feedstock/actions/runs/8235126304.

mbargull and others added 2 commits March 11, 2024 16:56
This is to avoid
  ValueError: Incompatible component merge:
    - '*_cpython'
    - '*cpython'
caused by conda/conda#11442 .

Signed-off-by: Marcel Bargull <marcel.bargull@udo.edu>
@sdebionne sdebionne force-pushed the add-cobalt-library branch from f8d31d4 to 5fcdcdc Compare March 11, 2024 15:56
@sdebionne
Copy link
Contributor Author

Why is {{ PY_DUMMY_VER }} not extended in the host section of the meta.yaml?

Otherwise conda-build hit a rendering issue (IDK why):
  conda.exceptions.InvalidVersionSpec: Invalid version '': empty version string

ref:
  conda-forge#194 (comment)

Signed-off-by: Marcel Bargull <marcel.bargull@udo.edu>
@mbargull
Copy link
Member

@conda-forge-admin, please rerender

@mbargull
Copy link
Member

Why is {{ PY_DUMMY_VER }} not extended in the host section of the meta.yaml?

No idea; pretty odd...
I've just pushed a (purportedly?) workaround.

conda-forge-webservices[bot] and others added 2 commits March 11, 2024 16:29
Signed-off-by: Marcel Bargull <marcel.bargull@udo.edu>
@h-vetinari
Copy link
Member

Why is {{ PY_DUMMY_VER }} not extended in the host section of the meta.yaml?

No idea; pretty odd...

The whole PY_DUMMY_VER thing is already a work-around... I'd like to just have - python 3.10 as a build requirement, but then smithy picks up a supposed python-dependence of libboost, and wants to build it for each python version (which ends up un-collapsing the CI jobs).

@h-vetinari
Copy link
Member

We still need

requirements:
  run:
    - __osx >={{ MACOSX_DEPLOYMENT_TARGET|default("10.9") }}  # [osx and x86_64]

here for any output that requires C symbols that aren't available in 10.9 - I didn't see the CI failures with 10.9, but it sounds we need it for libboost at least, and libboost-python gets it transitively; but it shouldn't be necessary for libboost-headers AFAICT.

@sdebionne
Copy link
Contributor Author

This last aa7b85c that changes the run requirements has an unexpected effect:

CXX_FOR_BUILD=$BUILD_PREFIX/bin/x86_64-conda-linux-gnu-clang++

but _build_env/bin/x86_64-conda-linux-gnu-clang++: No such file or directory.

I dont understand the causal link. It was building fine without it, so do you think it is really necessary?

@xhochy
Copy link
Member

xhochy commented Mar 12, 2024

I clicked on re-run. This was a faulty compiler package and should now be fixed.

@sdebionne
Copy link
Contributor Author

Thank you! Now that we have a Boost that compiles with c++20, shall we introduce variants for the different cppstd? And pin the ABI ?

Taken from Abseil feedstock:

    run_exports:
        string: cxx{{ cxx_standard }}_h{{ PKG_HASH }}_{{ PKG_BUILDNUM }}
        # also pin on ABI variant
        - libboost =*=cxx{{ cxx_standard }}*

@sdebionne
Copy link
Contributor Author

Apparently not a good idea to have cppstd variants: conda-forge/abseil-cpp-feedstock#45

@h-vetinari
Copy link
Member

It really depends on whether the library changes ABI based on the standard version. Basically no library does that (at least intentionally) as far as I'm aware, but it's abseil's entire raison d'être.

With boost being quite low-level as well, I'm not sure if they switch anywhere based on the availability of newer STL-types. If so, we should disable that (which is what we did for abseil >C++17 now; allowing compilation of our libabseil with C++20 and up without breaking the ABI).

@sdebionne
Copy link
Contributor Author

I asked on Slack whether libraries might change ABI based on the standard version such as libraries that would switch to using std counterpart of boost library when building with latest cpp standard for instance. Here are the answers I got:

So, I don't know of libraries that currently do this, but the proposed Parser does it

Usually we don't do this exactly because of these ABI problems and the amount of grief it causes to users and consequently to us when we try to investigate their issues

It is library dependent, hard to guess/check. I've never seen any rule or guideline for that either.

Yeah, there are no rules, you'd basically have to check on library basis. Some libraries' maintainers are extremely cautious about this sort of thing. Some are less so.

ABI compatibility across C++ standards versions is implementation defined. I would advise against mixing them without a thorough investigation of the target toolchain and environment.

@sdebionne
Copy link
Contributor Author

sdebionne commented Mar 14, 2024

Could you explain the meaning of the run_exports above an whether it is worth adding in our case?

@h-vetinari
Copy link
Member

Thanks for inquiring upstream!

Could you explain the meaning of the run_exports above an whether it is worth adding in our case?

I really doubt it's worth building separate variants of boost for different standard versions, already due to the overhead in and of itself, but more importantly to the problems in conda-forge/abseil-cpp-feedstock#45. In particular, introducing C++-std-flavours of boost would mean that we have to bifurcate the ecosystem into (say) a C++17 version and a C++20 version, and because there are two identical shared libraries with a (potentially!) different ABI, they cannot co-exist (without some namespacing shenanigans that need to be explicitly supported upstream for this to be feasible).

That means it would be impossible to mix packages built against libboost=*=cxx17* and libboost=*=cxx20*, which would force a full duplication of the build matrix of all boost-dependent feedstocks. And once someone starts using C++23, we'd have to triple it, etc.

I consider it infeasible to do this. Already with abseil (where at least technically, it could work due to the upstream namespacing), the appetite was not there to do it. And boost has even more consumers.

I think the only realistic scenario is to assume that the ABI does not change. This is - AFAICT - also what the boost packaging in conda-forge has done historically. Kind of like "if boost changes its ABI, and no-one's affected by it, does it really constitute a break?".

Abseil has a lot more experience with this and explicitly provides options.h to make these ABI-relevant choices at compile-time. Boost should really consider introducing a similar mechanism, if they plan on having divergent ABIs between standard versions.

In particular, I find

ABI compatibility across C++ standards versions is implementation defined.

a bit disingenuous. The entire C++ standardization process is completely dominated by ABI-compatibility, especially across C++ standard versions (c.f. the death of epochs), and all big three compilers de facto promise to keep the ABI in various ways. This might not be explicitly sanctioned by the standard (which also has no concept of its own evolution AFAIU, at least not normatively), but it is close enough to hard reality that it's effectively the bedrock underlying our ability to run a distribution (that includes lots of C++; and thus packages with very different standard requirements) at all.

CC @conda-forge/boost, if someone has thoughts on this.

@sdebionne
Copy link
Contributor Author

Sounds good to me. As there are no objections, shall we proceed with the merge?

Copy link
Member

@h-vetinari h-vetinari left a comment

Choose a reason for hiding this comment

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

OK, in it goes, thanks for the PR and discussion!

@h-vetinari h-vetinari merged commit a6e3e15 into conda-forge:main Mar 25, 2024
8 checks passed
@sdebionne sdebionne deleted the add-cobalt-library branch March 26, 2024 08:28
h-vetinari pushed a commit to regro-cf-autotick-bot/boost-feedstock that referenced this pull request May 6, 2024
Otherwise conda-build hit a rendering issue (IDK why):
  conda.exceptions.InvalidVersionSpec: Invalid version '': empty version string

ref:
  conda-forge#194 (comment)

Signed-off-by: Marcel Bargull <marcel.bargull@udo.edu>
h-vetinari pushed a commit to regro-cf-autotick-bot/boost-feedstock that referenced this pull request May 6, 2024
Otherwise conda-build hit a rendering issue (IDK why):
  conda.exceptions.InvalidVersionSpec: Invalid version '': empty version string

ref:
  conda-forge#194 (comment)

Signed-off-by: Marcel Bargull <marcel.bargull@udo.edu>
h-vetinari pushed a commit to regro-cf-autotick-bot/boost-feedstock that referenced this pull request May 6, 2024
Otherwise conda-build hit a rendering issue (IDK why):
  conda.exceptions.InvalidVersionSpec: Invalid version '': empty version string

ref:
  conda-forge#194 (comment)

Signed-off-by: Marcel Bargull <marcel.bargull@udo.edu>
h-vetinari pushed a commit to regro-cf-autotick-bot/boost-feedstock that referenced this pull request May 15, 2024
Otherwise conda-build hit a rendering issue (IDK why):
  conda.exceptions.InvalidVersionSpec: Invalid version '': empty version string

ref:
  conda-forge#194 (comment)

Signed-off-by: Marcel Bargull <marcel.bargull@udo.edu>
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.

Add missing new (built) cobalt library
5 participants