Skip to content

Conversation

@BhoomishGupta
Copy link
Contributor

Made change for issue no. #1035 (Stablish rule for submodule procedures declaration compatible with older cmake versions)

@jvdp1 jvdp1 requested review from jalvesz and perazz October 14, 2025 18:45
@jvdp1
Copy link
Member

jvdp1 commented Oct 14, 2025

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?

@jalvesz
Copy link
Contributor

jalvesz commented Oct 15, 2025

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
Copy link
Member

jvdp1 commented Oct 15, 2025

@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
Copy link
Contributor Author

@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

@jalvesz
Copy link
Contributor

jalvesz commented Oct 17, 2025

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.

Copy link
Contributor

@jalvesz jalvesz left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Almost ready!

@BhoomishGupta BhoomishGupta requested a review from jalvesz October 17, 2025 10:16
@jalvesz
Copy link
Contributor

jalvesz commented Oct 17, 2025

@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)

@BhoomishGupta
Copy link
Contributor Author

@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!

Copy link
Member

@jvdp1 jvdp1 left a 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.

@jvdp1 jvdp1 merged commit e5d296d into fortran-lang:master Oct 17, 2025
16 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants