Skip to content

Refactor run command #229

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 9 commits into from
Nov 12, 2020
Merged

Refactor run command #229

merged 9 commits into from
Nov 12, 2020

Conversation

LKedward
Copy link
Member

@LKedward LKedward commented Nov 6, 2020 •
edited
Loading

This PR simplifies the run command implementation to use information provided by the model%targets structure. This avoids the duplication of code which has caused the run command to construct executable paths differently to the build command; this PR fixes this problem.

@LKedward LKedward mentioned this pull request Nov 6, 2020
Merged
@LKedward LKedward requested a review from urbanjost November 6, 2020 11:43
@urbanjost
Copy link
Contributor

urbanjost commented Nov 7, 2020 •
edited
Loading

So far the code looks good and is passing many tests but ran into
a few problems that might be on my side, and I have not gotten
through some more complex builds with C code so I am still testing.

I am excited on how well it it working on a machine with no bootstrap
version installed, just the Fortran version. Basic build, run, and
test are working with many fpm(1) packages I have with --release and
--list.

I like the change to just show the pathname with --list which I was
going to put a little PR in for. It allows the names to be used with
external filters easily. It works particularly well with the Unix
xargs(1) command.

On a machine with just a single Fortran source built from #pr229
many fpm(1) packages are working. The ones that I do not think I
can build with this version are ones that require a "fpm.mk" or custom
build script.

So this is the only issue I have to raise at this point ...

When an executable does not exist there is no message.

   ffpm new A --lib
   cd A
   ffpm run
   ffpm test

This is different than the h-fpm version, which produces a warning
message. It is arguable whether it should or should not produce a
message for the case where there is no executable but even when the name
is incorrect no error or warning is produced. eg:

   ffpm run this_is_a_typo

does not warn that there is no such executable. I think it should produce
a message and perhaps do the equivalent of the --list option to show the
available choices. Maybe just the basenames would be preferable in a
little table.

 box_month       d2j              d2o              d2u            d2w
 date_to_julian  date_to_unix     days2sec         dow            easter
 fmtdate         fmtdate_usage    guessdate        j2d            julian_to_date
 mo2d            mo2v             moon_fullness    M_time_oop     now
 o2d             ordinal_seconds  ordinal_to_date  phase_of_moon  sec2days
 system_sleep    u2d              unix_to_date     v2mo           w2d
 runTests

maybe just a message to say to use the --list option to see available choices when an unknown name is entered?

@urbanjost
Copy link
Contributor

A minor inconsistency. The default search includes the current directory
when anything is found, but if you have no app/ directory it tells you there is nothing to build.
But if you explicitly tell it to build a program in the current directory
you end up getting two executables if you do not use the default name
for your executable.

I like the concept of having a project in a directory with
nothing but a source file and an fpm.toml file with external dependencies.
And if I add the auto-executables directive I get the intuitive result.

So if I have a directory that contains nothing but "main.f90" and the
following manifest file I get the binary that I was trying to produce (so I
consider this the "minimal project"). I can see use cases for this where
I have a program that needs nothing else except external directories.

CASE I

name = "test"
[build]
auto-executables = false

[[executable]]
name="testit"
source-dir="."
main="main.f90"

but if I just have a manifest with a name in it and the same program file

CASE II

name = "test"

I get

Neither library nor executable found, there is nothing to do
ERROR STOP 1

Error termination. Backtrace:
   :
   :

So it acts like the current directory is not automatically searched.

But if I change the manifest to

CASE III

name = "test"

[[executable]]
name="testit"
source-dir="."
main="main.f90"

It builds two executables, main and testit because the code
automatically searches the current directory. So to have the minimal
two-file project just generate a single file I have to turn off
auto-detection.

The documentation for the manifest file says it searches in the
default directories, so one suggestion might be that it not search
the current directory.

Another approach would be to have it find and build without the
error message when it finds a program in the current directory instead
of saying there are not any.

I think others might argue it should only allow pathnames below the
current directory but I actually like being able to make a two-file
project where the manifest just lists external dependencies. I think
this could be a common use of fpm.

@LKedward
Copy link
Member Author

LKedward commented Nov 7, 2020

@urbanjost, I like the idea of a minimal two file project - it was not a test case I thought of testing before. It looks like there's a bug in the source processing of explicit [[executable]] entries; the current/root directory should not be searched by auto-discovery. Both Case I and case III should work, but not case II. I will look into this. Thanks for bringing it up.

