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

tighten fmt in libmamba #628

Merged
merged 1 commit into from
Jan 3, 2024
Merged

Conversation

jaimergp
Copy link
Member

@jaimergp jaimergp commented Jan 2, 2024

Checklist

  • Used a static YAML file for the patch if possible (instructions).
  • Only wrote code directly into generate_patch_json.py if absolutely necessary.
  • Ran pre-commit run -a and ensured all files pass the linting checks.
  • Ran python show_diff.py and posted the output as part of the PR.
  • Modifications won't affect packages built in the future.

fmt 10.2.0 breaks libmamba on Windows: conda-forge/fmt-feedstock#39

@jaimergp jaimergp requested a review from a team as a code owner January 2, 2024 18:03
@conda-forge-webservices
Copy link
Contributor

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.

@jaimergp
Copy link
Member Author

jaimergp commented Jan 2, 2024

@conda-forge/mamba @conda-forge/fmt

@saraedum
Copy link
Member

saraedum commented Jan 2, 2024

Sorry for breaking libmamba by merging that 10.2 release. Please merge this fix :)

Is this in any way libmamba specific or is fmt 10.2 just a breaking ABI change? @jaimergp
If so, we probably need to update the run-exports of fmt (and also make that change to the previous 10.x builds…)

@bdice
Copy link
Contributor

bdice commented Jan 2, 2024

I agree with all @saraedum said above. This PR seems like the right path forward.

@h-vetinari
Copy link
Member

