Add support for Makefiles and generic build scripts #99

everythingfunctional · 2020-06-15T00:11:07Z

No description provided.

everythingfunctional · 2020-06-15T00:33:29Z

I think I might need some help from somebody who understands Windows a bit better to help me with the issue there. I'm guessing either something with the absolute paths is screwing up make, or the environment variables aren't working properly. I'm leaning towards the former, but I don't have access to a Windows machine to test it with at the moment.

certik

I think this looks great. We have to fix the Windows error, but otherwise that's what we talked about.

certik · 2020-06-15T00:58:39Z

test/example_packages/makefile_complex/Makefile

@@ -0,0 +1,9 @@
+INCLUDE_FLAGS = $(addprefix -I,$(INCLUDE_DIRS))
+
+$(BUILD_DIR)/libmakefile_complex.a: $(BUILD_DIR)/wrapper_mod.o


I think we need to prefix all the environment variables with FPM_, so:

FPM_INCLUDE_FLAGS

FPM_BUILD_DIR

etc.

The ones I used are actually the standard environment variables for Makefiles. You (almost) can use an empty Makefile and these would actually work. We can use prefixed variables, but it's one extra difference from what a lot of people will already have and be used to.

This seems fine to me - FC and FFLAGS are standard Makefile variables - and I think we should be minimising the amount of changes people have to do to their Makefiles for compatibility.

certik · 2020-06-15T00:59:01Z

test/example_packages/makefile_complex/fpm.toml

+with_makefile = { path = "../with_makefile" }
+
+[library]
+source-dir = "src"


This is the default, isn't it? So it can be removed?

Right now if you have a [library] section, it has to contain source-dir. I can make it optional, I just haven't done that yet.

certik · 2020-06-15T01:00:39Z

The windows error is:

# gfortran (for build/gfortran_debug/test/main.o)
# gfortran (for build/gfortran_debug/test/runTests.exe)
 Hello, from test!
mingw32-make: *** No rule to make target 'D:\a\fpm\fpm\test\example_packages\with_makefile\build\gfortran_debug\with_makefile\libwith_makefile.a'.  Stop.
mingw32-make: *** No rule to make target 'D:\a\fpm\fpm\test\example_packages\makefile_complex\build\gfortran_debug\with_makefile\libwith_makefile.a'.  Stop.
mingw32-make: *** No rule to make target 'D:\a\fpm\fpm\test\example_packages\makefile_complex\build\gfortran_debug\makefile_complex\libmakefile_complex.a'.  Stop.

certik · 2020-06-15T01:01:37Z

test/example_packages/with_makefile/Makefile

@@ -0,0 +1,9 @@
+INCLUDE_FLAGS = $(addprefix -I,$(INCLUDE_DIRS))
+
+$(BUILD_DIR)/libwith_makefile.a: $(BUILD_DIR)/hello_makefile.o


I am guessing you cannot use a target like this on Windows.

I suspect it's the D: at the front. Make sees that as the end of the target name. can you put quotes around that in a makefile?

milancurcic

Looks great!

This reverts commit 91a649c.

certik · 2020-06-15T04:20:47Z

I am worried it's going to mess up buildsystems. Cargo is not using such variables either.

…

On Sun, Jun 14, 2020, at 9:53 PM, Brad Richardson wrote: ***@***.**** commented on this pull request. In test/example_packages/makefile_complex/Makefile <#99 (comment)>: > @@ -0,0 +1,9 @@ +INCLUDE_FLAGS = $(addprefix -I,$(INCLUDE_DIRS)) + +$(BUILD_DIR)/libmakefile_complex.a: $(BUILD_DIR)/wrapper_mod.o The ones I used are actually the standard environment variables for Makefiles. You (almost) can use an empty Makefile and these would actually work. We can use prefixed variables, but it's one extra difference from what a lot of people will already have and be used to. — You are receiving this because your review was requested. Reply to this email directly, view it on GitHub <#99 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAAFAWGC5W5YVTCC3G5C4JDRWWLMJANCNFSM4N5VZI5A>.

everythingfunctional · 2020-06-15T04:53:46Z

I am worried it's going to mess up buildsystems. Cargo is not using such variables either.

I'm not sure I understand. The environment variables are only set while fpm is running. Can you give me an example of a situation that might break?

certik · 2020-06-15T05:58:10Z

I'm not sure I understand. The environment variables are only set while fpm is running. Can you give me an example of a situation that might break?

I don't know for sure, but an example would be an autotools build system, that reads these common environment variables, and if they are set by fpm it might break the build. I've definitely seen build systems where things break if you set these variables. So then the user would have to unset them before running autotools.

