conda-forge / boost-feedstock

A conda-smithy repository for boost.
BSD 3-Clause "New" or "Revised" License
15 stars 41 forks source link

Add cobalt library #194

Closed sdebionne closed 5 months ago

sdebionne commented 6 months ago

Fixes #193

Checklist

conda-forge-webservices[bot] commented 6 months ago

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 commented 6 months ago

@conda-forge-admin, please rerender

jakirkham commented 6 months ago

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 commented 6 months ago

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 commented 6 months ago

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 commented 6 months ago

https://www.boost.org/doc/libs/master/libs/cobalt/doc/html/index.html#compiler

sdebionne commented 6 months ago

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 commented 6 months ago

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 https://github.com/conda-forge/conda-forge.github.io/issues/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 timeline[^1], we could move to VS2022 sometime this year.

[^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.

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.

h-vetinari commented 6 months ago

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 https://github.com/conda/conda/pull/11612 it seems. Not sure where this is coming from now, as the recipe hasn't changed?

sdebionne commented 6 months ago

The alternative is redoing https://github.com/conda-forge/conda-forge.github.io/issues/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 commented 6 months ago

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 commented 6 months ago

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 commented 6 months ago

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 commented 6 months ago

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 commented 6 months ago

@conda-forge-admin, please rerender

sdebionne commented 6 months ago

@conda-forge-admin, please rerender

sdebionne commented 6 months ago

For the records, the missing symbols on osx64:

Undefined symbols for architecture x86_64:
  "typeinfo for char8_t", referenced from:
      std::__1::basic_string<char8_t, std::__1::char_traits<char8_t>, std::__1::allocator<char8_t> > boost::locale::ios_info::string_set::get<char8_t>() const in formatter.o

And I just found out that there is a cxxstd feature in Boost.Build that should be used, rather than manually specifying the flag for each compilers.

h-vetinari commented 6 months ago

Try cherry-picking this commit for fixing the regex merge issue: https://github.com/conda-forge/boost-feedstock/pull/195/commits/914c72ffb3d9000c1c93975dac96cf4bb60e70b5

sdebionne commented 6 months ago

New file patching issue, see https://github.com/conda/conda-build/issues/4241#issuecomment-1690157067...

mbargull commented 6 months ago

Try cherry-picking this commit for fixing the regex merge issue: 914c72f

Oh, that one didn't have proper commit message since it was just for me testing stuff. If you want a proper commit message, you can add something like

Use same build string glob as python's run_exports

This is to avoid
  ValueError: Incompatible component merge:
    - '*_cpython'
    - '*cpython'
caused by https://github.com/conda/conda/issues/11442 .

. Additionally (which I didn't bother to fix while testing), the requirement constraints with the _DUMMY variables have to actually look like

- python {{ PY_DUMMY_VER }}.* *_cpython
- numpy {{ NP_DUMMY_VER }}.*

since with the added build string match component you get python 3.10 *_python which is equivalent to python 3.10.0 *_cpython, i.e., it will only choose 3.10.0 but not newer 3.10 patch releases.

mbargull commented 6 months ago

And N.B.: The ValueError: Incompatible component merge: has previously not been raised since conda-mambabuild didn't exert the MatchSpec component merging code path; i.e., this has been observed here because conda-build is now used.

sdebionne commented 6 months ago

Thanks for the explanations. I'll reword the commit when rebasing the branch to squash my fixups and fix the DUMMY part. The good news is that conda build is fixed... but uncover new issues:

fatal error: crypt.h: No such file or directory

include in Python.h (3.10). ~Wrong openssl version?~ Rather missing libcrypt

mbargull commented 6 months ago

fatal error: crypt.h: No such file or directory

Ha, this would probably be a 3.10.0 build it picked up :). If you add the .* then this should go away with the newer 3.10.* builds.

sdebionne commented 6 months ago

Indeed, the problematic platforms are aarchr64 and ppc64le and

host env:

python:                3.10.0-h2265c99_3_cpython      conda-forge

build env:

python:                                    3.10.13-hd12c33a_1_cpython conda-forge
sdebionne commented 6 months ago

@conda-forge-admin, please rerender

github-actions[bot] commented 6 months ago

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.

sdebionne commented 6 months ago

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

mbargull commented 6 months ago

@conda-forge-admin, please rerender

mbargull commented 6 months ago

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.

h-vetinari commented 6 months ago

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 commented 6 months ago

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 commented 6 months ago

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 commented 6 months ago

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

sdebionne commented 6 months ago

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 commented 6 months ago

Apparently not a good idea to have cppstd variants: https://github.com/conda-forge/abseil-cpp-feedstock/issues/45

h-vetinari commented 6 months ago

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 commented 6 months ago

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 commented 6 months ago

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

h-vetinari commented 6 months ago

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 https://github.com/conda-forge/abseil-cpp-feedstock/issues/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 commented 5 months ago

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