-
Notifications
You must be signed in to change notification settings - Fork 338
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
Only full shared or full static build #2342
Conversation
05b40db
to
3c208f6
Compare
fyi @pavelzw |
9ff079a
to
88c4471
Compare
Looks good to me! |
@@ -48,19 +47,15 @@ message(STATUS "libmamba binary version: v${LIBMAMBA_BINARY_VERSION}") | |||
|
|||
option(BUILD_TESTS "Build libmamba C++ tests" OFF) | |||
option(BUILD_SHARED "Build shared libmamba library" OFF) | |||
option(BUILD_STATIC "Build static libmamba library" OFF) | |||
option(BUILD_STATIC_DEPS "Build static libmamba library with static linkage to its dependencies" OFF) | |||
option(BUILD_STATIC "Build static libmamba library with static linkage to its dependencies" OFF) |
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.
Should we take this opportunity to introduce an prefix on options: MAMBA_BUILD_STATIC
etc. ?
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.
I don't know, MAMBA_BUILD_MICROMAMBA
sounds a bit weird. And there is already a lot to type to build micromamba for instance
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.
MAMBA_BUILD_MICRO
?
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.
still weird. And I don't think we need this prefix as long as this is not used as a framework in another repo building with cmake.
endif () | ||
endif () | ||
|
||
if (NOT (BUILD_SHARED OR BUILD_STATIC OR BUILD_STATIC_DEPS)) | ||
if (NOT (BUILD_SHARED OR BUILD_STATIC)) |
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.
Should there be s single option, where true means static, false means shared?
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.
yeah I think we should default to SHARED
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.
I think BUILD_SHARED
and BUILD_STATIC
are more expressive. And you may want to build both at the same time.
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.
Ha right, it's too target, not one that transmute
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.
yeah I think we should default to
SHARED
But in that case if you want to build the static version only, you need to pass two options ( BUILD_SHARED=OFF
and BUILD_STATIC=ON
) instead of a single one
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.
I can add options like:
- BUILD_ALL_SHARED
- BUILD_ALL_STATIC
The only question is should we build tests with these targets? (sometimes you just want to build micromamba for reproducing an issue or debugging, and you don't want tot build the tests).
This PR simplifies the cmake and removes an hybrid build that might be problematic because of mixing different runtimes.
MICROMAMBA_LINKAGE
andMAMBA_PACKAGE_LINKAGE
can be set toSTATIC