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: external-modules key to build table for non-fpm modules #438

Merged
merged 6 commits into from
Apr 16, 2021
Merged

Add: external-modules key to build table for non-fpm modules #438

merged 6 commits into from
Apr 16, 2021

Conversation

LKedward
Copy link
Member

Resolves #405 by allowing external modules to be specified in the manifest.

e.g.

[build]
external-modules = "netcdff"

See also: #377 (comment), fortran-lang/fpm-registry#39 (comment)

@milancurcic
Copy link
Member

This feature is a game changer for me, thanks a lot for implementing it.

How to specify the include path in which to search for modules? Is it in manifest, for example

[library]
include-dir = "/opt/netcdf-fortran/include"

Or do I need to pass -I/opt/netcdf-fortran/include as flags on the command line?

@awvwgk
Copy link
Member

awvwgk commented Apr 12, 2021

Absolute paths should never enter the package manifest. For netcdf-fortran, there should be a way to find it with pkg-config using

pkg-config netcdf-fortran --cflags

For the time being, --flag "$(pkg-config netcdf-fortran --cflags)" would work, but eventually fpm must support something like pkg-config to find (external) dependencies.

@milancurcic
Copy link
Member

Absolute paths should never enter the package manifest.

Why? That's quite a strong statement. Assume that I don't share my manifest and that pkg-config doesn't work for me. What's my next best solution?

It's a different question whether absolute paths should appear in manifests that are meant to be distributed (I agree that the answer is no).

Passing a path via --flag is okay, though not as convenient in the long run as it is to be able to set it in a file and not worry about it when building.

@awvwgk
Copy link
Member

awvwgk commented Apr 12, 2021

I fear we are derailing this thread towards build system design questions. Let's discuss this at #439.

@LKedward
Copy link
Member Author

Yes, using include-dir should work for non-distributable projects and --flag I... is currently the best distributable option but is unfortunately very verbose (don't forget you also have to specify --profile if you want default profile flags when using --flag). It's a shame there isn't an compiler environment variable that could be set for specifying Fortran mod locations; perhaps an fpm environment variable instead? Otherwise providing in-built support for pkg-config or cmake (#439) seems like the only option.


Aside: I do wonder how much work it would be to just write an fpm package with module interfaces for these system-installed libraries?

@milancurcic
Copy link
Member

I just played with it and was able to compile and run (using --flag), with a caveat: I can only run if I specify all the flags on the CLI:

LIBRARY_PATH=$NETCDFLIB fpm build --flag "-I$NETCDFINC -fcoarray=single"
LIBRARY_PATH=$NETCDFLIB fpm run --flag "-I$NETCDFINC -fcoarray=single"

However, if I do just fpm run (as I naively expected should work), fpm begins a new build under a new unique hash profile (I assume because the flags have changed). I also couldn't set the specific build hash as the profile. But that's for another issue and not related to this PR.

Yes, using include-dir should work for non-distributable projects

Do you mean that this should work now with this PR, or just that you support it working in the future? I tried it and it didn't seem to make a difference.

@LKedward
Copy link
Member Author

However, if I do just fpm run (as I naively expected should work), fpm begins a new build under a new unique hash profile (I assume because the flags have changed).

Yes this has always been the case (since Haskell fpm) because each invocation of fpm should be independent of previous ones (stateless). The situation for command line flags will be improved somewhat with response file support coming in #364.

Do you mean that this should work now with this PR, or just that you support it working in the future? I tried it and it didn't seem to make a difference.

Yes it should work with this PR as is because include-dir just adds to the compiler flags.

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.

This is a good step forward already. We can refine the external dependency handling later.

@milancurcic
Copy link
Member

Yes it should work with this PR as is because include-dir just adds to the compiler flags.

Hmm, I just tried it again and can't seem to get it to work:

$ tree .
.
├── app
│   └── main.f90
└── fpm.toml

1 directory, 2 files
$ cat app/main.f90 
use netcdf
end
$ cat fpm.toml 
name = "test-include-dir"

[build]
external-modules = "netcdf"

[library]
include-dir = "/usr/include"
$ which fpm
/home/milan/.local/bin/fpm
$ fpm-ext-mod build
 + mkdir -p build/dependencies
 + mkdir -p build/gfortran_2A42023B310FA28D/test-include-dir
 + gfortran -c app/main.f90 -Wall -Wextra -Wimplicit-interface -fPIC -fmax-errors=1 -g -fcheck=bounds -fcheck=array-temps -fbacktrace -fcoarray=single -J build/gfortran_2A42023B310FA28D/test-include-dir -I build/gfortran_2A42023B310FA28D/test-include-dir -o build/gfortran_2A42023B310FA28D/test-include-dir/app_main.f90.o
app/main.f90:1:4:

 use netcdf
    1
Fatal Error: Can't open module file ‘netcdf.mod’ for reading at (1): No such file or directory
compilation terminated.
...

Do you know what I'm doing incorrectly?

@LKedward
Copy link
Member Author

Do you know what I'm doing incorrectly?

Apologies, I forgot that include-dir is specified as a path relative to the project top-level directory; so the absolute path doesn't work here since fpm can't find it in the project directory.

manifest-reference.md Outdated Show resolved Hide resolved
@milancurcic
Copy link
Member

Okay, that's great to know. I often make dependency links inside project directories, so this works well for me.

The error message should be improved, or perhaps just detect and error out if a path is absolute. I'll open a separate issue or PR for this.

Co-authored-by: Milan Curcic <[email protected]>
manifest-reference.md Outdated Show resolved Hide resolved
@LKedward
Copy link
Member Author

Thanks for reviewing everyone. I'll merge later today if there are no more comments.

@Carltoffel
Copy link
Member

Thank you for this feature!
I'm not done yet to make it work, but I'm confident I will make it.

I had problems figuring out how to use the external-module key, as I was using the prebuild binary in which this feature is not included yet. I know it is still a very experimental project, but maybe it would be a good thing, to keep the documentation somehow consistent with the published binary. Maybe use milestones or mention the target release in the documentation.
I am still very new to this project, but maybe you can help to me get involved. For example I could help to improve the experience for new users, because I am still fresh and don't have organisational blindness (yet).

Also, I found a incomplete comment: The external-module key is not mentioned in the possible fields in the build.f90

@LKedward
Copy link
Member Author

LKedward commented May 1, 2021

Many thanks for the feedback @Carltoffel. It definitely helps to get a fresh pair of eyes from a user perspective and you raise a good point. This may perhaps be good motivation to start using a separate development branch so that the default branch remains 'in sync' with the current release. Do feel free to open issues and pull requests if you have ideas for how to improve the documentation.

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.

fpm(1) should produce a message not stop when it cannot find the source for a module
4 participants