De-vendor fmt

[bluescarni]

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-linter]

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.

[traversaro]

Hi @bluescarni, are you sure that this change did not broke the ABI of the spdlog package 1.10.*? Ideally if a new release is done anytime soon, we should do the change while we bump the spdlog version.

[bluescarni]

@traversaro I am not sure, but it probably did. Is it possible to trigger a rebuild of the conda packages depending on spdlog?

[cav71]

Hi, this latest change (4e0b46c->1762193) seems breaking building mamba:

.../include/spdlog/fmt/fmt.h:24:14: fatal error: 'spdlog/fmt/bundled/core.h' file not found

To build the libmamba, I've followed the instructions in build steps

[ryanvolz]

@traversaro I am not sure, but it probably did. Is it possible to trigger a rebuild of the conda packages depending on spdlog?

This did break the ABI: I have a package that was built with spdlog before this change and one that depends on that one and is trying to build with the new spdlog. The build works, but there are missing symbols when trying to run it because of the change from internal fmt 8 and external fmt 9.

I think all conda-forge packages depending on spdlog will need to be rebuilt. And should spdlog now add a run_exports for fmt because it requires anything compiling against it to link and depend on the external fmt?

[h-vetinari]

And should spdlog now add a run_exports for fmt because it requires anything compiling against it to link and depend on the external fmt?

fmt already has a run-export, so if it's used as a host-dep, it should be there at runtime.

AFAICT from the comment above, the issue is that we'll have to patch /include/spdlog/fmt/fmt.h to point to "our" fmt, as opposed to spdlog/fmt/bundled/core.h.

[ryanvolz]

fmt already has a run-export, so if it's used as a host-dep, it should be there at runtime.

I could be wrong, but with the package that I am failing to build following this change, which itself does not depend on fmt but does depend on spdlog, does not get a run requirement added through that run_exports even though it now links to fmt by virtue of using spdlog. My understanding is that the run_exports for fmt does not come into effect because it is not a host requirement of that package, even though it is present in the host environment because spdlog now requires it. That's why I think spdlog needs the additional run_exports.

[h-vetinari]

That's why I think spdlog needs the additional run_exports.

The run-dependence on fmt is there already:

I don't know which package you're having problems with, but one useful thing to keep in mind in all this is "host dependencies are not transitive".

