Skip to content

Bootstrap submodule support #213

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

Conversation

everythingfunctional
Copy link
Member

@everythingfunctional everythingfunctional commented Oct 22, 2020

This implements support for submodules in the Haskell (bootstrap) version of fpm. A quick note that this removes any requirements or restrictions on naming conventions for modules and source files. Those could in theory be added back in as explicit checks somewhere.

Additionally, this should now work with multiple programs in the same folder. I haven't really tested that yet, but it should work.

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.

There are binary files checked into this branch, (several object files, some submodule files and an archive). Those should be removed from the commit history.

@everythingfunctional everythingfunctional force-pushed the bootstrap_submodule_support branch from 739ab2b to 03c9efc Compare October 22, 2020 21:44
@everythingfunctional
Copy link
Member Author

There are binary files checked into this branch, (several object files, some submodule files and an archive). Those should be removed from the commit history.

Not sure how I missed that. Fixed.

@LKedward LKedward linked an issue Oct 23, 2020 that may be closed by this pull request
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.

Great stuff @everythingfunctional! I've played around with it and everything is working as expected! Nice to see non-interface changes don't propagate to child submodules as you discussed. Also glad the example packages have been moved to repo root, this is a better location for them.

I'm not qualified to make specific comments on the Haskell code, but the new unit tests look sound and I haven't been able to break any of the parsing. 👍

@certik
Copy link
Member

certik commented Oct 23, 2020

Great work, thank you.

Why were tests moved out of the tests directory directly into top level? I would like to have a tests directory I think.
In general, the PR would be a lot easier to review if such unrelated changes were not done in the same PR.

Consequently, I hope I didn't miss it, it seems there is no test for submodules in the (no removed) tests directory. I think there might be a test for this in the Haskell version, but I think we should strive for providing tests in the tests directory, so that the Fortran version can reuse them.

@everythingfunctional
Copy link
Member Author

@certik , I moved the examples out of the tests folder, because I thought it strange that they were in the fpm/tests folder, but were still used (and in fact there was a softlink) by the bootstrap version. Also, I find the examples as useful for more than just testing the code; they are examples of how one might organize their own projects to work with fpm, and should thus be much more prominently available (i.e. at the top level). We could put a softlink in the tests folder if you like.

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.

Works quite robustly, nicely done.

@LKedward
Copy link
Member

The example packages are more useful as demonstrations of the capabilities of fpm to potential and existing users/developers than as actual tests. We need them currently for end-to-end regression testing but their usefulness to identify bugs or general regressions is quite limited.

@certik
Copy link
Member

certik commented Oct 24, 2020 via email

@LKedward
Copy link
Member

It's useful to have examples, no doubt. But more importantly we need tests. I thought these were a good start. How should our testsuite look like?

Yes, I certainly didn't mean to imply that examples are more important than tests. Rather, I want to point out that we already have a fairly comprehensive set of unit tests which give much better code coverage and diagnostic capability than building the example packages (though I agree that we absolutely still need to do this for end-to-end and regression testing).


I would like to have a tests directory I think.

The reason this test directory isn't in the root directory is because fortran fpm is in a sub-directory - this doesn't need to be the case. Indeed, fpm assumes other fpm packages exist in the top-level directory, why should fpm be any different? We can still keep the bootstrap version separate in a sub-directory.

@everythingfunctional
Copy link
Member Author

Glad you guys are happy with the new functionality. Is there anything more you would like done in this PR before merging in?

@LKedward
Copy link
Member

+1 to merge from me!

@everythingfunctional
Copy link
Member Author

Ok, with 3 approvals I'm going to go ahead and merge. Feel free to create an issue and tag me if you'd like additional changes after.

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.

Support Fortran submodules
5 participants