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

Add objects for handling compiler and archiver #527

Merged
merged 8 commits into from
Aug 25, 2021

Conversation

awvwgk
Copy link
Member

@awvwgk awvwgk commented Jul 30, 2021

Adds some abstraction for the compiler and archiver to remove the logic from the backend.

@awvwgk
Copy link
Member Author

awvwgk commented Jul 30, 2021

This partly addresses #354 by allowing to detect the compiler command inside an MPI wrapper.

@certik
Copy link
Member

certik commented Jul 31, 2021

I am not very good with fpm internals yet. Are you looking for a thorough review? I would be able to do it next week. Feel free to merge if somebody else can do a better job sooner.

@awvwgk
Copy link
Member Author

awvwgk commented Jul 31, 2021

I'm currently more looking for feedback on the preliminary MPI support. Just pinged you and Milan, since you were involved in #354. Let me know if you can break it.

@LKedward
Copy link
Member

Thanks for starting this Sebastian.

I am not very good with fpm internals yet. Are you looking for a thorough review?

I might be a better reviewer to request for changes to the model and backend. This generally all looks good as per your previous PR and this is definitely the direction we want to go with the backend.

However I think the timing may be a little unfortunate here; I'm concerned that this overlaps significantly with Jakub's work in #498. Given the stage that Jakub is at with his project and with only a few weeks left, can we hold off on this until Jakub has finished? I'm confident that you'll be able to pick it up once #498 is completed, but I don't think the other way around would be fair if that makes sense?

@awvwgk
Copy link
Member Author

awvwgk commented Jul 31, 2021

I did a check against Jakub's PR earlier, there is some overlap, in the sense that we will probably run into a merge conflict in src/fpm.f90, but the changes are similar if not identical in intent (like the build_name removal from the CLI module) and should be straight-forward to resolve. Otherwise the two patches are orthogonal enough to move forward separately.

There are some issues I encountered in the backend, which require fixing at some point (like -I flag for all compilers while Intel on Windows would require /I), but I didn't touched those yet as they would definitely collide with Jakub's work.

@awvwgk awvwgk self-assigned this Aug 12, 2021
@certik
Copy link
Member

certik commented Aug 23, 2021

I tested this PR with LFortran and I can confirm that it works. Thank you @awvwgk. It seems to still work with gfortran.

What else is needed to get this merged?

@certik
Copy link
Member

certik commented Aug 23, 2021

After this is merged in master and a package in Conda, I plan to set this up at LFortran's CI on all platforms, so that we can test that simple projects work everywhere. So for LFortran's development it would be very good to get this merged. We can help Jakub to resolve conflicts in his PRs.

@awvwgk
Copy link
Member Author

awvwgk commented Aug 23, 2021

It's mainly a refactor with some straight-forward feature additions (MPI support and now also LFortran support). I'm still hoping for feedback on the initial MPI support before starting with the actual review of the refactoring.

@certik
Copy link
Member

certik commented Aug 23, 2021

How can I test the MPI support? I am happy to test.

@awvwgk
Copy link
Member Author

awvwgk commented Aug 23, 2021

How can I test the MPI support? I am happy to test.

Just set --compiler mpifort or whatever and add

build.external-modules = ["mpi", "mpi_f08"]

to the package manifest and you should be ready to go.

Running executables/tests requires to the --runner argument with mpiexec or mpirun to work or you get the sequential version.

- relevant for conda-forge's gfortran on OSX which comes without C compiler
@certik
Copy link
Member

certik commented Aug 25, 2021

Do you know if there is a way to install mpi using Conda on macOS? I tried to install the packages mpi, openmpi, but none of them provide mpirun or mpiexec.

I also tried spack but openmpi requires Fortran, and spack couldn't find Fortran. Installing gfortran using spack fails: spack/spack#25598.

@certik
Copy link
Member

certik commented Aug 25, 2021

@awvwgk, @LKedward, @milancurcic. This PR is of utmost importance for LFortran as it makes fpm working with LFortran. We want to release LFortran's MVP before FortranCon. For that, we need fpm and LFortran to work really well together. I need to setup a CI infrastructure on all platforms (Linux, macOS, Windows) and ensure things work with fpm. So I need an fpm release in Conda. I want to update our documentation etc. So I need at least 2 weeks before FortranCon, and time to get this all setup.

So I would like to get this PR merged as soon as possible and make an fpm release. It's a blocker for me.

What do we need to do to merge this? It seems to me this is good enough to merge, and we can improve upon it in master.

@milancurcic
Copy link
Member

I will test this now. Sorry for the delay.

@milancurcic
Copy link
Member

It seems to work well:

$ cat fpm.toml 
name = "test-mpi"
version = "0.1.0"
license = "license"
author = "Jane Doe"
maintainer = "[email protected]"
copyright = "Copyright 2021, Jane Doe"

[build]
external-modules = ["mpi"]
$ cat app/main.f90 
program test_mpi
  use mpi
  integer :: ierr, num_procs, this_proc
  call mpi_init(ierr)
  call mpi_comm_rank(MPI_COMM_WORLD, this_proc, ierr)
  call mpi_comm_size(MPI_COMM_WORLD, num_procs, ierr)
  print '(2(a,i2))', 'Hello from MPI process', this_proc, ' of ', num_procs
  call mpi_finalize(ierr)
end program test_mpi

