-
-
Notifications
You must be signed in to change notification settings - Fork 19
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
fix external fmt support for downstream packages #51
Conversation
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 ( |
fixes #52 |
@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. |
And yes, there might be incompatible combination of spdlog/fmt at compile time: the C/C++ equivalent of the dll hell. |
@cav71 thanks for the explanation! Is mamba using the CMake config-file packages installed by spdlog though? I am asking because without forcing 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) |
@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 |
Nope, it uses a heuristic approach peeking in tweakme.h file:
|
We could do this using |
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 find_package(spdlog REQUIRED CONFIG)
target_link_libraries(mamba PRIVATE spdlog::spdlog) And that's it. The |
It does propagate, as long as one is using the imported CMake target provided by |
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. |
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: Let's see what they say, I offered to write a PR. |
Actually, now looking at the diff, I wonder what's objectionable about this approach. It looks like 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. 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?
That builds fine, so I don't think it has anything to do with the |
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).
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). |
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. |
I think this discussion is relevant: gabime/spdlog#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 |
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 |
My understanding regarding the mamba issue is that it was solved upstream by proper use of CMake targets: 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 |
If someone isn't using cmake then they wouldn't get the 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 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. |
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 @cav71 I think this just needs my include guards suggestion and fixing the merge conflict and this should be good to go. |
@cav71 gentle nudge. I'm happy to push the changes to this PR if it's okay with you. |
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 ping the 'conda-forge/core' team (using the @ notation in a comment) if you believe this is a bug. |
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 ( |
@conda-forge-linter please rerender |
…nda-forge-pinning 2023.01.12.13.25.31
…ck into fix-external-spdlog
@conda-forge/spdlog please take a look when you get a chance. This should be good to go! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not a maintainer here, but LGTM
Checklist
0
(if the version changed)conda-smithy
(Use the phrase@conda-forge-admin, please rerender
in a comment in this PR for automated rerendering)This change allows building downstream packages following the #50 change.