It seems that prefixing them with FPM_, it would ensure that as we expose more variables in the future, things that already work will continue working.

However, one variable I would set automatically, and that is CMAKE_PREFIX_PATH. I think Spack does it, and that makes any CMake project to easily find all the fpm dependencies without the user having to do anything.

I think your idea is to follow a similar philosophy for raw Make also.

Also as a documentation, it's hard to tell from looking at the script who sets the BUILD_DIR variable. But if it is FPM_BUILD_DIR, then it is clear that it is set by fpm.

I don't know. Let's try this, and keep our options open to possibly prefix it if we run into problems?

everythingfunctional · 2020-06-15T06:16:40Z

My thinking is that if having those environment variables set means your build script doesn't work, that your build script isn't compatible with fpm, and you need a different one for fpm to use. Since the variables aren't persistent after fpm is done running, it won't break existing build systems. They can coexist side by side.

If we use prefixed variables, then users are practically guaranteed to have to write a new build script for fpm to use. If we use commonly used ones, there's a chance their existing one might actually work.

I agree, let's see how this works out, and if we have to change it later, we can.

I'm not all that familiar with CMake, but we can always do different things for different build tools when we add support for them.

LKedward

This is great @everythingfunctional; will really help bring compatibility to existing packages!

Regarding environment variables, adding a prefix actually means more changes to existing Makefiles since I would want to also maintain the existing Makefile behaviour (with FC etc.).

My only minor nitpick is with the compounding of Makefiles and build scripts / executables in the fpm.toml definition - I think we should try to keep the fpm.toml options simple and explicit, and avoid options with 'implicit' rules of the form "OPTION does A except if it contains B then it does C".
I would prefer there be an option to specify a build script and a separate mutually exclusive option to specify a Makefile (and possibly a third for cmake).

LKedward · 2020-06-15T08:15:30Z

PACKAGING.md

+* `FC` - The Fortran compiler to be used
+* `FFLAGS` - The flags that should be passed to the Fortran compiler
+* `BUILD_DIR` - Where the compiled files should be placed
+* `INCLUDE_DIRS` - The folders where any dependencies can be found


The trailing 'S' implies multiple folders can be specified - is this done in a system-dependent manner using colon/semicolon separators? If so, it's worth mentioning that here. (I had thought multiple include paths had to be specified with multiple -I flags.)

They are space separated. So yes, that should be mentioned here, and it is then necessary for your build script to turn this list into the appropriate flags for the compiler(s). I'll add this info.

LKedward · 2020-06-15T08:18:45Z

PACKAGING.md

+Additionally, script will be called with the name of the archive (`*.a` file)
+that should be produced as the command line argument.


Is this a full path to the archive file including BUILD_DIR or just the filename?
Perhaps worth mentioning explicitly that this is only for scripts not Makefiles to avoid confusion.

Yes, it is the full path. And it actually is also for Makefiles. This is the target specified, so your Makefile doesn't necessarily have to have the library as the default target.

Ah I see, that makes a lot of sense. I would suggest also stating that explicitly in the following section about invoking make.

everythingfunctional · 2020-06-15T14:53:04Z

My only minor nitpick is with the compounding of Makefiles and build scripts / executables in the fpm.toml definition

I hadn't really thought about it that hard. It's worth considering. Usually I'd agree, but somehow this seemed simpler from a user and documentation stand point. If you have a custom build process, there's one way/place to specify it, and fpm just knows what to do with it.

@milancurcic or @certik , do you guys have an opinion on this?

certik · 2020-06-15T17:07:21Z

These are all really tough questions with good arguments on both sides. My only suggestion is to be open and open to change things if things don't feel right. I think the way forward is to insist that we are still in a prototype phase, and so things can (and will) change --- let's put this in bold in the README? And in a prototype phase, let's just try it, and get more experience, and be ready to change things. Only after we declare we are in production, we should be backwards compatible as much as possible.

In general, I was hoping that fpm would simply execute the "script" as an executable, and always pass the same environment variables. I feel it is ok for fpm to know how to execute the script properly, so a bash script using bash, a Xonsh script using Xonsh, Make file using make, and an executable as an executable.

I think it is absolutely ok for users to write a short build script. You will have to do it anyway for cmake projects.

milancurcic · 2020-06-15T18:10:22Z

I don't see issues with this. Like @certik said, let's try it and we can revise if there's need for it. We can't think of all correct answers ahead of time without trying things first in practice.

