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

1.10.0_1 build broke ABI w.r.t. to 1.10.0_0 builds #53

Closed
traversaro opened this issue Oct 25, 2022 · 22 comments
Closed

1.10.0_1 build broke ABI w.r.t. to 1.10.0_0 builds #53

traversaro opened this issue Oct 25, 2022 · 22 comments

Comments

@traversaro
Copy link

See discussion in #50 .

@bluescarni while we discuss how to properly do the de-vendoring, I think that we can mark these packages as broken?

@traversaro
Copy link
Author

traversaro commented Oct 25, 2022

@bluescarni while we discuss how to properly do the de-vendoring, I think that we can mark these packages as broken?

In case @conda-forge/spdlog agrees, the corresponding PR is ready to be merged at conda-forge/admin-requests#501 . Personally, I think the most important thing at the moment is fix the break that was induced by this change, then when everything is working we can think of how to properly de-vendor fmt.

@traversaro
Copy link
Author

Hi @cav71, I may be wrong but it seems that this is more a comment on the de-vendoring in general? In that case, perhaps it could make sense to have a dedicated issue for the de-vendoring of fmt?
I am asking as this issue is dedicated just to the ABI breakage of 1.10.0_1 , and I think we can close it once we solve it, i.e. for example by merging conda-forge/admin-requests#501 .

@cav71
Copy link
Contributor

cav71 commented Oct 25, 2022

My bad, sorry: I pasted it the text in the wrong thread.

@bluescarni
Copy link
Contributor

bluescarni commented Oct 25, 2022

@traversaro I guess this is the only viable solution at this time.

It really sucks because the status quo (before this breakage) is such that it is impossible to use the current conda pinned fmt version (9.x) together with spdlog in the same package (or within a dependency chain). I guess I'll have to go and migrate back a bunch of packages to fmt pinned to 8.x :(

@traversaro
Copy link
Author

Perhaps while we wait for an spdlog release we could think of doing a variant of this package with non-vendored fmt, but with a lower priority w.r.t. to the abi compatible vendored fmt spdlog . I would need to think about it, but perhaps we can find a solution before the new spdlog release.

@bluescarni
Copy link
Contributor

@traversaro if such a strategy worked it would be a life saver... anything I can do to help? It sounds a bit beyond my conda-fu...

@traversaro
Copy link
Author

@traversaro if such a strategy worked it would be a life saver... anything I can do to help? It sounds a bit beyond my conda-fu...

@bluescarni can you point me to one of the packages in which you want to install spdlog 1.10 and fmt 9.0 at the same time? So that I can try to get a clear idea on this.

@kkraus14
Copy link
Contributor

+1 to a variant with a track feature to weigh it down now and then consider moving to external fmt for 1.11 where we can more safely have an ABI break

@traversaro
Copy link
Author

+1 to a variant with a track feature to weigh it down now and then consider moving to external fmt for 1.11 where we can more safely have an ABI break

The only missing piece of the puzzle for me is: how does the downstream package explictly depend on the custom variant? It may be obvious but I am not enough experienced with that.

@bluescarni
Copy link
Contributor

Hi @traversaro thanks a lot for helping out...

So I have this package, heyoka, which depends on both spdlog and fmt:

https://github.com/conda-forge/heyoka-feedstock/blob/main/recipe/meta.yaml

As you can see from the recipe, I pinned spdlog to 1.10 and fmt to 8.1 in order to prevent ABI incompatibilities. So far so good.

The issue for the next version of heyoka if that one of its other dependencies, called mp++, in its latest version depends on fmt:

https://github.com/conda-forge/mppp-feedstock/blob/main/recipe/meta.yaml

The fmt version in this recipe is not pinned, so it currently depends on fmt 9.

This means that, as far as I have understood, it won't be possible to release the next version of heyoka until fmt is unbundled from spdlog, so that heyoka, spdlog and mp++ can all depend on the same version of fmt (i.e., the one globally pinned by conda-forge).

@kkraus14
Copy link
Contributor

The only missing piece of the puzzle for me is: how does the downstream package explictly depend on the custom variant? It may be obvious but I am not enough experienced with that.

I've seen two different approaches to it:

  1. You have a selector package, i.e. something like the arrow-cpp-proc package that has cpu and cuda variants defined via build strings. The arrow-cpp package then has a run_constrained entry on that package, i.e. https://github.com/conda-forge/arrow-cpp-feedstock/blob/main/recipe/meta.yaml#L117
  2. You just use build strings, i.e. here we could have bundledfmt and extfmt in the build string and a downstream package can then have a specification like spdlog=1.10=*extfmt*

@h-vetinari
Copy link
Member

Bear is an example that doesn't depend on fmt, but breaks.

@bluescarni
Copy link
Contributor

The only missing piece of the puzzle for me is: how does the downstream package explictly depend on the custom variant? It may be obvious but I am not enough experienced with that.

