Boost unification - Githubissues

h-vetinari commented 1 year ago

Implementation for https://github.com/conda-forge/boost-cpp-feedstock/issues/137

Closes #53 Closes https://github.com/conda-forge/boost-cpp-feedstock/issues/25

As discussed in that issue, this PR:

unifies the feedstocks for boost & boost-cpp in this feedstock (due to better name)
renames boost-cpp -> libboost & boost -> liboost-python (maintaining the old output names as wrappers for now)
adds a run-export to the new libboost (while boost-cpp stays unchanged without one)
introduces a header-only version libboost-headers that will fill the role of the run-export-less boost-cpp

The history of https://github.com/conda-forge/boost-cpp-feedstock is preserved through a merge (after some preparatory commits to reduce collisions). This makes the PR look unwieldy but the changes before the merge aren't very interesting. The upside is that this allows digging into the full archeology in one place (with the only snag that GH now resolves the PR links incorrectly).

Implementation notes:

libboost depends on libboost-headers, so the headers are really only packaged once
I tried but quickly gave up to build all three outputs at once; I believe this would be hard both because boost's b2 built tool doesn't seem to support only-build-don't-install so well, and more importantly, because we need to build libboost-python per python version.
Therefore, libboost & libboost-headers are built only once in build.sh / bld.bat (essentially the build scripts from boost-cpp), with the only substantial change that they're now "installed" into a temporary prefix from which we can then actually install the respective files into our $PREFIX per output
libboost-python essentially keeps the build scripts of boost, only that we now reuse the b2 that was bootstrapped during the global build phase (this now allows full cross-compilation; previously ppc was emulated here).
There's still substantial duplication between the (configuration parts of) build-py.{sh,bat} and the global build scripts, but for now I've tried to keep the changes to the respective histories to a minimum.
OTOH, the tests should be cleaned up nicely now.
Switched libboost-python to only package dynamic libs, and add a run-export. Also, while it previously depended on the full boost-cpp, the link-check says the libs aren't necessary, so switch to depend on libboost-headers instead.

PTAL @conda-forge/boost @conda-forge/boost-cpp CC @conda-forge/core

beckermr commented 1 year ago

I want to make sure I understand one item. The old boost-cpp has a constraint that libboost<0. The new boost-cpp has an exact pin. So the run constraint on the conda-forge libboost of boost-cpp at the same version will force loading the new boost-cpp. Am I following this correctly?

We may want to put a note in the recipe to always keep the exact pin. Otherwise I suppose we could pull in libboost from defaults in some conda configurations (though this would not be officially supported).

h-vetinari commented 1 year ago

The old boost-cpp has a constraint that libboost<0. The new boost-cpp has an exact pin.

Yes, otherwise it could not be a wrapper around libboost.

So the run constraint on the conda-forge libboost of boost-cpp at the same version will force loading the new boost-cpp. Am I following this correctly?

Yes, the constraint ensures that we can only co-install libboost & boost-cpp when both have equal version, and since our libboost that carries this constraint only exists since this PR (and boost-cpp wraps that), it remains impossible to co-install either one with old libboost from Anaconda.

The same sort of constraint is already working for similar (though less ambitious) renamings of abseil, arrow & grpc.

We may want to put a note in the recipe to always keep the exact pin. Otherwise I suppose we could pull in libboost from defaults in some conda configurations (though this would not be officially supported).

There is already a comment that explains this (happy to take suggestions for improvement...), though the tables are switched now -- since we now have the same output name as Anaconda's libboost, a whole lot of weird interactions aren't possible in the first place (because regardless of channels, each package can only be installed once).

beckermr commented 1 year ago

I see. I see the current comment but it doesn't explain in detail. It may be worth putting that explanation in a comment.

Thanks a bunch for the work here. Lgtm!

h-vetinari commented 1 year ago

Thanks a bunch for the work here. Lgtm!

Thanks! :)

To be clear, this is not in a rush and still needs a PR to feedstock-outputs, aside from discussing / fixing the "outstanding points" from the OP.

conda-forge-webservices[bot] commented 1 year 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.

h-vetinari commented 1 year ago

Regarding what I originally had as outstanding points in the OP:

not sure why boost packages both static & dynamic libs, but I didn't touch that part yet before I could ask for the "why".

Now switched to dynamic only, as is our default

boost previously depended on boost-cpp, but according to the link-check, the libraries aren't actually necessary; perhaps depending on libboost-headers would be enough?