Minor nit pick: As a user I didn't expect that for using make the build_script value would be Makefile or *.mk. build_script = "make" or build_script = "make target" seems more intuitive, because that's what I would've typed otherwise. However, this is also perhaps problematic because neither Makefile nor make are scripts (Makefile is a specification file and make is a command/tool). Perhaps a build_command parameter would be more appropriate if both arbitrary scripts and make are to be invoked with it.

certik · 2020-06-15T19:47:09Z

One issue with build_command is that we need to specify the shell. So it could be Bash on Linux/macOS and PowerShell on Windows. Simple commands like make target would work in both. More advanced things, such as referencing an environment variable will not work cross-platform. Essentially the issue is a lack of cross-platform shell.

Rust / Cargo goes around this by simply writing the "build script" in Rust itself, thus making it cross-platform.

I think there is a huge need for a cross-platform native shell. We even started one: https://github.com/xonsh/minixonsh (Xonsh itself is one, but being a Python program, it has the typical Python issues related to distribution and speed).

But until there is one (and well supported), I think we should not just pick one and require it.

So given all of this, the two option are:

build_script would be directly executed in Bash / PowerShell, so you have to use a subset that works in both, such as make or make target. Or bash my_buildscript.sh.
build_script points to a script or executable that gets executed. So Makefile gets executed via make, as fpm knows this.

I still think 2. seems better.

But overall, I don't know what the best way forward is. So let's just try something to get moving, and iterate.

everythingfunctional · 2020-06-16T02:48:48Z

I think there will be some packages which are just not supported on all platforms, and that's ok. As a community we can encourage that and provide support when we're able, but that's going to be a big hurdle in some instances.

As everybody seems to agree, let's try this PR out and see how it goes.

Brad Richardson added 2 commits June 14, 2020 16:52

Add support for Makefiles and generic build scripts

16daa62

Add info on custom build scripts to Packaging Guide

778fb0e

everythingfunctional requested review from LKedward, certik and milancurcic June 15, 2020 00:11

certik reviewed Jun 15, 2020

View reviewed changes

Try escaping the colon in buildscript BUILD_DIR

91a649c

milancurcic approved these changes Jun 15, 2020

View reviewed changes

Brad Richardson added 5 commits June 14, 2020 19:45

Revert "Try escaping the colon in buildscript BUILD_DIR"

ac01e26

This reverts commit 91a649c.

See if removing the drive letter works

100d30c

Try using Linux like paths for build scripts

5fae72c

Fix missed unWindowsPath

a124c42

Fix another path issue for build script

ec8855d

LKedward approved these changes Jun 15, 2020

View reviewed changes

Add clarifications to PACKAGING.md

df531fc

everythingfunctional merged commit 410537b into fortran-lang:master Jun 16, 2020

everythingfunctional deleted the MakefileSupport branch June 19, 2020 02:20

ivan-pi mentioned this pull request Jun 21, 2020

Projects with multiple drivers #105

Closed

		@@ -0,0 +1,9 @@
		INCLUDE_FLAGS = $(addprefix -I,$(INCLUDE_DIRS))

		$(BUILD_DIR)/libmakefile_complex.a: $(BUILD_DIR)/wrapper_mod.o

		@@ -0,0 +1,9 @@
		INCLUDE_FLAGS = $(addprefix -I,$(INCLUDE_DIRS))

		$(BUILD_DIR)/libwith_makefile.a: $(BUILD_DIR)/hello_makefile.o

		Additionally, script will be called with the name of the archive (`*.a` file)
		that should be produced as the command line argument.

Add support for Makefiles and generic build scripts #99

Add support for Makefiles and generic build scripts #99

Uh oh!

Conversation

everythingfunctional commented Jun 15, 2020

Uh oh!

everythingfunctional commented Jun 15, 2020

Uh oh!

certik left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

certik commented Jun 15, 2020

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

milancurcic left a comment

Choose a reason for hiding this comment

Uh oh!

certik commented Jun 15, 2020 via email

Uh oh!

everythingfunctional commented Jun 15, 2020

Uh oh!

certik commented Jun 15, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

everythingfunctional commented Jun 15, 2020

Uh oh!

LKedward left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

everythingfunctional commented Jun 15, 2020

Uh oh!

certik commented Jun 15, 2020

Uh oh!

milancurcic commented Jun 15, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

certik commented Jun 15, 2020

Uh oh!

everythingfunctional commented Jun 16, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

certik commented Jun 15, 2020 •

edited

Loading

milancurcic commented Jun 15, 2020 •

edited

Loading