-
Notifications
You must be signed in to change notification settings - Fork 181
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
Remove support for manual make builds #657
Remove support for manual make builds #657
Conversation
If we agree to remove the make builds, this PR would be a good opportunity to move the fpm section in README.md to the top. There was a discussion on that, but I forgot where. |
@Carltoffel I agree but suggest to do it in a separate PR to make this easier to review. |
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.
Happy to see them gone from the repo.
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.
Thanks for making this PR, I agree it will reduce clutter and streamline the process for contributors.
I have two suggested changes, but unfortunately I can't make them myself using the built-in diff/comment/suggestions system because they are outside the lines that your proposed changes target.
- on line 66 of
Workflow.md
is still advertises support for make - The "Using stdlib in your project" section of
README.md
should be slightly changed. For example, line 191 implies that there are non-CMake builds of the stdlib (which there won't be after this PR). I might change it something like "If your project uses CMake, the local installation of stdlib can be found using<same CMake snippet>
".
Besides these, everything looks good to me, and I'm happy to approve!
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.
It looks fine.
find . -name "Make*"
and find . -name "make*"
returns nothing so they are gone for good. Aside the comments from @ghbrown
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.
I am in favor of dropping the makefiles to unify the build process. Seeing it with netcdf-fortran for example where the different install/build methods are out of sync and can hamper the usage of the lib as a dependency. Also, relying on CMake, it enables the use of CPM which I really love for dependency management.
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.
I support this PR and change. Thanks.
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.
I'm happy with this change. This seems like the right move based on the current maintenance burden and also the results of the Discourse poll. Now that there's the fpm deployment branch I personally will mostly be using stdlib via fpm now.
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.
I don't have time to look too closely at the changes, but as they're all deletions I'd say this is probably good unless you missed some. I also think the idea of reducing the number of build systems being maintained is a good one.
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.
I'll admit, I did prefer the makefile approach, but overall I do think that "less is more" as they say. I'm approving this.
If there are no objects, I will apply the outstanding fixes and merge on Monday, May 23. |
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.
I think you can merge this now! Let's get it done. Just fix up things if anything breaks after the merge.
@certik, great, thanks for the encouragement. Will merge now. |
Just wanted to mention this discussion from Reference-LAPACK/lapack#488. They have similar problems as we had, keeping two build systems functioning and up to date. For the time being they are keeping both CMake and Make, but there seems to be a slight preference for using CMake. |
This PR removes the manual Makefiles from directories, CI, and instructions docs (README and WORKFLOW). Its purpose is to remove the burden from PR contributors and stdlib maintainers when adding features to stdlib. The obvious downside is that people wouldn't be able to build stdlib with Make anymore.
Even if there is wide approval, I'd like this PR to stay open a while (at least a few weeks) to provide an opportunity for people to voice objections.
I think I removed all Make instances (files and instructions), but it's possible that I missed something.
Because this is a such a high-impact and project-wide change that affects users, I request review from many people. While I am in favor of this change, I am not sure that it's the best thing to do.
Discussion and poll on Discourse: https://fortran-lang.discourse.group/t/how-do-you-intend-to-build-the-fortran-stdlib/1862
Closes #526.