$ fpm_mpi run --compiler mpif90 --runner "mpiexec -n 6"
Hello from MPI process 1 of  6
Hello from MPI process 2 of  6
Hello from MPI process 3 of  6
Hello from MPI process 4 of  6
Hello from MPI process 0 of  6
Hello from MPI process 5 of  6

I only skimmed through the code changes, but no thorough review. I'm not very familiar with the backend, but I trust @awvwgk with that.

I also recommend going forward with this in interest of time for LFortran, and then work on resolving any conflicts when @kubajj's PR is ready.

Copy link
Member

@milancurcic milancurcic left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you!

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.

The code looks good enough to me to merge into master, all tests pass. We can improve upon it in master.

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.

Thanks Sebastian, LGTM! 👍

As we discussed, help with resolving conflicts in Jakub's branch would be most appreciated after merging. Is there anything else to go into this @awvwgk?

@awvwgk
Copy link
Member Author

awvwgk commented Aug 25, 2021

Thanks a lot for the swift review. I'll go ahead and merge now, in case anything breaks I'll be around to help fixing it again ;).

@awvwgk awvwgk merged commit b69042d into fortran-lang:master Aug 25, 2021
@awvwgk awvwgk deleted the compiler-object branch August 25, 2021 19:44
@certik
Copy link
Member

certik commented Aug 25, 2021

Thank you! Let's test this for a while and maybe do a release on Friday? So that it gets into Conda.

I am making progress on compiling fpm examples with LFortran: https://gitlab.com/lfortran/lfortran/-/issues/540

@awvwgk
Copy link
Member Author

awvwgk commented Aug 26, 2021

Thank you! Let's test this for a while and maybe do a release on Friday? So that it gets into Conda.

No, I don't think this is a good point to create an fpm release. I'd like to have the compiler profiles ready before the next fpm release. Also, I think the partial LFortran support in fpm would do more harm than good if the LFortran version on conda doesn't work yet (see #545). We can of course coordinate the next LFortran and fpm release.

If you want to have an installable fpm version for CI testing we can possibly come up with something workable here.

@milancurcic
Copy link
Member

milancurcic commented Aug 26, 2021

Though I understand, and don't disagree with, @awvwgk's position, I do recommend releasing early and more often, even if some things are broken. It's okay. They're supposed be broken in alpha. Releasing early and often keeps engagement and excitement about fpm going. One more thing for people to talk about and share. Last release was 2 months ago.

I can't comment on when exactly to release this as it's likely not be me doing it. :)

@awvwgk
Copy link
Member Author

awvwgk commented Aug 26, 2021

Fair point, we can talk about releasing in the next weeks if we have enough features together and whether we aim with the profiler profiles for this version or the next one.

At least from my side, it won't happen tomorrow. I think Laurence is away and I'm at a conference with limited availability next week as well. I just think this is not a good point to make a release.

@certik
Copy link
Member

certik commented Aug 26, 2021

As far as fpm is concerned, it calls LFortran using the API that is described in the document updated in this PR. It is LFortran's job to deliver. The master of LFortran works, and we can release anytime. I am happy to release today if you need an LFortran release first.

Regarding an fpm release, I want things to work by FortranCon on September 23. Realistically, I want everything in Conda at least 2 weeks before, working on all platforms (as far as we know), so that people can test it out and report bugs and we can fix it. So both LFortran and fpm has to work in Conda by Sunday September 5. As I mentioned above, I need about a week before that to ensure our CI works and to fix up any issues which I expect there will be especially on Windows. So I need the first fpm Conda package by Sunday August 29. This particular weekend I'll be busy, so I want to release fpm this Friday. Any later than that and it's going to be really tough time wise.

I volunteer to do the fpm release. Is it documented somewhere how to do that? If not, I will do my best.

As @milancurcic said, we need to be releasing early and often. I 100% agree with that. The main advantage is that you don't have to "possibly come up with something workable here.". We just do the release. That's it. Otherwise it is more work not just for you, but for me also --- getting the CI working on all platforms is a lot of work (for example by installing gfortran on Windows first and compiling fpm from source, the same on macOS, and then uninstalling gfortran to ensure cmake only finds LFortran to compile our tests, etc. --- a lot of possible bugs to be worried about), and then I have to redo it with the official Conda fpm release. It is truly more work for everybody. If, on the other hand, we release often, then I simply set it up once. Updating a version from the same conda-forge repository is then easy.

It is in the benefit of everybody for LFortran and fpm to work well together, and the above approach is the easiest how to get there. Furthermore, I need a build system on Windows that works, and I can't get cmake to work with LFortran. I need this just for LFortran to ensure that things work well on Windows. Fpm turned out to be the easiest way to get there.

@awvwgk
Copy link
Member Author

awvwgk commented Aug 26, 2021

Allright then, I went ahead and opened #546 for a version bump.

@awvwgk
Copy link
Member Author

awvwgk commented Aug 26, 2021

@certik I see that this is an important and urgent issue for you, therefore let's just go ahead and make this release. Sorry for holding this back earlier.

I'm currently just a bit short of time and have a lot of other things that require my attention as well. I'll try to get my head free and look into the expected issues with the conda-forge build, but I can't make any promises, unfortunately.

@certik
Copy link
Member

certik commented Aug 26, 2021

Thanks a lot for this!

I'll help with Conda builds of fpm. This is the work that makes sense for me to do, as I need it, and fpm needs it, long term. Let's worry about it after we release. We can easily add patches in the code package if we need to modify anything in fpm, I've done it for LFortran many times. The patch then gets incorporated into the next upstream release.

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.

4 participants