recipe>python show_diff.py
================================================================================
================================================================================
linux-armv7l
================================================================================
================================================================================
win-32
================================================================================
================================================================================
osx-arm64
osx-arm64::libmamba-1.5.6-h90c426b_0.conda
osx-arm64::libmamba-1.5.0-h6089622_1.conda
osx-arm64::libmamba-1.5.4-h0a6dc31_0.conda
osx-arm64::libmamba-1.5.1-h0a6dc31_2.conda
osx-arm64::libmamba-1.5.3-h0a6dc31_1.conda
osx-arm64::libmamba-1.5.1-hace0f30_1.conda
osx-arm64::libmamba-1.5.2-h0a6dc31_1.conda
osx-arm64::libmamba-1.5.2-h0a6dc31_0.conda
osx-arm64::libmamba-1.5.1-h6089622_0.conda
osx-arm64::libmamba-1.5.5-h0a6dc31_0.conda
osx-arm64::libmamba-1.5.3-h0a6dc31_2.conda
-    "fmt >=10.1.1,<11.0a0",
+    "fmt >=10.1.1,<10.2.0a0",
================================================================================
================================================================================
linux-ppc64le
linux-ppc64le::libmamba-1.5.1-h5f46139_2.conda
linux-ppc64le::libmamba-1.5.3-h5f46139_2.conda
linux-ppc64le::libmamba-1.5.6-h5f46139_0.conda
linux-ppc64le::libmamba-1.5.3-h5f46139_1.conda
linux-ppc64le::libmamba-1.5.0-hcbafca5_1.conda
linux-ppc64le::libmamba-1.5.2-h5f46139_1.conda
linux-ppc64le::libmamba-1.5.4-h5f46139_0.conda
linux-ppc64le::libmamba-1.5.5-h5f46139_0.conda
linux-ppc64le::libmamba-1.5.1-h2246aa7_1.conda
linux-ppc64le::libmamba-1.5.1-hcbafca5_0.conda
linux-ppc64le::libmamba-1.5.2-h5f46139_0.conda
-    "fmt >=10.1.1,<11.0a0",
+    "fmt >=10.1.1,<10.2.0a0",
================================================================================
================================================================================
linux-aarch64
linux-aarch64::libmamba-1.5.2-hea3be6c_1.conda
linux-aarch64::libmamba-1.5.1-hea3be6c_2.conda
linux-aarch64::libmamba-1.5.1-h5d583b4_1.conda
linux-aarch64::libmamba-1.5.3-hea3be6c_2.conda
linux-aarch64::libmamba-1.5.0-h604a1c0_1.conda
linux-aarch64::libmamba-1.5.2-hea3be6c_0.conda
linux-aarch64::libmamba-1.5.1-h604a1c0_0.conda
linux-aarch64::libmamba-1.5.6-hea3be6c_0.conda
linux-aarch64::libmamba-1.5.4-hea3be6c_0.conda
linux-aarch64::libmamba-1.5.3-hea3be6c_1.conda
linux-aarch64::libmamba-1.5.5-hea3be6c_0.conda
-    "fmt >=10.1.1,<11.0a0",
+    "fmt >=10.1.1,<10.2.0a0",
================================================================================
================================================================================
noarch
================================================================================
================================================================================
win-64
win-64::libmamba-1.5.1-h925f7fc_1.conda
win-64::libmamba-1.5.0-h16d0179_1.conda
win-64::libmamba-1.5.2-h3f09ed1_0.conda
win-64::libmamba-1.5.1-h3f09ed1_2.conda
win-64::libmamba-1.5.3-h3f09ed1_1.conda
win-64::libmamba-1.5.3-h3f09ed1_2.conda
win-64::libmamba-1.5.6-h3f09ed1_0.conda
win-64::libmamba-1.5.1-h16d0179_0.conda
win-64::libmamba-1.5.4-h3f09ed1_0.conda
win-64::libmamba-1.5.2-h3f09ed1_1.conda
win-64::libmamba-1.5.5-h3f09ed1_0.conda
-    "fmt >=10.1.1,<11.0a0",
+    "fmt >=10.1.1,<10.2.0a0",
================================================================================
================================================================================
osx-64
osx-64::libmamba-1.5.5-hb976dfd_0.conda
osx-64::libmamba-1.5.3-hb976dfd_1.conda
osx-64::libmamba-1.5.6-ha449628_0.conda
osx-64::libmamba-1.5.2-hb976dfd_1.conda
osx-64::libmamba-1.5.1-h08141de_1.conda
osx-64::libmamba-1.5.3-hb976dfd_2.conda
osx-64::libmamba-1.5.0-hb45792e_1.conda
osx-64::libmamba-1.5.2-hb976dfd_0.conda
osx-64::libmamba-1.5.1-hb45792e_0.conda
osx-64::libmamba-1.5.4-hb976dfd_0.conda
osx-64::libmamba-1.5.1-hb976dfd_2.conda
-    "fmt >=10.1.1,<11.0a0",
+    "fmt >=10.1.1,<10.2.0a0",
================================================================================
================================================================================
linux-64
linux-64::libmamba-1.5.0-h744094f_1.conda
linux-64::libmamba-1.5.3-had39da4_2.conda
linux-64::libmamba-1.5.1-hfcfce90_1.conda
linux-64::libmamba-1.5.6-had39da4_0.conda
linux-64::libmamba-1.5.5-had39da4_0.conda
linux-64::libmamba-1.5.1-h744094f_0.conda
linux-64::libmamba-1.5.3-had39da4_1.conda
linux-64::libmamba-1.5.2-had39da4_1.conda
linux-64::libmamba-1.5.4-had39da4_0.conda
linux-64::libmamba-1.5.2-had39da4_0.conda
linux-64::libmamba-1.5.1-had39da4_2.conda
-    "fmt >=10.1.1,<11.0a0",
+    "fmt >=10.1.1,<10.2.0a0",

@h-vetinari h-vetinari merged commit ae3d41a into conda-forge:main Jan 3, 2024
4 checks passed
@h-vetinari
Copy link
Member

I merged this to stop the bleeding, but we still have a bunch of work to do (short of marking 10.2 as broken and not releasing it again). Everything built against 10.1 needs to get the run-export tightened (incl fmt 10.1 itself, as well as the migrator).

@bdice
Copy link
Contributor

bdice commented Jan 3, 2024

Thanks so much @h-vetinari. Would this plan of action agree with your expectations? Am I missing any steps?

  1. Release a new build of fmt 10.2 with a tighter run-export
  2. Backport the tighter run-export to fmt 10.1 (and possibly other versions)
  3. Mark older fmt packages without the tighter run-export as broken

