More issues for new

[urbanjost]

In regards to the Fortran implementation of fpm(1) I believe this
should close

• fpm new should do --with-executable by default #109 fpm new should do --with-executable by default
• Rename --with-executable to --bin (or --app) #110 Rename --with-executable to --bin (or --app) enhancement
• Introduce --lib to initalize a project with a library #111 introduce --lib to initalize a project with a library
• Command line interface (CLI) #135 Command line interface (CLI) fpm-fortran
• fpm run --args "a b c" should become fpm run -- a b c #138 fpm run --args "a b c" should become fpm run -- a b c
• Implement fpm new #154 Implement fpm new fpm-fortran
• not all directory names are valid project names #196 not all directory names are valid project names

and show

   fpm help SUBCOMMAND

extended to

   fpm help SUBCOMMAND|FORTRAN_LANGUAGE

This integrates Fortran documentation into the CLI in a platform-independent
manner. In addition to installing packages fpm(1) already is well on it's
way to replacing make(1)-like utilities and masks using ar(1), compiling and
loading and creating static libraries. Some integration with debuggers,
profilers, and automated reformatters seems obtainable. Maybe it should be
be renamed "fip" for "Fortran Integration and Packaging". It is a lot easer
to type.

[LKedward]

Great work @urbanjost. I really like the cross-platform solution for including the Fortran man pages! I will look over in more detail later. Looks like you need to update your CLI tests?

[urbanjost]

I will put in the matching tests if the initial response is to go forward with the changes later today. If there were not so many nuances to color and terminal emulators I would put in the color version. That was just a quick "what if" idea but I liked the results.

[urbanjost]

It is hitting what is reported as a compiler bug in the log, which may have to do with MSWindows filenames. I renamed the git(1) repository to test if the name was the issue but from the log files it does not appear that the build is pulling the remote dependency packages. Is there a way I can clear the test to get a full build from scratch? I am hoping the filename is the issue as it passes on Linux and Mac.

[urbanjost]

Since --test creates test/ and --app creates app/ anyone feel that it should be --lib and lib/ and --src and src/? I checked some other package managers and --lib and src/ seems common and lib/ is usually reserved for object files so if changed it seems --src and src/ would be a more intuitive naming if this was being done in a vacuum; but that other package managers that others may be familiar with seem to use --lib and src/ from what I have seen so far. That seems to be worth some weight (?).

[LKedward]

I've built this PR on one of my Windows setups (MSYS2, MinGW-w64, gfortran 9.2.0) and I can't reproduce the internal compiler error - everything builds and tests pass. The new functionality appears to be working as well. I'm not sure what is different between my MinGW installation and that of the CI, perhaps a different gcc version? Without reproducing the error, I'm stuck for a solution.

An ICE is always a compiler bug, but I am highly doubtful that it is an issue with the Windows filenames since this would almost certainly produce a meaningful error message from the compiler and I don't see how it could affect compiler internals. My guess is that the Windows CI is using an older version (8?) of gfortran that has since been fixed.

Is there a way I can clear the test to get a full build from scratch?

With the exception of some caching for Haskell and h-fpm, the CI builds are always completely from scratch - there is no state preserved between CI invocations. This means that the git repositories are being fetched correctly since we can see the corresponding source files being compiled.

---

Edit: a similar ICE bug reported here https://gcc.gnu.org/bugzilla/show_bug.cgi?id=92065

[LKedward]

Okay, I've reproduced the ICE outside of MSYS2, using plain MinGW (gfortran 8.1.0). I will have a look into it today.

[arjenmarkus]

There is a new distribution of MinGW/MSYS2 that offers gfortran 10.2 (MinGW) and 9.2 (MSYS2). Perhaps the ICE as been solved with these versions Op do 8 okt. 2020 om 11:19 schreef Laurence Kedward < notifications@github.com>:

…

Okay, I've reproduced the ICE outside of MSYS2, using plain MinGW (gfortran 8.1.0). I will have a look into it today. — You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub <#204 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAN6YR2LWS56BVXCGW4AHYLSJV7ZXANCNFSM4SHAJCVA> .

[LKedward]

I think I've found a fix @urbanjost : in package fortran-intrinsic-manpages, in file M_intrinsics.f90, change line 104 from

character(len=:),allocatable   :: textblock(:)

to

character(len=132),allocatable   :: textblock(:)

---

Thanks @arjenmarkus, I need to update my MSYS2! I think you're probably right that this has been fixed after version 8.
We should probably add a separate target for MSYS2 with gfortran-9 in our CI checks for fpm.

