-
Notifications
You must be signed in to change notification settings - Fork 197
submodule procedures declaration made compatible with older cmake version #1041
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
Conversation
Thank you. Shoudl the version in the CMakeLists.txt be updated?
Also, should a CI/CD be included to test the minimum version of CMake?
Thanks @BhoomishGupta for opening this PR.
@jvdp1 I saw that in the cmake files there is cmake_minimum_required(VERSION 3.14.0) but if indeed the available version is more recent, then this kind of issues goes unnoticed. When you propose to have a CI/CD job for this, do you mean to say to force installation of a specific old cmake version under one runner to ensure that minimum is actually functional?
@BhoomishGupta beyond just changing the code, it would be useful for this modification to be documented in the style_guide.md as a suggestion.
@jvdp1 I saw that in the cmake files there is
cmake_minimum_required(VERSION 3.14.0)but if indeed the available version is more recent, then this kind of issues goes unnoticed. When you propose to have a CI/CD job for this, do you mean to say to force installation of a specific old cmake version under one runner to ensure that minimum is actually functional?
Yes, I think it would be good to have at least a old cmake corresponding to the minimum required version of cmake mentioned in CMakeLists.txt. So, users will be sure that at least this old version is fine with stdlib.
I am not sure that having a CI/CD with an older version that the minimum required one will be useful, as it might fail (or not if we are lucky). What do you think?
@BhoomishGupta beyond just changing the code, it would be useful for this modification to be documented in the style_guide.md as a suggestion.
Good suggestion!
@BhoomishGupta beyond just changing the code, it would be useful for this modification to be documented in the style_guide.md as a suggestion.
Will do that
I am not sure that having a CI/CD with an older version that the minimum required one will be useful, as it might fail (or not if we are lucky). What do you think?
One thing we could try in a separate PR is to affect the gcc-10 job in the CI.yml with cmake 3.14 to have it as a reference.
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.
Almost ready!
@BhoomishGupta when you took the text I proposed to you, the link was lost. In order for a link to be shown properly with markdown you should wrap the actual text with "[]" and the link with "()" as follows: [Spurious modules](https://gitlab.kitware.com/cmake/cmake/-/issues/18427#note_983426)
@jalvesz that is incredibly helpful, thank you. I didn't realize I needed to wrap it that way, so I appreciate you teaching me something new!
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! Thank you @BhoomishGupta for this PR. As @jalvesz approved it too, I will merge it.
Made change for issue no. #1035 (Stablish rule for submodule procedures declaration compatible with older cmake versions)