fix external fmt support for downstream packages

[cav71]

Checklist

• [x] Used a personal fork of the feedstock to propose changes
• [x] Bumped the build number (if the version is unchanged)
• [x] Reset the build number to 0 (if the version changed)
• [x] Re-rendered with the latest conda-smithy (Use the phrase code>@<space/conda-forge-admin, please rerender in a comment in this PR for automated rerendering)
• [x] Ensured the license file is being packaged.

This change allows building downstream packages following the #50 change.

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

[cav71]

fixes #52

[bluescarni]

@cav71 I am not sure I understand what is going on here.

In #50 I enabled the spdlog build option -D SPDLOG_FMT_EXTERNAL=ON, and I thought this was enough. Are you saying that we need also to manually alter the tweakme.h in order to truly prevent spdlog from using its internal fmt copy?

[cav71]

@bluescarni Yes, because setting -D SPDLOG_FMT_EXTERNAL=ON doesn't propagate to any downstream application.

spdlog is a header only, so there's nothing to compile (except for the tests) and tweakme.h gets included always with SPDLOG_FMT_EXTERNAL commented out (by default, which is probably the correct behavior).

Ideally every application (let's say libmamba) including spdlog should be defining the SPDLOG_FMT_EXTERNAL define (-D) and there won't be need for tweakme.h: so they can freely decide if using the external fmt or not.

[cav71]

And yes, there might be incompatible combination of spdlog/fmt at compile time: the C/C++ equivalent of the dll hell. So having it always external is a good call IMHO.

[bluescarni]

@cav71 thanks for the explanation! Is mamba using the CMake config-file packages installed by spdlog though?

I am asking because without forcing -DSPDLOG_FMT_EXTERNAL in the C(XX)FLAGS of my project and without touching tweakme.h, it seems to me that the external spdlog definition is picked up automatically:

https://github.com/bluescarni/heyoka/actions/runs/3311147440/jobs/5466235710#step:3:3249

(this is a CI run from a project of mind depending on spdlog)

[bluescarni]

@cav71 indeed, if you look into the CMake files in the conda package of spdlog:

https://anaconda.org/conda-forge/spdlog/1.10.0/download/linux-64/spdlog-1.10.0-hf88bb37_1.tar.bz2

You will see that the -DSPDLOG_FMT_EXTERNAL definition is already (correctly) included in the spdlog exported target.

[cav71]

@cav71 thanks for the explanation! Is mamba using the CMake config-file packages installed by spdlog though?

Nope, it uses a heuristic approach peeking in tweakme.h file:

# Check spdlog C header for external fmt configuration.
find_file(SPDLOG_TWEAK_H NAMES spdlog/tweakme.h)
if(SPDLOG_TWEAK_H)
    file(READ ${SPDLOG_TWEAK_H} SPDLOG_TWEAK_H_STRING)
    if("${SPDLOG_TWEAK_H_STRING}" MATCHES "\n *#define *SPDLOG_FMT_EXTERNAL *")
        find_package(fmt REQUIRED)
        target_link_libraries(bindings PRIVATE fmt::fmt)
    endif()
endif()

[h-vetinari]

Yes, because setting -D SPDLOG_FMT_EXTERNAL=ON doesn't propagate to any downstream application.

We could do this using target_compile_definitions(spdlog INTERFACE SPDLOG_FMT_EXTERNAL), then it gets propagated to consumers.

[bluescarni]

We could do this using target_compile_definitions(spdlog INTERFACE SPDLOG_FMT_EXTERNAL), then it gets propagated to consumers.

There's no need to do any of this, I tried to explain this already but perhaps I wasn't clear enough. Let me try again.

When one builds spdlog with the option -D SPDLOG_FMT_EXTERNAL=ON, the CMake files installed by spdlog already export this type of information. Thus, all a downstream project (such as mamba) needs to do, is the following:

find_package(spdlog REQUIRED CONFIG)
target_link_libraries(mamba PRIVATE spdlog::spdlog)

And that's it. The spdlog::spdlog imported target already brings in the SPDLOG_FMT_EXTERNAL definition required by mamba.

[bluescarni]

@bluescarni Yes, because setting -D SPDLOG_FMT_EXTERNAL=ON doesn't propagate to any downstream application.

It does propagate, as long as one is using the imported CMake target provided by spdlog.

[h-vetinari]

I support the unvendoring in principle, but since this came not at a version boundary, it leads to breakage. That needs to be addressed, either by marking the current builds as broken and trying again later, or proactively fixing those feedstocks (& packages) that were relying on previous behaviour.

I'm not sure how many packages consume spdlog, nor if all of those are ready/willing to use CMake as a build system. A build-system-independent change would be to patch in the right header inclusions (pointing to our fmt, not the spdlog-bundled one), then the problem becomes moot.

Whether you feel that's a good solution or not is completely up to you, but if you don't want to go that route, the we need to have another way to fix this in the next few days.

[bluescarni]

I'm not sure how many packages consume spdlog, nor if all of those are ready/willing to use CMake as a build system.

The subject of this report is mamba, which does use CMake. I opened a report here asking why they don't just try to use directly spdlog's CMake support rather than writing custom code in their build system:

https://github.com/mamba-org/mamba/issues/2044

Let's see what they say, I offered to write a PR.

[h-vetinari]

Actually, now looking at the diff, I wonder what's objectionable about this approach. It looks like #define SPDLOG_FMT_EXTERNAL was meant exactly for this kind of situation (see surrounding comment) - so it's even less surprising that other projects would build upon that.

There's at least one other feedstock affected already (https://github.com/conda-forge/gnuradio-satellites-feedstock/pull/72), and - as always - the non-reported number is probably a bunch higher.

FWIW, I think this PR should be merged. I'd much prefer this than slowly fixing all consumers to go to CMake, while an unknown number of feedstocks is broken in the meantime.

[bluescarni]

FWIW, I think this PR should be merged. I'd much prefer this than slowly fixing all consumers to go to CMake, while an unknown number of feedstocks is broken in the meantime.

We can merge the PR, but I fear we will have to mark the build as broken anyway as it seems there's widespread ABI breakage. Unless there's a way of triggering a rebuild of all packages depending on spdlog on conda-forge?

There's at least one other feedstock affected already (conda-forge/gnuradio-satellites-feedstock#72), and - as always - the non-reported number is probably a bunch higher.

That builds fine, so I don't think it has anything to do with the SPDLOG_FMT_EXTERNAL definition.

[h-vetinari]

We can merge the PR, but I fear we will have to mark the build as broken anyway as it seems there's widespread ABI breakage.

Yeah, that could unfortunately be the case. Not sure how often there are new spdlog releases (i.e. whether it makes sense to wait for the next one and do it then).

Unless there's a way of triggering a rebuild of all packages depending on spdlog on conda-forge?

The bot infra will have that somewhere (e.g. if one constructs the graph from https://github.com/regro/libcfgraph/ and looks at it from the right angle), but otherwise the only reasonable way (especially for not super widespread packages) is to just search for "- " within the conda-forge organisation and click through all the hits to check which feedstocks they're from (the new GitHub code search is much more reliable btw).

[cav71]

i would put this PR on hold, maybe is a mamba issue after all and this can help any (spdlog) downstream package to fix the issue.

[ryanvolz]

I think this discussion is relevant: https://github.com/gabime/spdlog/issues/2310

The way I read it: mamba doesn't consume either the CMake target or the CFLAGS from spdlog's pkg-config, so either it needs to change to do that, it needs to define SPDLOG_FMT_EXTERNAL on its own, or it needs something like this PR (maybe with #ifndef guards as shown in the linked issue). For what it's worth, it seems like Homebrew includes changes similar to this PR.

[traversaro]

Not sure if it is related to this PR, but apparently Debian has a patch to unvendor fmt that is not just setting to `SPDLOG_FMT_EXTERNAL to ON, but also adding some additional includes: https://salsa.debian.org/med-team/spdlog/-/blob/master/debian/patches/use-external-fmt.patch .

[bluescarni]

My understanding regarding the mamba issue is that it was solved upstream by proper use of CMake targets:

https://github.com/mamba-org/mamba/pull/2048

It looks to me like they are now explicitly linking to the header-only versions of both spdlog and fmt in order to avoid ABI issues (I presume).

Shall we keep this report open or can we close it? @traversaro @cav71

[kkraus14]

If someone isn't using cmake then they wouldn't get the #define SPDLOG_FMT_EXTERNAL without this change.

That being said, I don't have a clear picture as to whether we should be differing the behavior that spdlog ships with. For C++20 they're also giving a mutually exclusive option to use std::format instead of fmt as well.

spdlog is also not strictly header only where there's a compiled binary that can be used to speed up downstream compilation. Whether bundled fmt, external compiled fmt, external header only fmt, or std::format is used would impact that compiled binary.

[kkraus14]

My 2c: we should make a choice for now which I'd vote for external compiled fmt. If for some reason someone needs other variants we could add build variants and use a track_features in order to keep external compiled fmt as the default variant.

@cav71 I think this just needs my include guards suggestion and fixing the merge conflict and this should be good to go.

[kkraus14]

@cav71 gentle nudge. I'm happy to push the changes to this PR if it's okay with you.

[conda-forge-webservices[bot]]

Hi! This is the friendly automated conda-forge-linting service.

I was trying to look for recipes to lint for you, but it appears we have a merge conflict. Please try to merge or rebase with the base branch to resolve this conflict.

Please ping the 'conda-forge/core' team (using the @ notation in a comment) if you believe this is a bug.

[conda-forge-webservices[bot]]

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.

[kkraus14]

@conda-forge-linter please rerender

[kkraus14]

@conda-forge/spdlog please take a look when you get a chance. This should be good to go!