I've seen two different approaches to it:

1. You have a selector package, i.e. something like the `arrow-cpp-proc` package that has `cpu` and `cuda` variants defined via build strings. The `arrow-cpp` package then has a `run_constrained` entry on that package, i.e. https://github.com/conda-forge/arrow-cpp-feedstock/blob/main/recipe/meta.yaml#L117

2. You just use build strings, i.e. here we could have `bundledfmt` and `extfmt` in the build string and a downstream package can then have a specification like `spdlog=1.10=*extfmt*`

Would it make sense to create a temporary package called sdplog-unbundled (or something like that), to be removed/marked as broken after the new (hopefully unbundled) release of spdlog?

@h-vetinari
Copy link
Member

New release is not far out, which means we could then do a clean break and re-migrate (it would be more work if it's only a patch version increment, but still feasible with some repodata patches).

@h-vetinari
Copy link
Member

Heads up @conda-forge/spdlog

Upstream just tagged 1.11, and this repo is currently set to automerge. Since #50 was not reverted on main, that should be fine (we'll enter the brave new world as of 1.11), but JFYI

@traversaro
Copy link
Author

Heads up @conda-forge/spdlog

Upstream just tagged 1.11, and this repo is currently set to automerge. Since #50 was not reverted on main, that should be fine (we'll enter the brave new world as of 1.11), but JFYI

Perhaps we want to disable automerge until #51 is fixed? @bluescarni

@h-vetinari
Copy link
Member

I agree that the Debian patch would be good to have. The good thing is that we don't paint ourselves into a corner resolver-wise even if the automerge runs through. We can just add those patches afterwards, and it'll be pulled into the newer spdlog-as-runtime-dep.

@bluescarni
Copy link
Contributor

Perhaps we want to disable automerge until #51 is fixed? @bluescarni

How does one disable automerge?

@h-vetinari
Copy link
Member

How does one disable automerge?

Remove https://github.com/conda-forge/spdlog-feedstock/blob/main/conda-forge.yml#L1

@bluescarni
Copy link
Contributor

How does one disable automerge?

Remove https://github.com/conda-forge/spdlog-feedstock/blob/main/conda-forge.yml#L1

Cheers, should be done now.

rapids-bot bot pushed a commit to rapidsai/rmm that referenced this issue Dec 6, 2022
This reverts commit 7bf7a76.

cc: @kkraus14 There's a handful of issues this caused downstream. We'll be sorting through these and fixing them but need to quickly revert this for now.

Currrent problems:

1. rmm's meta.yaml doesn't correctly specify spdlog as a build dep. It's [incorrectly listed as a runtime dep](https://github.com/rapidsai/rmm/blob/7bf7a763a3b6a32a7a57f900de35e2e5f30f17c0/conda/recipes/librmm/meta.yaml#L56). This means that rmm [pulls spdlog via CPM with rapids-cmake](https://github.com/rapidsai/rmm/actions/runs/3595940512/jobs/6056061203#step:6:595) `-- CPM: adding package spdlog@1.8.5 (v1.8.5)` and then clobbers its own installed spdlog headers:

```
2022-12-01T19:46:46.2167513Z ClobberWarning: This transaction has incompatible packages due to a shared path.
2022-12-01T19:46:46.2168766Z   packages: conda-forge/linux-64::spdlog-1.10.0-h924138e_0, file:///tmp/conda-bld-output/linux-64::librmm-23.02.00a-cuda11_gc328a18d_11
2022-12-01T19:46:46.2169686Z   path: 'include/spdlog/async.h'
```

2. spdlog 1.10.0 doesn't work with nvcc. We need spdlog 1.11.0 to get fmt 9.1.0, which contains [a necessary fix](fmtlib/fmt#2971). However, there's some uncertainty about how to handle the spdlog conda-forge package which [no longer vendors fmt](conda-forge/spdlog-feedstock#50) while rapids-cmake is still using the upstream repo which continues to vendor fmt. Does this inconsistency matter, as long as we pin `fmt=9.1.0` in the conda package cases? @kkraus14 It looks like you weighed in on this matter [here](conda-forge/spdlog-feedstock#53) and [here](conda-forge/conda-forge-pinning-feedstock#3654). If you have insights that can help us with this quandary, please share!

Authors:
  - Bradley Dice (https://github.com/bdice)

Approvers:
  - Mark Harris (https://github.com/harrism)
  - Ray Douglass (https://github.com/raydouglass)

URL: #1176
@kkraus14
Copy link
Contributor

kkraus14 commented Oct 7, 2023

This is now resolved

@kkraus14 kkraus14 closed this as completed Oct 7, 2023
@cav71
Copy link
Contributor

cav71 commented Oct 7, 2023

Thanks!

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

No branches or pull requests

5 participants