Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly

Sign up
Appearance settings

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

Merged
jvdp1 merged 8 commits into fortran-lang:master from BhoomishGupta:master
Oct 17, 2025

Conversation

@BhoomishGupta
Copy link
Contributor

@BhoomishGupta BhoomishGupta commented Oct 14, 2025

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
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?

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.

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!

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

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.

jvdp1 reacted with thumbs up emoji

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!

Copy link
Contributor

jalvesz commented Oct 17, 2025
edited
Loading

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

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

Reviewers

@jvdp1 jvdp1 jvdp1 approved these changes

@jalvesz jalvesz jalvesz approved these changes

@perazz perazz Awaiting requested review from perazz

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

AltStyle によって変換されたページ (->オリジナル) /