Now switched to only depend on libboost-headers (and also added a run-export for the newly created libboost-python)

Windows CI seems to hang currently, will fix whatever issues persist after a restart

Fixed in https://github.com/conda-forge/boost-feedstock/pull/164/commits/3784106878cc21040996ea3cebb98f00c263718c

I regularly see warnings of the following kind, not sure how serious those are? Nor how to fix them...

SafetyError: The package for libboost located at /Users/runner/miniforge3/pkgs/libboost-1.82.0-h01504c6_2
appears to be corrupted. The path 'lib/cmake/boost_headers-1.82.0/boost_headers-config.cmake'
has an incorrect size.
  reported size: 1244 bytes
  actual size: 1778 bytes

Should be fixed by https://github.com/conda-forge/boost-feedstock/pull/164/commits/56e3fcd08bbee44b6e6e95735b76e6e1ed426785 (and/or making -python depend only on -headers), but in any case it doesn't appear in any of the logs anymore.

h-vetinari commented 1 year ago

I'm taking the hearts as indications that things are not looking too bad. ;-)

(@isuruf, of course I'm willing to address your feedback if you think something needs to be done; though I think leaving the -python builds more or less as they were before should be OK for a first step)

Opened PR for the remaining pre-requisite: https://github.com/conda-forge/feedstock-outputs/pull/47

isuruf commented 1 year ago

Let's give this some time. There's no particular hurry for this and I'll give some feedback later.

h-vetinari commented 1 year ago

Let's give this some time. There's no particular hurry for this and I'll give some feedback later.

In terms of timeline, I'd like to get the migrations for boost (and flang!) done before we have the Python 3.12 migration in ~October, to avoid having to deal with these big migrations getting mixed, which caused some heartache last time. Also we're kinda due a boost migration anyway, as we're on 1.78 now and 1.82 is ready (last increment was +4 in minor number as well after a bunch of discussion).

h-vetinari commented 1 year ago

In terms of timeline, I'd like to get the migrations for boost (and flang!) done before we have the Python 3.12 migration in ~October, to avoid having to deal with these big migrations getting mixed

Python 3.12b1 got released today. A migration is still a while away, but it'd be nice to leave a bit of buffer. Can we start wrapping up the review here?

isuruf commented 1 year ago

Let's merge this with a different label than main and work on it before releasing it into main.

h-vetinari commented 1 year ago

Let's merge this with a different label than main and work on it before releasing it into main.

OK. Could you write up a list which work is still necessary in your view? It should already be as capable as the current boost setup, though I guess we could add static outputs? I'm willing to do it, but I'd need to know what you think should be done.

h-vetinari commented 1 year ago

Note that this still requires https://github.com/conda-forge/feedstock-outputs/pull/47 before merging.

isuruf commented 1 year ago

How about the following?

boost-headers (only headers)
libboost (only shared libraries)
libboost-python (only shared libraries)
libboost-devel (boost-headers + libboost with run_exports + pkg-config & cmake files)
libboost-python-devel (boost-headers + libboost-python with run_exports + pkg-config & cmake files)

h-vetinari commented 1 year ago

How about the following?

I'm fine with that separation, though I'd have a preference to keep libboost-headers (rather than boost-headers), for consistency with the other libboost-* outputs.

The one thing that I'm not thrilled about is that it opens another can of worms, which is about naming consistency between the different variants foo-devel, foo-dev, foodev, etc. for spelling this concept. The former is very widespread in distros of course, so it seems like a natural choice, but I'd like us to decide on one way to do these, rather than have a hodgepodge of inconsistent namings and ways in which the files are sliced into outputs.

I also noticed that there's a deferred CFEP on this topic by @SylvainCorlay (which proposes -dev); I haven't read all the discussion in https://github.com/conda-forge/cfep/pull/39 in detail, but it looks like another big topic, and I doubt we'd come to a solution quickly (e.g. the new CUDA setup introduced two -dev feedstocks recently, we have llvmdev and clangdev that sit really low in our stack, etc.)

In short, I don't see why we have to attach this to the unification here. The same approach (e.g. refactor and migrate with renaming) could be used for a future boost-version when that discussion has taken place.

With your proposal we're IMO risking to solidify that divergence around -devel namings, or stalling this effort for months or longer. If people only want the renaming hassle once (or insist to do this concurrently for other reasons), I can live with moving forward with this PR+-devel (or -dev), though my preference would be to keep it separate.

h-vetinari commented 1 year ago

Another point in the -devel discussion is that distros usually also package the static libraries (boost in ubuntu is a sprawling set of outputs, but here's an example; here's another for a simpler lib). Aside from going against CFEP-18 (so it should be something exceptional), we generally don't want to have a run-export (like libboost-devel would have) for something that builds against the static libs. So I think it'd makes sense that feedstocks continue into opting into providing foo-static as a separate output (we have ~25 libs that do this already), rather than absorbing static libs into foo-devel.

This is not because I want to argue that point here, but to illustrate that "we should have a -devel package" is fraught with all the other questions about this topic this would open (or alternatively, risk further balkanizing the situation). And it's a question that should be considered, as the desire for static boost has been mentioned above already (xref #119 & #159)

It's also worth noting that any future renaming from the state of this PR to @isuruf's proposal would be mechanically very simple (i.e. libboost -> libboost-devel), rather than the more involved logic ("is it also in run or just in host?") that we need for https://github.com/regro/cf-scripts/pull/1668.

h-vetinari commented 1 year ago

From the discussion in the core call last week, the message was:

disruption only once
there must be a lib output without headers
people didn't feel enthusiastic about either -dev or -devel, the option was brought up to keep boost-cpp.

Based on these inputs, here's an updated proposal:

output	content	run-depends	run-exports
`libboost-headers`	headers	-	-
`libboost`	shared libraries	-	-
`libboost-python`	shared python libraries	-	-
`boost-cpp`	pkg-config & cmake files	`libboost-headers` `libboost`	`libboost`
`boost-python`	pkg-config & cmake files	`libboost-headers` `libboost-python`	`libboost-python`
`libboost-static`	static library plus metadata	`boost-cpp`
`libboost-python-static`	static library plus metadata	`boost-python`

Some observations:

Anaconda's libboost would then correspond to boost-cpp again (as previously); name would stay the same within conda-forge, but now have a run-export (so we're relying on the piggyback migrator to take care of the behaviour change)
libboost-python & boost-python fit consistently into the naming pattern, but matches neither existing boost nor Anaconda's py-boost
I have a preference for the headers to contain the "lib" prefix, but if someone strongly prefers boost-headers, I'm fine with that.
If we ever decide on something like CFEP-20, it would be easy to rename boost-cpp -> libboost-dev with no other changes in the packaging.

Aside from the impact on not achieving mostly unified naming with Anaconda (which is a somewhat foregone conclusion now that we need at least two extra outputs), is this something that people could agree on?

h-vetinari commented 1 year ago

Aside from the impact on not achieving mostly unified naming with Anaconda (which is a somewhat foregone conclusion now that we need an extra two outputs), is this something that people could agree on?

Gentle ping @isuruf @beckermr @mbargull @chenghlee @conda-forge/boost

(I've updated the table to show how the optional -static outputs would fit in).

h-vetinari commented 1 year ago

Boost 1.83beta just got released, so it looks like we can just move the setup with the new version. Still, could I please ask for feedback on the approach here?

h-vetinari commented 1 year ago

I'm running out of ways to ask for feedback here, which is quite frustrating. For those who don't want to write out a full response, please "emoji-react" to this post:

👍 The above plan, without libboost-{python-,}static
❤️ The above plan, with libboost-{python-,}static
🚀 Use @isuruf's suggestion verbatim
🎉 Use @isuruf's suggestion, but with -dev instead of -devel
😕 Revive CFEP-20
😄 Keep things as in the current state of this PR (no additional output without headers)
👀 Something else entirely

All but "😄" fail the "unify with Anaconda naming" goal, but I guess that's for Anaconda to deal with.

beckermr commented 1 year ago

Hey! I'm sorry I've been silent on this. In general I'd say that core members consult with one another and try to reach consensus. At some point though, the person doing the work needs to be unblocked. So IMHO you should feel free to simply proceed unless there are very strong objections. If someone has a strong objection, they should be willing to do the work in a timely manner.

I'm personally fine with any of the options and happy to help with bot work.

isuruf commented 1 year ago

At some point though, the person doing the work needs to be unblocked. So IMHO you should feel free to simply proceed unless there are very strong objections. If someone has a strong objection, they should be willing to do the work in a timely manner.

I highly disagree with this. That's disrespectful to the maintainers of this feedstock. If the maintainers have a strong objection, the PR author should address it. Not the other way around. Please don't set such disrespectful precedents.

beckermr commented 1 year ago

I think this is a simple miscommunication.

I said "unless there is a strong objection" or similar. People asking for things should be willing to pitch in, either by being willing to maintain the feedstock or by helping directly.

I am not saying you personally have done anything wrong at all, since you've clearly done tons of work and would help maintain.

h-vetinari commented 1 year ago

That's disrespectful to the maintainers of this feedstock. If the maintainers have a strong objection, the PR author should address it. Not the other way around. Please don't set such disrespectful precedents.

That is... really far off from what happened. 5 weeks ago this was again discussed in the core call with you present, and the plan I wrote up was based on your suggestion to keep using boost-cpp (after I raised a concern about -devel vis-à-vis CFEP-20).

What I find disrespectful is raising a blocker in a PR and then not responding anymore, for weeks or months (until something moves and you're back immediately to reiterate your objection). This is a situation that I find myself with you with some regularity[^1], and I honestly don't know what I've done to deserve that.

It's disheartening from start to finish.

[^1]: I could name 10 PRs off the top of my head, but my point is not to dig up old grievances, just to underscore that I'm not responding to a single incident.

jaimergp commented 1 year ago

I think the proposal so far has tried to accommodate every possible angle of feedback while pushing for the intended changes. I am fine with any of the first 4 options, but I must also say that this it outside of my comfort zone in terms of knowledge.

If there's no more feedback, what is it needed to unblock this contribution? If there's feedback, what is it needed to have a productive conversation that moves this forward instead of letting it stall?

isuruf commented 1 year ago

@h-vetinari, my comment was not towards you, but for the comment highlighted.

I apologize if that seemed disrespectful. We've discussed this multiple times in Github and in core meetings and it seems like these discussions with you never seem to end. I stop responding for my own sanity.

As I said in the core meeting, I would prefer option 3, but okay with option 1.

beckermr commented 1 year ago

OK thanks @isuruf! We have four core members who are happy with either 3 or 1 with one person having a preference for 3. @h-vetinari I think that is enough to go ahead with 3. However, you are doing the work so your judgement matters too.

h-vetinari commented 1 year ago

@isuruf: We've discussed this multiple times in Github and in core meetings

I've accommodated every feedback, the latest having just been "write up the plan we agreed to". Then crickets.

@isuruf: it seems like these discussions with you never seem to end. I stop responding for my own sanity.

Now imagine having both a blocker and a pertinent question about solving it (e.g. something unclear), and it becomes an bottomless pit to throw energy into. Try to ask for clarification again a few weeks later - still silence. Try to move forward somehow - up pops the blocker again, mostly with no further elaboration.

@beckermr: I think that is enough to go ahead with 3. However, you are doing the work so your judgement matters too.

Option 3. was proposed before three unanswered comments of mine, and until someone makes a convincing argument why it should be -devel, my point is that it increases balkanization of the dev / -dev / -devel space, rather than reduce it (e.g. all the recently added cuda libs use -dev; ref; for -devel we have some archived CDT's, perl, blas & mkl).

I'm fine with the equivalent(-up-to-naming) option 4. though; with preference to name it libboost-headers until someone tells me why it's beneficial to chop off the lib for the headers.

beckermr commented 1 year ago

Sure! Option 1 is on the table too right?

h-vetinari commented 1 year ago

Sure! Option 1 is on the table too right?

Yes, that's the closest to what was discussed in the core call. I actually don't like keeping the old names (even though it's pragmatic vis-à-vis CFEP-20), so I have a slight preference for 4.

beckermr commented 1 year ago

I think we shouldn't let the perfect be the enemy of the good. Whatever we do, the new system will be better than what we have now. I'm fine with 4 too.

h-vetinari commented 1 year ago

So boost 1.83 was just released, and we should use this new version to make a clean break (so not merge any migrator PRs for 1.83). I also just found out about the libboost-mpi feedstock (which no-one mentioned so far...), but at least that seems to fit into the naming scheme.

Concretely (following Option 4):

libboost{,-dev,-headers,-mpi,-python,-python-dev}  # {-static,-python-static} as possibile future extensions

I'll try to update the PR for that soon.

beckermr commented 1 year ago

I didn't even know we had that feedstock. 🤦‍♂️

h-vetinari commented 1 year ago

I looked at the content of libboost-mpi, and it intentionally does not contain the headers (they're deleted in the build script), only:

lib/cmake/boost_mpi-1.82.0/libboost_mpi-variant-shared.cmake
lib/libboost_mpi.so
lib/libboost_mpi.so.1.82.0

So that seems to fit well with the proposed setup here. @sdebionne please let us know if you think otherwise.

h-vetinari commented 1 year ago

Also, I made a sweep for other boost flavours still lurking, and found boost_mp11, which says:

    # Cannot be installed at the same time as boost-cpp as it clobbers the mp11 headers
    - boost-cpp <0a0

That would presumably become obsolete with the new libboost-headers, as boost_mp11 contains no libs; but then I don't understand why it needed to be a separate package... it has no users in conda-forge AFAICT. CC @conda-forge/boost_mp11

jakirkham commented 1 year ago

Would also be interested to hear from @jschueller and @xhochy who have been pretty active in both boost-cpp & boost

h-vetinari commented 1 year ago

@xhochy is aware of this PR. We've had irregular conversations (on other things) and I've invited him several times to chime in here over the last months. He's a busy man though.

xhochy commented 1 year ago

I've been on leave for four weeks and still catching up.I'll give this a re-read on Monday!

h-vetinari commented 1 year ago

OK, the refactor into libboost-dev etc. is now finally getting somewhere. Unix should be good, ~probably still overlooked something on windows somewhere.~ nope, it worked :)

I rebased out the separate label again, as people seem to be in agreement that this should be the actual design now.

Beyond that, I see two questions:

[ ] do we want to keep the compatibility outputs (boost-cpp & boost), or do we want a "clean cut"?
[ ] do we want to migrate for 1.82 or 1.83?

I also see something weird on all the builds that finished so far.

SafetyError: The package for libboost-dev located at /home/conda/feedstock_root/build_artifacts/pkg_cache/libboost-dev-1.82.0-h00ab1b0_2
appears to be corrupted. The path 'lib/cmake/boost_headers-1.82.0/boost_headers-config.cmake'
has an incorrect size.
  reported size: 1268 bytes
  actual size: 1778 bytes

h-vetinari commented 1 year ago

Aside from the points mentioned in the last comment, this should be good! The entire diff is probably a bit cumbersome to review, I think it might be easiest to go commit-by-commit after the merge of the different histories. Alternatively, here's a collected diff from the merge until the head of the branch.

beckermr commented 1 year ago

I think we should keep the compat outputs, but hint/lint and have a bot migrator.

beckermr commented 1 year ago

I'd prefer 1.82 over 1.83 to stick with every other but I don't have strong feelings.

h-vetinari commented 1 year ago

I think we should keep the compat outputs, but hint/lint and have a bot migrator.

The renaming migrator is essentially ready (modulo renames). And we recently made it easier to add such rules to the linter too, so no problem there.

xhochy commented 1 year ago

I also see something weird on all the builds that finished so far

This sounds to me that a later build step is overwritting the contents of this file. As installed files are hardlinked globally, this has an effect on all subsequent installs.

isuruf commented 1 year ago

Before merging, let's discuss option 3 vs 4 in the next meeting.

xhochy commented 1 year ago

Slight preference for -dev from my side as this is what I'm used from Debian-based systems.

h-vetinari commented 1 year ago

For me the point is about consistency. We have more (and more recent) packages with -dev than with -devel. I'd like us to become more consistent, not less, though which variant is less important to me.

The remaining slight preference for -dev is that the extra characters of -devel do not buy us any clarity IMO (I don't find parallels to other distros all that relevant, because our setup doesn't match in any case; e.g. we don't include static libs & don't have more granular boost libs).

Before merging, let's discuss option 3 vs 4 in the next meeting.

Could you write down your argument here, so we don't take too much of the scarce core call time with a pretty bikesheddy topic?

isuruf commented 1 year ago

All the packages in defaults are -devel. For eg: tbb-devel, mkl-devel, openblas-devel. I doubt defaults will go towards adding -dev simply because they only have -devel. If that's the case -dev will permanently diverge conda-forge from defaults. Correct me if I'm wrong @chenghlee.
CDTs all have -devel. This will be the case in the future as well if we stay with a centos based distro, but if we switch, this might change.
We already have -devel and -dev. It's better to pick one. (CUDA package naming have usually been done offline and it's hard to change them due to issues outside the control of c-f like nvidia legal. I would simply disregard the precedent set there as it's not done with community input)

h-vetinari commented 1 year ago

Thanks. That reasoning is fine by me. If the others agree, I can change to -devel directly.

h-vetinari commented 1 year ago

Given @xhochy's 👍, I've updated the PR.

conda-forge / boost-feedstock

Boost unification #164