@urbanjost
Copy link
Contributor

That was fast. Perfect for ffpm run **not_there** when there are valid targets. When there are no valid targets a simple ffpm run produces no message, but for consistency should it say "no valid targets" or show the same thing that "ffpm run empty" did in the directory with no targets in the following sequence of commands? I am fine with it as-is but I think it would be less confusing if you were working with a package you did not create and/or were new to fpm. Really nice work. Least number of comments I think I have ever had for something with so many arbitrary paths through it in an alpha version; and nothing I spotted by eye in the code to comment on at all.

#!/bin/bash
(
exec 2>&1
 set -x
 ffpm run asdf
 ffpm new A -lib
 cd A
 ffpm run     <<<< NO OUTPUT?
 ffpm run empty
)|tee -a $0
exit
################################################################################
+ ffpm run asdf
fpm::run<ERROR> specified names "asdf" not found.

 Available names:
        box_month              d2j              d2o              d2u
              d2w   date_to_julian     date_to_unix         days2sec
              dow           easter          fmtdate    fmtdate_usage
        guessdate              j2d   julian_to_date             mo2d
             mo2v    moon_fullness       M_time_oop              now
              o2d  ordinal_seconds  ordinal_to_date    phase_of_moon
         sec2days     system_sleep              u2d     unix_to_date
             v2mo              w2d
STOP 1
+ ffpm new A -lib
 + mkdir -p A
 + cd A
 + mkdir -p A/src
 + git init A
Initialized empty Git repository in /home/urbanjs/venus/V600/github/M_time/A/.git/
+ cd A
+ ffpm run
+ ffpm run empty
fpm::run<ERROR> specified names "empty" not found.

 Available names:

STOP 1

@urbanjost
Copy link
Contributor

This is an edge case but it looks like it would only take a few lines to warn the user something went wrong.
Since all the applications go into one directory it is relatively easy to create duplicates that collide. So with a manifest
like this

name = "test"

[[executable]]
name="testit"
source-dir="."
main="main.f90"

[[executable]]
name="testit"
source-dir="other"
main="main.f90"

[[executable]]
name="testit"
source-dir="other"
main="main2.f90"

