-
Notifications
You must be signed in to change notification settings - Fork 6.6k
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
[mimalloc] Add new port (fix #6996) #7011
Conversation
good scripts in the port file for execution) |
ports/mimalloc/fix-cmake.patch
Outdated
+ set_target_properties(mimalloc-static PROPERTIES OUTPUT_NAME ${mi_output_name}) | ||
+else() | ||
+ set_target_properties(mimalloc-static PROPERTIES OUTPUT_NAME ${mi_basename}) | ||
+endif() |
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.
Because this is wrapped in if(NOT BUILD_SHARED_LIBS)
, is it required to rename the library?
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.
Hi! @ras0219-msft Thanks for your kindly review!
I created a PR in the upstream the day before yesterday: microsoft/mimalloc#22. And the fix-cmake.patch
used in this PR is based on that one. That's why it looks weired. I will fix it.
ports/mimalloc/fix-cmake.patch
Outdated
+endif() | ||
target_compile_definitions(mimalloc-static PRIVATE ${mi_defines} MI_STATIC_LIB) | ||
+if(NOT WIN32) | ||
+ # It is only possible to override malloc on Windows when building as a DLL. |
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.
This looks like it should be a compile error to me; the user explicitly requested the override
feature and static libs -- we should tell them that this isn't possible.
I think this would be easiest done in the portfile (checking for system_name etc). Given that, perhaps these patches aren't needed and we can use upstream's list(APPEND mi_defines MI_MALLOC_OVERRIDE)
?
vcpkg_replace_string( | ||
${CURRENT_PACKAGES_DIR}/include/mimalloc.h | ||
"!defined(MI_SHARED_LIB)" | ||
"0 // !defined(MI_SHARED_LIB)" |
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.
Do we not need to force-define this the other way in static mode?
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 we do not need to do anything when built as a static library as long as the users do not define MI_SHARED_LIB
by themselves.
#ifdef _MSC_VER
#if !defined(MI_SHARED_LIB)
#define mi_decl_export
#elif defined(MI_SHARED_LIB_EXPORT)
#define mi_decl_export __declspec(dllexport)
#else
#define mi_decl_export __declspec(dllimport)
#endif
...
#endif
ports/mimalloc/portfile.cmake
Outdated
|
||
check_feature(asm MI_SEE_ASM) | ||
check_feature(secure MI_SECURE) | ||
check_feature(override MI_OVERRIDE) |
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.
Your vcpkg_check_features()
function has been merged, wouldn't using it here be cleaner?
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.
Hi! @vicroms Thanks!
Thanks for the PR! |
No description provided.