-
Notifications
You must be signed in to change notification settings - Fork 60
New issue
Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? # to your account
Add option to allow relaxing the required match of python versions #368
Conversation
CMakeLists.txt
Outdated
@@ -80,20 +81,33 @@ if (NOT "cxx_std_17" IN_LIST ROOT_COMPILE_FEATURES) | |||
message(FATAL_ERROR "You are trying to build podio against a version of ROOT that has not been built with a sufficient c++ standard. podio requires c++17") | |||
endif() | |||
#Check if Python version detected matches the version used to build ROOT | |||
SET(Python_FIND_FRAMEWORK LAST) | |||
set(Python_FIND_FRAMEWORK LAST) |
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.
Please no gratuitous changes like these
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 I revert it? Doesn't it help to use the preferred cmake style? Unless we have one but this file has both lower and upper case
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.
Yes, please revert for this PR. We can fix the inconsistencies in a different PR that only fixes that. Otherwise we will have commits that are mixing format / style changes and functional changes, which makes it hard to analyze the history.
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.
It still had mixed style after the couple changes. Yes, it would be better to use one style, but that should be CI enforced, and then we can use the git features to ignore them when using git blame
.
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 those changes are now removed
8501cd4
to
28efac0
Compare
BEGINRELEASENOTES
ENDRELEASENOTES
find_package
will let everything go through but a later check will only allow mismatches in the patch version and throw and error if there is a mismatch in the major or minor versions. Does it make sense to have it ON by default since it's only the patch version? The error message looks like this:in red, and stops
cmake
.Also a few changes to lower case to be more consistent across the fileCloses #366