Explicit and smart CMake target #2935
Conversation
AntoinePrv
force-pushed
the
libmambapy
branch
from
7131cef to
9ec3697
Compare
There was a problem hiding this comment.
I like the namespacing of the targets, but I am a bit skeptical about the target alias (see my comment below).
@Klaim @jjerphan @Hind-M @jonashaag any comment about this?
libmamba/libmambaConfig.cmake.in
Outdated
| if(BUILD_SHARED_LIBS AND TARGET mamba::libmamba-dyn) | ||
| add_library(mamba::libmamba ALIAS mamba::libmamba-dyn) | ||
| else() | ||
| add_library(mamba::libmamba ALIAS mamba::libmamba-static) |
There was a problem hiding this comment.
I'm a bit skeptical about this alias, I find it confusing to have a target that can be either the dynamic or the static library depending on what has been built and/or installed. The pattern "libname" / "libname-static" for the target names is quite widespread and more explicit.
There was a problem hiding this comment.
Very often though libname would also be either static or dynamic depending on the options, so I thought let's make one target that is always dynlib, and keep the dispatch.
I'm also not such a fan, but it is convenient for inside our own CMake, otherwise we keep repeating if/else in tests/libmambapy/mamba-package...
There was a problem hiding this comment.
It seems possible to build both the shared and static libraries (options seem not exclusive). This might cause a problem if this is actually the case.
As a developer using libmamba, I think I would find it normal to have mamba::libmamba alias mamba::libmamba-dyn, but I am not sure about having it alias mamba::libmamba-static conditionally.
Note: I am just getting started on contributing to mamba.
There was a problem hiding this comment.
Naive question: it seems possible to build both the shared and static libraries (options seem not exclusive). This might cause a problem if this is actually the case.
Outside Conda it's quite classic to provide both this way, so that the end-user decides what to use easilly.
Note that in this project, we use the static version to build the stand-alone micromamba. I'm ignoring the conda packaging mess in this comment, obviously. :)
There was a problem hiding this comment.
Very often though libname would also be either static or dynamic depending on the options, so I thought let's make one target that is always dynlib, and keep the dispatch.
I agree, and I think it's a bad practice that we should fix (at least in conda where we can).
I'm also not such a fan, but it is convenient for inside our own CMake, otherwise we keep repeating if/else in tests/libmambapy/mamba-package...
Then let's use an alias internally, but I realy think we should not expose it.
There was a problem hiding this comment.
I think the use case for pubic mamba::libmamba is in a VCPKG (or source) like workflow, you configure static/dynamic externally, so then it is not the responsibility of the user CMake script to choose the target.
In fact, that is the bahaviour for CMake add_library: it dispatches between dynamic and static.
But really, that works for me too, I don't really care.
| if(BUILD_SHARED_LIBS AND BUILD_SHARED) | ||
| add_library(mamba::libmamba ALIAS libmamba-dyn) | ||
| elseif(BUILD_STATIC) | ||
| add_library(mamba::libmamba ALIAS libmamba-static) | ||
| elseif(BUILD_SHARED) | ||
| add_library(mamba::libmamba ALIAS libmamba-dyn) | ||
| else() |
There was a problem hiding this comment.
Continuing the discussion from https://github.com/mamba-org/mamba/pull/2935/files#r1375979846: what if BUILD_SHARED_LIBS AND BUILD_SHARED AND BUILD_STATIC?
Also, could it be simplified to:
| if(BUILD_SHARED_LIBS AND BUILD_SHARED) | |
| add_library(mamba::libmamba ALIAS libmamba-dyn) | |
| elseif(BUILD_STATIC) | |
| add_library(mamba::libmamba ALIAS libmamba-static) | |
| elseif(BUILD_SHARED) | |
| add_library(mamba::libmamba ALIAS libmamba-dyn) | |
| else() | |
| if(BUILD_SHARED_LIBS OR BUILD_SHARED) | |
| add_library(mamba::libmamba ALIAS libmamba-dyn) | |
| elseif(BUILD_STATIC) | |
| add_library(mamba::libmamba ALIAS libmamba-static) | |
| else() |
There was a problem hiding this comment.
if BUILD_SHARED_LIBS is set but not BUILD_SHARED (which IMHO we should really scope with MAMBA_BUILD_SHARED), then the libmamba-dyn target will not exists. The Point of the mamba::libmamba alias is more "whaterver, as long as it works".
AntoinePrv
force-pushed
the
libmambapy
branch
from
9ec3697 to
0a9c023
Compare
|
Remaining tests are not impacted by CMake. |
No description provided.