In any case, spdlog has a run_export for itself (meaning if you host-depend on spdlog for a package, it'll gain a run-dep on spdlog), and spdlog itself has a host-dependence on fmt (as of the most recent build), from whence it inherits a run-dep on fmt as well. That means both spdlog & fmt should be in the final env for a package build against the newest spdlog.

Without more precise errors, I don't know what else people are seeing, but the libmamba case from above points to a problem with spdlog hardcoding the paths to its bundled fmt. This we need to undo.

BTW. If the breakage is widespread enough, I'm perfectly fine if we mark the builds of this PR as broken (https://github.com/conda-forge/admin-requests/), and then we can try again with less pressure.

[bluescarni]

AFAICT from the comment above, the issue is that we'll have to patch /include/spdlog/fmt/fmt.h to point to "our" fmt, as opposed to spdlog/fmt/bundled/core.h.

I don't think so, the problem there is that mamba is not using the CMake config-file packages installed by spdlog, and it is trying to figure out the correct definitions on its own instead. See discussion here:

#51 (comment)

[bluescarni]

BTW. If the breakage is widespread enough, I'm perfectly fine if we mark the builds of this PR as broken (https://github.com/conda-forge/admin-requests/), and then we can try again with less pressure.

If we revert this, it's going to be impossible for packages using the current fmt blessed by conda (i.e., 9.x) to depend on spdlog, and a different set of breakages will follow. I think we should push ahead instead. I wish I had pushed back against #35 when I had the chance :(

[h-vetinari]

If we revert this, it's going to be impossible for packages using the current fmt blessed by conda (i.e., 9.x) to depend on spdlog, and a different set of breakages will follow.

I didn't mean revert indefinitely, but stop the bleeding and then do a fix that doesn't break people. I fully agree that we need to get away from vendoring fmt.

I don't think so, the problem there is that mamba is not using the CMake config-file packages installed by spdlog, and it is trying to figure out the correct definitions on its own instead. See discussion here:

Seeing a path like spdlog/fmt/bundled/... makes me highly suspicious, but if I understood the discussion in #51 correctly, there should be a solution that doesn't require excessive patching - I've commented there

[traversaro]

The problem is that even if we rebuilt all the packages that depend on spdlog in conda-forge, their old build will continue to be broken (without considering conda packages in other channels that were relying on the ABI stability of the spdlog conda-forge package) . I totally agree that we should mark the build with the broken ABI for now, and then due a proper migration to a spdlog with a de-vendored fmt. The only way that I know of is to wait for the next ABI incompatible release of spdlog (based on https://github.com/conda-forge/spdlog-feedstock/blob/main/recipe/meta.yaml#L16a minor release, i.e. 1.11) and de-vendor fmt while we upgrade to the new version, but perhaps there is some other alternative.

[traversaro]

Anyhow, as this discussion is mixing several discussions (the actual PR itself, discussion on the ABI break, discussion on mamba not supporting spdlog with de-vendored fmt) I think we can open a new issue just to track the ABI break: #53 .

[ryanvolz]

Sorry for not being clear, and for trying to rectify that now with even more words on a closed PR.

The error I'm seeing is with gr-satellites, and it has to do entirely with the ABI break. The run_exports discussion is more an enhancement.

In any case, spdlog has a run_export for itself (meaning if you host-depend on spdlog for a package, it'll gain a run-dep on spdlog), and spdlog itself has a host-dependence on fmt (as of the most recent build), from whence it inherits a run-dep on fmt as well. That means both spdlog & fmt should be in the final env for a package build against the newest spdlog.

Builds against spdlog with the de-vendored fmt do succeed, and (ABI break aside) would run fine because of what you've outlined: spdlog run-depends on the appropriate fmt and so fmt will always be present if a package depends on spdlog.

The slight inconvenience is that with this current package, gr-satellites for example ends up with DSO warnings:

WARNING (gnuradio-satellites,lib/libgnuradio-satellites.so.5.1.0.0): Needed DSO lib/libfmt.so.9 found in ['fmt']
WARNING (gnuradio-satellites,lib/libgnuradio-satellites.so.5.1.0.0): .. but ['fmt'] not in reqs/run, (i.e. it is overlinking) (likely) or a missing dependency (less likely)

This is not, strictly speaking, a problem, because spdlog will always bring along fmt and gr-satellites depends on spdlog.

1. I could ignore it, and that's fair enough. But other maintainers might see the warning on their own packages, and they might spend time wondering if something is wrong. If they do anything about it, they probably end up doing (2) below.
2. I could add fmt as a host dependency and clear the warning, but that's wrong enough that it bothers me. gr-satellites doesn't itself depend on fmt, only through spdlog. If spdlog were to switch back to using the internal fmt, gr-satellites would be left with an extra unnecessary dependency on fmt, and that might even break things. It would also get migrated when the fmt pin changes, even though it wouldn't need that.
3. I think the way to be pedantic about the dependencies and clear the DSO warning for all consumers is to have spdlog tell its consumers whether or not they need to depend on fmt, i.e. this version of spdlog would include a run_exports: - {{ pin_compatible('fmt', max_pin='x') }}.

But I can understand if that's seen as unnecessary.

[h-vetinari]

The slight inconvenience is that with this current package, gr-satellites for example ends up with DSO warnings:

WARNING (gnuradio-satellites,lib/libgnuradio-satellites.so.5.1.0.0): Needed DSO lib/libfmt.so.9 found in ['fmt']
WARNING (gnuradio-satellites,lib/libgnuradio-satellites.so.5.1.0.0): .. but ['fmt'] not in reqs/run, (i.e. it is overlinking) (likely) or a missing dependency (less likely)

How certain are you that this is a misfire (i.e. is there truly no way that the package had depended implicitly on fmt through spdlog)? E.g. if any fmt(-derived) types are used in an interface.

I ask because this goes against all experience for how the layering (and this warning) is supposed to work. I can be wrong of course.