Let me know if you'd like my help with any piece of this. I'm not sure where the migrator comes in but I think I can fix the run-export on 10.2, backport that to 10.1, and mark older packages as broken (if that's the right course of action).

@h-vetinari
Copy link
Member

  1. & 2. are good, and it would be nice if you could do it. 3. should not be necessary, but we need a repodata patch that replaces
-    "fmt >=10.1.1,<11.0a0",
+    "fmt >=10.1.1,<10.2.0a0",

for any packages built against fmt.

I can do the change for the migrator (which currently injects fmt 10 into host, but really needs to do fmt 10.1 to stay consistent with everything built for that ABI).

@bdice
Copy link
Contributor

bdice commented Jan 3, 2024

Great. Item 1 is here: conda-forge/fmt-feedstock#40

For item 2, I can backport this change to 10.1.1 once that PR is merged. Any other versions I should backport to?

@h-vetinari
Copy link
Member

For item 2, I can backport this change to 10.1.1 once that PR is merged. Any other versions I should backport to?

Backporting to 10.1 should be enough (as that's the version that we've been building against for almost 5 months).

@saraedum
Copy link
Member

saraedum commented Jan 3, 2024

Release a new build of fmt 10.2 with a tighter run-export

I am not opposed to this but why do we think that this is necessary? It seems to me that the fmt authors are careful about ABI changes. (In fact, I don't understand what's the actual problem here. On Linux, I don't see a problematic ABI change.)

@saraedum
Copy link
Member

saraedum commented Jan 3, 2024

@bdice @h-vetinari is anybody working on the repodata patch already or should I have a go at it?

@h-vetinari
Copy link
Member

I am not opposed to this but why do we think that this is necessary?

The fact that things broke is a pretty strong argument that it is necessary, unfortunately.

It seems to me that the fmt authors are careful about ABI changes.

I know. Perhaps they can be enlisted to help (or at least elucidate what happened)?

In fact, I don't understand what's the actual problem here. On Linux, I don't see a problematic ABI change.

Are you able to pinpoint the ABI break (or otherwise problematic change) on windows?

is anybody working on the repodata patch already or should I have a go at it?

Please go for it!

@saraedum
Copy link
Member

saraedum commented Jan 3, 2024

Are you able to pinpoint the ABI break (or otherwise problematic change) on windows?

No, I didn't look into extracting the symbol table of a Windows DLL on my Linux machine.

@saraedum
Copy link
Member

saraedum commented Jan 3, 2024

The fact that things broke is a pretty strong argument that it is necessary, unfortunately.

Shouldn't we then pin x.x.x and not just x.x.

I guess an alternative could be to add an integration test to the fmt feedstock that tries to install mamba and checks whether it still works.

@saraedum
Copy link
Member

saraedum commented Jan 3, 2024

So, the problematic symbol is this one:

-                  bool __cdecl fmt::v10::detail::write_console(struct _iobuf * __ptr64,class fmt::v10::basic_string_view<char>)
+                  bool __cdecl fmt::v10::detail::write_console(int,class fmt::v10::basic_string_view<char>)

@h-vetinari
Copy link
Member

So, the problematic symbol is this one:

Cool, thanks for digging! Do you want to open an issue in fmt if such an ABI break in a minor-version was intentional?

@saraedum
Copy link
Member

saraedum commented Jan 3, 2024

Yes, will do once I understand why it happened because that thing is not in the changeset.

@saraedum
Copy link
Member

saraedum commented Jan 3, 2024

Upstream issue: fmtlib/fmt#3785

@saraedum
Copy link
Member

saraedum commented Jan 3, 2024

@h-vetinari wouldn't the proper and easier fix here be to provide the missing symbol again in a patch, rebuild, and then mark build 0 as broken on Windows? (I now realize that the same symbol also changed on Linux. Somehow I screwed up when looking at the symbol table initially. However, that symbol is unused on Linux.)

@h-vetinari
Copy link
Member

wouldn't the proper and easier fix here be to provide the missing symbol again in a patch, rebuild, and then mark build 0 as broken on Windows?

Yeah, that's what I was saying in the other issue. Thanks again for digging into this!

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.

4 participants