Skip to content
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

Merged
merged 4 commits into from
May 19, 2022

Conversation

milancurcic
Copy link
Member

@milancurcic milancurcic commented May 7, 2022

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.

@Carltoffel
Copy link
Member

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.

@milancurcic
Copy link
Member Author

@Carltoffel I agree but suggest to do it in a separate PR to make this easier to review.

Copy link
Member

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

Copy link
Member

@ghbrown ghbrown left a 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!

Copy link
Member

@14NGiestas 14NGiestas left a 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

Copy link
Contributor

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

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.

I support this PR and change. Thanks.

Copy link
Member

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

Copy link
Member

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

Copy link
Contributor

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

@milancurcic
Copy link
Member Author

If there are no objects, I will apply the outstanding fixes and merge on Monday, May 23.

Copy link
Member

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

@milancurcic
Copy link
Member Author

@certik, great, thanks for the encouragement. Will merge now.

@ivan-pi
Copy link
Member

ivan-pi commented Jun 3, 2022

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
build: make Issue with stdlib's manual makefile build
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Should we continue maintaining the manual Makefiles?