-
Notifications
You must be signed in to change notification settings - Fork 186
Fix subproject build #284
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
Fix subproject build #284
Conversation
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.
Thank you @awvwgk for these improvements. Unfortunately my knowledge in CMake is really basic. So I am not sure to be the appropriate reviewer for these changes ;)
@certik @milancurcic Could you have a look, please?
For me (as far as I can check), it is good to be merge as is.
I created a repo to ease the review process of this PR: https://github.com/awvwgk/stdlib-cmake-example |
I'm a true CMake n00b so I can't comment on the code. It seems good to me. But I played with the stdlib-cmake-example and it works like a charm. Actually, CMake fetching stdlib and using it as a dependency felt like magic because I never did anything like that with CMake. So I think this will significantly lower the bar for using stdlib for some users. I'm still a bit confused what this PR addresses exactly. If I'm not mistaken, the stdlib-cmake-example works as is with the current master. I think that is using the fetch method. Does this PR then allow using stdlib with the other method described in the stdlib-cmake-example README? Likely just me not understanding the CMake lingo. Anyhow, whichever it is, I trust that this is good to go. |
@milancurcic It does not, I currently let it fetch from my fork instead of the fortran-lang/stdlib repo (see https://github.com/awvwgk/stdlib-cmake-example/blob/623da353140578646daf125f6b6dd46981d9947a/subprojects/CMakeLists.txt#L11-L21). I force-pushed the repo a few times, the initial example fetching from The repo README still explains it with the |
@awvwgk Perfect, that's clear now. :) |
I'll go ahead and merge this PR, we can still revert or patch it later. |
Required changes to use stdlib as subproject in CMake.