In a intentionally setup with intentional duplicates both h-fpm and
f-fpm build all the targets. h-fpm places them in subdirectories
corresponding to their original location while f-fpm places them all
in build/*/app/. h-fpm has no good way to execute them except by the
basename that I know of so all the targets exist but it is not without
issues, but f-fpm overwrites. f-fpm does list them all with

fpm build --list

so I think it would be relatively easy to warn when there are duplicate
basenames so you know there was an overwrite.

In my testcase the filesystem looks like

./app/main.f90
./app/main2.f90
./other/main2.f90
./other/main.f90
./other/main3.f90

I agree this an aberrant case but it looks like the warning would be easy
to produce.

I much bigger change would be to put the executables into separate directories like h-fpm but unless the run command took subdirectory names that really does not solve the issue; and if the user is warned they can easily (probably) resolve it by giving a unique name to the output executable in the manifest file.

@urbanjost
Copy link
Contributor

When I took a second look there is a fixed format of 17 characters, which I sometimes
exceed.

Available names:
demo_system_accesdemo_system_chdirdemo_system_chmoddemo_system_chown                                                               
demo_system_cleardemo_system_closedemo_system_cpu_tdemo_system_errno                                                               
demo_system_getcwdemo_system_getegdemo_system_getendemo_system_geteu                                                               
demo_system_getgidemo_system_getgrdemo_system_gethodemo_system_getlo                                                               
demo_system_getpidemo_system_getppdemo_system_getpwdemo_system_getsi                                                               
demo_system_getuidemo_system_getumdemo_system_initedemo_system_isblk                                                               
demo_system_ischrdemo_system_isdirdemo_system_isfifdemo_system_islnk                                                               
demo_system_isregdemo_system_issoc demo_system_kill demo_system_link                                                               
demo_system_mkdirdemo_system_mkfifdemo_system_opend demo_system_perm                                                               
demo_system_perrodemo_system_puten demo_system_randdemo_system_readd                                                               
demo_system_readedemo_system_realpdemo_system_removdemo_system_renam                                                               
demo_system_rewindemo_system_rmdirdemo_system_setsidemo_system_setum                                                               
demo_system_sranddemo_system_unamedemo_system_unlindemo_system_unset                                                               
demo_system_utime
STOP 1diff --git a/fpm/src/fpm.f90 b/fpm/src/fpm.f90I did ask for this :>.

I cleaner way would require two passes to find the longest string I think. Maybe there is a simpler way you see. A kludge that
makes sure nothing is truncated without taking a second pass worked. It would not keep columns aligned but it would prevent truncattion:

index 31927fc..d852cb6 100644
--- a/fpm/src/fpm.f90
+++ b/fpm/src/fpm.f90
@@ -304,7 +304,7 @@ subroutine cmd_run(settings,test)
     class(fpm_run_settings), intent(in) :: settings
     logical, intent(in) :: test
 
-    integer :: i, j
+    integer :: i, j, column_width
     logical :: found(size(settings%name))
     type(error_t), allocatable :: error
     type(package_t) :: package
@@ -382,8 +382,8 @@ subroutine cmd_run(settings,test)
 
         j = 1
         write(stderr,*) 'Available names:'
+        column_width=17
         do i=1,size(model%targets)
-
             exe_target => model%targets(i)%ptr
     
             if (exe_target%target_type == FPM_TARGET_EXECUTABLE .and. &
@@ -394,7 +394,9 @@ subroutine cmd_run(settings,test)
                 if (exe_source%unit_scope == &
                     merge(FPM_SCOPE_TEST,FPM_SCOPE_APP,test)) then 
 
-                    write(stderr,'(A17)',advance=(merge("yes","no ",modulo(j,4)==0))) basename(exe_target%output_file)
+                    column_width=max(column_width,len(basename(exe_target%output_file))+1)
+                    write(stderr,'(A)',advance=(merge("yes","no ",modulo(j,4)==0))) &
+                    & [character(len=column_width) ::basename(exe_target%output_file)]
 
                     j = j + 1
fpm::run<ERROR> specified names "asdf" not found.

 Available names:
demo_system_access demo_system_chdir  demo_system_chmod  demo_system_chown  
demo_system_clearenv demo_system_closedir demo_system_cpu_time demo_system_errno    
demo_system_getcwd   demo_system_getegid  demo_system_getenv   demo_system_geteuid  
demo_system_getgid   demo_system_getgrgid demo_system_gethostname demo_system_getlogin    
demo_system_getpid      demo_system_getppid     demo_system_getpwuid    demo_system_getsid      
demo_system_getuid      demo_system_getumask    demo_system_initenv     demo_system_isblk       
demo_system_ischr       demo_system_isdir       demo_system_isfifo      demo_system_islnk       
demo_system_isreg       demo_system_issock      demo_system_kill        demo_system_link        
demo_system_mkdir       demo_system_mkfifo      demo_system_opendir     demo_system_perm        
demo_system_perror      demo_system_putenv      demo_system_rand        demo_system_readdir     
demo_system_readenv     demo_system_realpath    demo_system_remove      demo_system_rename      
demo_system_rewinddir   demo_system_rmdir       demo_system_setsid      demo_system_setumask    
demo_system_srand       demo_system_uname       demo_system_unlink      demo_system_unsetenv    
demo_system_utime       
STOP 1

I was looking for a quick way to find the longest name and did not see it off the bat and thought I would show you this first before looking deeper. Maybe just listing the basenames instead of the table is simpler. I do not know how common having this many applications or tests is. I happen to try to have a demo as part of each manpage; which can add up quickly.

@urbanjost
Copy link
Contributor

FYI: worked very nicely with a module of mine called M_system that contained a C file that required me to use an fpm..mk file with h-fpm. Refactoring all my fpm packages to work with f-fpm; which is breaking some of them for use with h-fpm. I am putting a note in the README.md files on the github sites to that effect, but a number of them are listed in the fpm registry which causes some confusion. I think I need to add a rev to the FPM registry files to point to the older ones

@LKedward
Minor fix: to output formatting for run cmd
e15996c 
When outputting a list of available run/test targets, format the output columns nicely depending on the maximum length of the targets.
@LKedward
Add check to run cmd for when there are no apps/tests to run
4d87edd
@LKedward
Minor fix: no recursive source search for [[executable]] entries
7701274 
Source directories specified in [[executable]] and [[test]] are not searched recursively. Fixes issue with having programs in the root directory.
@LKedward
Merge branch 'refactor-sources' into refactor-run-cmd
1440469
@urbanjost
Copy link
Contributor

urbanjost commented Nov 8, 2020 •
edited
Loading

All the other tests I have passed other than what I commented on. I am moving a few repositories to use the new fpm but unless I stumble on something all my tests are complete. Seems like a bootstrappable single Fortran file would be due for people to experiment with? They would just need a Fortran compiler and gfortran. build, run, test, new, help all function, the TOML interface and build with a remote dependency works, the auto-discovery makes for some simple manifest files. Some people might be able to give feedback more readily if they just have to pull a file down and compile it with the language they are developing in. Still cannot do custom build scripts, add external libraries, options of OpenMP, OpenACC, MPI, Coarrays, ... but getting pretty useful.

@urbanjost urbanjost mentioned this pull request Nov 9, 2020
Closed
@urbanjost
Copy link
Contributor

A build of this branch placed into a single file and built with a simple "gfortran ffpm.f90" on a machine with git and gfortran but no bootstrap version built and ran all the repositories in the fpm registry except one that got a compiler error (it was gfortran 10.0) that was part of the package and not an fpm issue. I could build run and test (and help and new which were not really being changed) in all those packages.
The only problem I had was on a CygWin machine where I had to unset the OS environment variable in order for the code to select OS_TYPE=cygwin instead of MSWindows. I think that is a seperate issue to resolve in a new PR.
Out of my own fpm packages the only one I cannot build is one that requires the X11 Windows libraries, which was expected as custom build scripts are not supported yet, nor are user-supplied load and compiler switches. Nice job.

@urbanjost
Copy link
Contributor

PS:

wget http://www.urbanjost.altervista.org/REMOVE/ffpm.f90 
gfortran ffpm.f90 -o ffpm

I am going to leave the file ffpm.f90 available for a week that can be accessed via a browser or something like the wget(1) above. If anyone has access to compilers other than gfortran I am very curious if the file compiles and runs. The fiile is fpm #229.

@arjenmarkus
Copy link
Member

arjenmarkus commented Nov 11, 2020 via email

@urbanjost
Copy link
Contributor

Thanks. That is basically all one issue I believe. Interesting differences.

@awvwgk
Copy link
Member

awvwgk commented Nov 11, 2020 •
edited
Loading

I have used Intel Fortran version 2018 to compile the file. It produced
quite a few error messages - see the attachment.

@arjenmarkus I'm responsible for parts of those issues with TOML-Fortran (get_value, see toml-f/toml-f#16), but those are now fixed with the v0.2.1 release and will be used in fpm with #233.

@milancurcic
Copy link
Member

I'm responsible for parts of those issues with TOML-Fortran (get_value, see toml-f/toml-f#16), but those are now fixed with the v0.2.1 release and will be used in fpm with #233.

@LKedward @awvwgk Considering this, is there a preferred order to merging? Should this PR wait for #233 or it doesn't matter?

@awvwgk
Copy link
Member

awvwgk commented Nov 11, 2020

Doesn't matter too much. In case there is a fix required in this PR as well, updating the TOML-Fortran dependency is just a simple patch version bump:

diff --git a/fpm/fpm.toml b/fpm/fpm.toml
index fc3a381..404e65c 100644
--- a/fpm/fpm.toml
+++ b/fpm/fpm.toml
@@ -8,7 +8,7 @@ copyright = "2020 fpm contributors"
 [dependencies]
 [dependencies.toml-f]
 git = "https://github.com/toml-f/toml-f"
-tag = "v0.2"
+tag = "v0.2.1"
 
 [dependencies.M_CLI2]
 git = "https://github.com/urbanjost/M_CLI2.git"

@LKedward
Copy link
Member Author

The fix isn't required for this PR and I have no preferred order of merging.

@milancurcic
Copy link
Member

Sounds good, I will merge this.

@milancurcic milancurcic merged commit b1fddf3 into fortran-lang:master Nov 12, 2020
@LKedward LKedward deleted the refactor-run-cmd branch December 9, 2020 16:17
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@milancurcic milancurcic milancurcic approved these changes

@urbanjost urbanjost urbanjost approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@LKedward @urbanjost @arjenmarkus @awvwgk @milancurcic