[urbanjost]

Thanks @LKedward. Unless the compiler was upgraded that seems to do it if I sent the length of allocatable character arrays.

[urbanjost]

During the refactoring I left out the patch to actually use the --backfill switch and to create a sample program not requiring a module if on fpm new NAME --app is used, apparently. When I pulled down the push request that was missing. Been a while so maybe I added that after the PR. Do the tests run in such a way that if I make a test program that actually runs 'fpm new' and checks for the existence of the output files will it clean up or allow that?

[LKedward]

Yes, what you describe is allowed by the tests. You are free to create files during tests, these files cease to exist once the test completes.

[LKedward]

This might not work as intended: list_files returns filenames with the directory name prepended. Easy fix with basename to strip it before checking for .

[urbanjost]

definitely differences and some unexpected differences. MSWindows output includes hidden files, Linux does not; was working around that rather than changing list_files but might want to make it consistent; etc. If it is not causing any problems this is probably going to take several iterations. I do not have a MSWIndows environment.

[LKedward]

Yes I agree, we should change list_files to be consistent.

[urbanjost]

Well, I think I found
list_files shows hidden on MSWindows, does not on Linux. Not sure if that should be an option, if both should show hidden, or both no show hidden. Thinking default is not to show with optional parameter to show it?
mkdir on Linux silently will remake an existing file, on MSWindows remaking an existing directory is fatal. Since there is an exists and isdir function that makes it cleaner I was going to work around that. Apparently, git(1) is not in the path on the MSWindows system? If it is I do not know why call run(cd DIRNAME;git init) is failing. OK for this test; but if not there
I was looking at cargo and it has a switch to select which CVS to use and allows for none that I could add quickly (I do not need that but would like it to work just to verify it would but even if the problem is no git the -cvs switch might be good to add anyway.

[urbanjost]

PS; might be something really simple. I have not kept up with all the development on the build command lately; was waiting till that settled down but run and test do not work with several of my fpm(1) packages. I am either asking for the wrong locations now (it worked and I think still does with running something made with h-fpm so something is different) or something is wrong.

[LKedward]

Are you able to point me to any of the packages that aren't building with f-fpm for me to have a look at?

[urbanjost]

The main part builds, it is running it or building the test/ file that is not working as I hoped. The file structure is odd to accomodate my own build method, being on github with a build using make(1) and h-fpm(1) and trying to add f-fpm(1). I would prefer putting the app/ files back in a single directory which f-fpm supports (Thanks!) but that would currently break h-fpm(1). So the issues for this one (M_color) are that
h-fpm allows me use #include to merge several modules into a file (f-fpm was not supporting remote dependencies and I needed something that would work with make(1) as well and that worked). So if for the time being I move the test directory to test.x and remove the test entries from fpm.toml it then builds the M_color module and builds the applications but puts them in build/debug/app where h-fpm put them in build/debug/PROGRAMS/. So they build, but the run command is looking for them where h-fpm put them; so I am not sure if it is build or run that needs changed to match. I think that resolving this one would fix several more complex ones, although they are similar. I have several non-typical layouts in the fpm registry that might make good tests and are easily accessible. I prefer your new layout capabilities but hate to break repositories with respect to h-fpm as there are users out there that might be testing -- but I think some of them "just" need the files re-arranged. So I pushed out M_color to show the problems I hit for now; hopefully without breaking anything. It is a simple module with multiple application demos and one unit test, but with a non-default name to accommodate the other methods.
Thanks for offering to take a look.

[LKedward]

Ah okay I understand your problem. Yes side-by-side use of f-fpm and h-fpm is problematic at the moment (I haven't been strict with keeping to h-fpm output convention), but it should still be possible to publish packages that are compatible with both (which I understand is important at this stage).

To answer your question, it is the run command that needs to change in this PR because currently f-fpm always puts programs in build/gfortran_<release>/app regardless of where the source file is located.

I've run into another issue while trying M_color with f-fpm; it's an already-known limitation of module dependency resolution in f-fpm whereby it does't check include files for module dependencies - not too difficult to fix, so I might address it this week.

[LKedward]

Many thanks for addressing my previous points and bringing this back into one PR.
I'm now happy with the changes. As I said on the old PR, everything works as expected when tested locally.

A few final points from me:

• The --help, --version, and --usage flags print "displayed help|version|usage text" at the end - can this be removed?

• The --help, --version, and --usage have non-zero exit code whereas fpm help ... does not - do we need a non-zero exit code for these flags? (Apologies if this has already been discussed)

• Can you update ci/run_tests.sh and ci/run_tests.bat to cleanup the test directories created by new_test?

• I realise new_test is incomplete from my suggestion because it doesn't check for files that shouldn't be there; I've open #208 to fix the issues you raised (thanks!) after which we should be able to update the test accordingly.

[urbanjost]

I changed the test scripts so remove the scratch files and gave the project a name of fpm_scratch_* to avoid inadvertently removing directories. The names were simple names that might possibly exist for some other reason. I could just add the commands to the test.
I added the specific REV for the intrinsics. I think the messages and STOP calls you are seeing are coming from a cached external dependency. There was a version where STOP and messages were added to explore other ways of testing the CLI module that were suggest here and because at that time f-fpm was doing stops and tracebacks for development purposes (I assume) that I was matching; but that should not match the REV in the TOML file and I do not see the messages in the webpage test logs or on my own machines. We do not have a "-refresh" function for external dependencies yet (at least in my version) so is that why you see the STOP and messages?

[LKedward]

I could just add the commands to the test.

Yes, if you can put the cleanup commands in the test program instead that would be better actually!

I think the messages and STOP calls you are seeing are coming from a cached external dependency.

Yep you are right, they went away when I built from scratch. Cheers!

[awvwgk]

Is there a way to reduce the whitespace changes in this PR?
Could we change the help printout to something like this instead?

[character(len=80) :: &
'NAME', &
'    build(1) - the fpm(1) subcommand to build a project', &
...
]

I think it would still work since we declare the length in the array constructor, but save us repeated whitespace changes and large diffs.

[urbanjost]

I use a preprocessor to generate the character variable declarations as Fortran sadly lacks a block text option, so I generally really do not manually generate the white space changes. Does github have typically diff(1) options to optionally ignore whitespace differences? I could change the preprocessor to always write the same length as an option so they come out the same width all the time, which would probably help.

[urbanjost]

Not sure if there is anything else I need to do or not? Is there a consensus on the intrinsics? On the pro side it is intended as an additional feature to promote use of f-fpm. It is an out-of-the-box feature that promotes discussion on just what f-fpm can be -- is it purely a source package manager or is it a general interface for all things Fortran? I would argue it could be a separate tool in a set of tools that could be accessed via f-fpm but there is not a mechanism for doing that yet. Should that be a future feature for everything like simple tools like a Fortran manual, fsplit90, dos2unix-like and expand-like programs to more elaborate features like formatters an auto-documenation tools? Anything that makes development easier an promotes better code seems fair game. I am open to removing it especially if there is momentum behind providing a simple "tools" utility although I have been finding I am using it quite often myself. I do not have a simple solution for the white-space issues other than I keep seeing mention of being able to toggle diff options on an off with github including ignoring white-space differences but the examples do not match up with anything I found on the github pages so far (although I did not look very long yet). I think I addressed the other issues and would like to wrap this one up and clean up some issues with run and test. They need changed to match up with the differences between f-fpm and h-fpm and so on and make room for the other changes like working with remote dependencies to proceed. Anything else? This is getting very close to implementing all the basic functionality except for conditional recompilation and environment variables for customized builds and so on. It would be exciting to get this to the point of a functioning tool for people to try.

[awvwgk]

@urbanjost You raised a fair point, fpm in both bootstrap and Fortran variant are still lacking features. In case of the intrinsics the desired functionality is installing documentation as described in #195.

Providing a basic install command to Fortran fpm would be the more sustainable solution than to include a feature into Fortran fpm due to the lack of the former. Regarding the other features you mentioned, maybe something like #211 would be a solution?

NB, https://github.blog/2018-05-01-ignore-white-space-in-code-review/ works perfectly for ignoring whitespace changes.

[LKedward]

Thanks @urbanjost, I don't think there's anything more you need to address currently. I think removing the intrinsic documentation was probably best since it hasn't been discussed in much detail - @awvwgk's plugin suggestion (#211) seems to address the extension of fpm's capabilities in a nicely modular manner and I agree an install command is probably the way forward there.

All that's left now is to get two more approvals.

[milancurcic]

I promise to review this tomorrow. Sorry for the delay.

[milancurcic]

I skimmed through the changes, installed it, and played with it. I think it can go forward. Thank you @urbanjost!

[LKedward]

Thanks for reviewing everyone, and many thanks @urbanjost! If there are no objections I will merge later today.