-
Notifications
You must be signed in to change notification settings - Fork 1.2k
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
[cmake] Add ninja support for Windows #15635
base: master
Are you sure you want to change the base?
Conversation
PoW item number 6 of the "Builds and Binaries" category |
Test Results 13 files 13 suites 2d 23h 39m 11s ⏱️ Results for commit 5815afb. ♻️ This comment has been updated with latest results. |
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.
LGTM. Let's not forget to update the PoW status accordingly.
Fix ACLiC in debug mode (match ROOT build type) Fix dictionary generation with Eve7 (adjusting rootcling skack size)
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.
One missing comment.
Nope, the code (the condition) onto which you commented has been removed (I fixed rootcling which was failing on eve7) |
Sorry, github misled me :). (and I equally missed the fix :) ) |
No problem! 🙂 |
@@ -2136,6 +2136,10 @@ void AddPlatformDefines(std::vector<std::string> &clingArgs) | |||
clingArgs.push_back(platformDefines); | |||
snprintf(platformDefines, 64, "-DG__VISUAL=%ld", (long)_MSC_VER); | |||
clingArgs.push_back(platformDefines); | |||
#if defined(_WIN64) && defined(_DEBUG) | |||
snprintf(platformDefines, 64, "-D_ITERATOR_DEBUG_LEVEL=0"); //, (long)_ITERATOR_DEBUG_LEVEL); |
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.
Does that fix: "the I/O is still broken in debug mode, due to the special iterators on x64," ?
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.
Unfortunately not, it's just a workaround. The problem is that one can only link the ROOT libraries in debug mode if they use the same flag with their software. Even worse, the externals have to be built with the same flag (e.g. I had the issue with Pythia8)!
And you are the only one able to fix the issue!
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.
Ok.
if(CMAKE_GENERATOR MATCHES Ninja) | ||
if (CMAKE_BUILD_TYPE MATCHES Debug) |
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.
if(CMAKE_GENERATOR MATCHES Ninja) | |
if (CMAKE_BUILD_TYPE MATCHES Debug) | |
if(CMAKE_GENERATOR MATCHES Ninja AND CMAKE_BUILD_TYPE MATCHES Debug) |
Maybe better?
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.
Nope. the if()
- else()
test is for ninja only, independently of the build type
builtins/pcre/CMakeLists.txt
Outdated
if(winrtdebug) | ||
set(PCRE_POSTFIX $<$<CONFIG:Debug>:d>) | ||
set(pcre_config_kind "Debug") | ||
else() | ||
set(pcre_config_kind "Release") | ||
endif() | ||
set(pcre_config "--config") |
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.
Wait, is this not in the else()
of the if(WIN32)
branch? Why is winrtdebug
relevant then? I thought if(WIN32)
is true for any ROOT build on Windows
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.
The code is correct, but I'll make it more clear
set(CLAD_BYPRODUCTS | ||
${_CLAD_LIBRARY_PATH}/cladPlugin.lib | ||
${_CLAD_LIBRARY_PATH}/cladDifferentiator.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.
Why can't CMAKE_STATIC_LIBRARY_PREFIX
and CMAKE_STATIC_LIBRARY_SUFFIX
be used in case of Ninja on Windows?
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.
Well, I'll cross-check...
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.
You were right. Fixed. Thanks!
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.
Very nice! I have only a few small questions and suggestions
No description provided.