Check options #243
Check options #243
Conversation
| set(OPTION_VALUE | ||
| "${OPTION_VALUE}" | ||
| PARENT_SCOPE | ||
| # https://cmake.org/cmake/help/latest/command/if.html#basic-expressions (1/0 is problematic) |
There was a problem hiding this comment.
It could mean "ON" or NUMBER_OF_CORE "1"
There was a problem hiding this comment.
Then why not change all values to "1" instead of to "ON"? Thus this case will be covered
There was a problem hiding this comment.
Good point with the truthy/falsy values! There could however be more problems with changing the passed parameter to the dependency, e.g. LETTER_TO_PARSE Y and so on, which would lead to strange and unexpected bugs for some users. I think we should either use the normalisation only when checking of parameter consistency or add a note in the readme that we recommend truthy/falsy values to be standardised in CPM.cmake options (e.g. to YES/NO).
There was a problem hiding this comment.
@TheLartians yes good point, this it's an other problem. Maybe CPM could accept only TRUE YES ON. It makes it more informative and I'm temptated to say using 1 or Y for TRUE should be avoided. I think we should either use the normalisation only when checking of parameter consistency or add a note in the readme that we recommend truthy/falsy values to be standardised in CPM.cmake options (e.g. to YES/NO). I agree totlaly to this point but maybe we could add TRUE and On as they are not preblematic and very well used. I have never seen 1 or Y.
cmake/CPM.cmake
Outdated
| @@ -271,6 +271,55 @@ function(cpm_check_if_package_already_added CPM_ARGS_NAME CPM_ARGS_VERSION) | |||
| "${CPM_INDENT} requires a newer version of ${CPM_ARGS_NAME} (${CPM_ARGS_VERSION}) than currently included (${CPM_PACKAGE_VERSION})." | |||
| ) | |||
| endif() | |||
|
|
|||
| if(CPM_ARGS_OPTIONS) | |||
There was a problem hiding this comment.
What if when the that package was first added options were not set and now they are, or vice-versa?
There was a problem hiding this comment.
Yes maybe this cases are not well checked.
There was a problem hiding this comment.
Hey @flagarde, thanks for re-implementing the options check!
Thinking a bit more about the options, I'm not sure if it's a good idea to perform the check in all cases. E.g. often an upstream package would override a downstream version with a newer dependency. In that case the option names and behaviours could have changed, so the old options no longer apply.
Perhaps we should only perform the check if the versions match, or at least offer a way to opt-out?
| set(OPTION_VALUE | ||
| "${OPTION_VALUE}" | ||
| PARENT_SCOPE | ||
| # https://cmake.org/cmake/help/latest/command/if.html#basic-expressions (1/0 is problematic) |
There was a problem hiding this comment.
Good point with the truthy/falsy values! There could however be more problems with changing the passed parameter to the dependency, e.g. LETTER_TO_PARSE Y and so on, which would lead to strange and unexpected bugs for some users. I think we should either use the normalisation only when checking of parameter consistency or add a note in the readme that we recommend truthy/falsy values to be standardised in CPM.cmake options (e.g. to YES/NO).
If I understand how CPM is working now, if the upstream ask for a newer version a warning will be raised and the new version will be used. I think this are two different problems and it would be nice to have the warning for the version and the warning for the options. If you are not warned you could disable some option necessary downstream without knowing it. Sometimes you don't know exactly what are the need of the used repositories. It could be possible to add an option to disable the check. Internally CMake use ON so if you want to force only one key maybe you should choose this one |
|
In case you include two time the same repo with the same version : Do you want to keep the previous option or the last one. I suppose you want to keep the last one ? In cas you have same repo but two version you want new version and the new options right ? |
|
Well with the way CPM.cmake currently works (we immediately add the package after definition), we always use the first version and options added. In case another dependency asks for a newer version that the one currently added, we inform the user it may be necessary to upgrade by outputting a warning. As for options I think we must ask ourselves which scenarios are likely. E.g. if a project adds the same dependency twice, but using different options, I see the following scenarios:
In all cases we might consider adding an |
Hi,
Restore the ability to check OPTIONS.