Cleanup the backend output #622

LKedward · 2021-11-23T10:36:10Z

Feel free to test it out - feedback welcome.

LKedward · 2021-11-27T19:01:58Z

I've now implemented everything I'd envisaged for this. Apologies for the size of the PR! Handling the isatty check and collecting compiler output made it a bit more involved.

In summary:

Compiler output is now redirected to log files by default and not printed to the screen
'Pretty' status messages are printed and an overall progress percentage
If the --verbose flag is passed, then pretty printing is disabled and the compiler output is printed to screen (including already-built targets during incremental builds)
If stdout is not connected to a terminal, then pretty printing is disabled

This PR bundles a small MIT licensed c code for properly checking isatty on Windows.
I also had to update the bootstrap version in the CI and the gfortran version in the Windows runner.

I've tested on Ubuntu, Windows command prompt and MSYS2 (Mintty) on Windows. Will need a volunteer to test on Mac.

awvwgk

Looks good, I left a couple of comments regarding the setup of the classes and the color attributes from a first sweep through the implementation.

I'll do some proper testing soon.

src/fpm.f90

src/fpm_backend_console.f90

src/fpm_backend_output.f90

src/fpm_environment.f90

certik

I only reviewed from a "high level" perspective. I don't see any major issues. As long as @awvwgk is satisfied with the details, this is good to merge as far as I am concerned.

urbanjost · 2021-11-28T18:10:36Z

Everything has tested well. I was going to suggest using the attr_update(3f) function in M_attr(3f) to add clearline, upline, and downline and moving the call to detect if plain or color mode to the fpm_command_line module so all routines could use colors, so the messages set up with and could use color but it looks like you took M_attr out.

As far as the interface goes, I would prefer that --verbose show all messages only when all targets are built, and only show messages from rebuilt files during an incremental build. On a large project a rebuild of a single file with --verbose mode shows 27 000, lines, which is actually more verbose than the original :>.

The conditional --verbose mode output is of course not an issue with a project composed of one file, and really only gets horrible with the gfortran compiler and allocatable strings but to me that is a must-have.

Still reviewing, and I think some other thoughts I have had (discussed in the previous canceled PRs) are not important enough to hold back this (IMO) major advancement (having messages in the associated log files). Some utilties use repeated use of --verbose or a --verbose level like --verbose [1,2,3...]. That would be easy to add transparently at the command line level.

certik · 2021-11-28T18:30:43Z

I tested on Apple M1. I rebuilt this PR, and then tried to run, it works, but "build" segfaults:

$ build/gfortran_2A42023B310FA28D/app/fpm 
Fortran Package Manager:
 
USAGE: fpm [ SUBCOMMAND [SUBCOMMAND_OPTIONS] ]|[--list|--help|--version]
       where SUBCOMMAND is commonly new|build|run|test

 subcommand may be one of

  build     Compile the package placing results in the "build" directory
  help      Display help
  list      Display this list of subcommand descriptions
  new       Create a new Fortran package directory with sample files
  run       Run the local package application programs
  test      Run the test programs
  update    Update and manage project dependencies
  install   Install project

 Enter "fpm --list" for a brief list of subcommand options. Enter
 "fpm --help" or "fpm SUBCOMMAND --help" for detailed descriptions.



$ build/gfortran_2A42023B310FA28D/app/fpm build

Program received signal SIGSEGV: Segmentation fault - invalid memory reference.

Backtrace for this error:
#0  0x104bd76f7
#1  0x104bd671b
#2  0x18dfc6c43
#3  0x1048c2ac7
zsh: segmentation fault  build/gfortran_2A42023B310FA28D/app/fpm build

certik · 2021-11-28T18:32:12Z

In lldb I get:

* thread #1, queue = 'com.apple.main-thread', stop reason = EXC_BAD_ACCESS (code=1, address=0xc)
    frame #0: 0x000000010004300c fpm`__fpm_filesystem_MOD_mkdir at fpm_filesystem.F90:360:23
   357 	    logical :: echo_local
   358 	    
   359 	    if(present(echo))then
-> 360 	        echo_local=echo
   361 	     else
   362 	        echo_local=.true.
   363 	     end if
Target 0: (fpm) stopped.
(lldb) bt
* thread #1, queue = 'com.apple.main-thread', stop reason = EXC_BAD_ACCESS (code=1, address=0xc)
  * frame #0: 0x000000010004300c fpm`__fpm_filesystem_MOD_mkdir at fpm_filesystem.F90:360:23
    frame #1: 0x0000000100022ac8 fpm`__fpm_backend_MOD_build_package at fpm_backend.f90:68:34
    frame #2: 0x000000010000da30 fpm`__fpm_MOD_cmd_build at fpm.f90:290:62
    frame #3: 0x0000000100005180 fpm`MAIN__ at main.f90:67:28
    frame #4: 0x000000010000565c fpm`main at main.f90:10:28
    frame #5: 0x000000018df99430 libdyld.dylib`start + 4

urbanjost · 2021-11-28T19:47:22Z

PS; I think ifort and gfortran always default to PAD='yes' but not sure, so might want to add that explicitly or getline(3f) will probably have a problem, as per the example in getline.

For compiler_t and archive_t objects

fpm_environment::run is moved to fpm_filesystem so that it can use the getline function to retrieve redirected output from file

awvwgk

Thanks a lot, the implementation looks fine to me. Just some minor nit-picks in the comments.

.github/workflows/CI.yml

src/fpm_backend.F90

Co-authored-by: Sebastian Ehlert <28669218+awvwgk@users.noreply.github.com>

urbanjost · 2021-11-30T14:27:04Z

somewhat ironically, I immediately miss being able to get all the messages on a built project. Is that something you were going to build back in?

LKedward · 2021-11-30T16:16:19Z

somewhat ironically, I immediately miss being able to get all the messages on a built project. Is that something you were going to build back in?

I agree we should build this in but I'm not sure what the user interface would be right now (fpm log, fpm build --log?) and so I suggest leaving this for a future PR.

awvwgk · 2021-12-04T10:46:25Z

A minor issue I encountered is that the output is not correctly aligned, the compiling... case starts a column before the done and failed output.

 fpm_backend_console.f90                done.
 fpm_constants.f90                      done.
 fpm_manual.f90                         done.
 fpm_strings.f90                        done.
script.f90                             compiling...
constants.f90                          compiling...
version.f90                            compiling...

Another thing, building a deeply nested project like TOML Fortran leads to printout like this

 + mkdir -p build/dependencies
 color.f90                              done.
 constants.f90                          done.
 version.f90                            done.
 tomlf.c                                done.
 datetime.f90                           done.
 error.f90                              done.
 verify.f90                             done.
 diagnostic.f90                         done.
 main.f90                               done.
 convert.f90                            done.
 utils.f90                              done.
 value.f90                              done.
 keyval.f90                             done.
 base.f90                               done.
 sort.f90                               done.
 vector.f90                             done.
 structure.f90                          done.
 array.f90                              done.
 table.f90                              done.
 type.f90                               done.
 ser.f90                                done.
 tokenizer.f90                          done.
 keyval.f90                             done.
 merge.f90                              done.
 character.f90                          done.
 array.f90                              done.
 table.f90                              done.
 build.f90                              done.
 de.f90                                 done.
 all.f90                                done.
 tomlf.f90                              done.
 libtoml-f.a                            done.
 toml-f                                 done.
[100%] Project compiled successfully.

Where files with different paths but identical basename look like they are appearing twice in the compilation step.

zoziha · 2021-12-04T11:42:48Z

First of all, thanks to this PR, this gives the user experience a big step up.

And, is it possible to add output path information of the target binaries to the --verbose information? It contains hash values that are easy for users to know, that will be useful.

# `fpm build --verbose` info
 <INFO> BUILD_NAME: build\gfortran
 <INFO> COMPILER:  gfortran
 <INFO> C COMPILER:  gcc
 <INFO> COMPILER OPTIONS:   -Wall -Wextra -Wimplicit-interface -fPIC -fmax-errors=1 -g -fcheck=bounds -fcheck=array-temps -fbacktrace -fcoarray=single
 <INFO> C COMPILER OPTIONS:
 <INFO> LINKER OPTIONS:
 <INFO> INCLUDE DIRECTORIES:  []
[100%] Project compiled successfully.

For example, when I first used fpm build, I couldn't know exactly where the generated binary was in unless: fpm run --runner file.

Project is up to date
build\gfortran_2A42023B310FA28D\app\fpm.exe: PE32+ executable (console) x86-64, for MS Windows

LKedward · 2021-12-06T09:02:54Z

A minor issue I encountered is that the output is not correctly aligned, the compiling... case starts a column before the done and failed output.

Ah yes, I'll sort that out.

Where files with different paths but identical basename look like they are appearing twice in the compilation step.

I'll have to have a think about what the best way to address this is. Including full paths by default may lead to a lot of redundant output noise.

LKedward · 2021-12-06T09:07:16Z

And, is it possible to add output path information of the target binaries to the --verbose information? It contains hash values that are easy for users to know, that will be useful.

Thanks @zoziha - the output path hash was removed from the verbose summary information because it technically varies on a per-file basis (it can be different for different files with different flags). However, you can run fpm build --list to quickly get the output paths before building your project.

awvwgk · 2022-01-16T12:57:20Z

@LKedward, any updates on this patch?

LKedward · 2022-01-17T12:35:39Z

Apologies, it has been an incredibly busy start to the year for me. I hope to get back on this soon.

Where files with different paths but identical basename look like they are appearing twice in the compilation step.

Part of the delay is that I haven't come up with a simple and non-verbose solution to the identical file problem - suggestions would be welcome.

awvwgk · 2022-01-17T15:30:35Z

I haven't come up with a simple and non-verbose solution to the identical file problem - suggestions would be welcome.

How about:

<project>:path/from/project/root/to/source.f90

LKedward · 2022-02-06T09:54:00Z

How about:

<project>:path/from/project/root/to/source.f90

Cheers, yes I was thinking along the same lines. Unfortunately this is a non-trivial addition to which I haven't been able to commit the time. Can that be left for a separate PR, or is someone able to help out with this?

awvwgk · 2022-02-06T09:55:05Z

I'm fine with having a separate PR for this. Let's move this one forward.

LKedward · 2022-02-12T11:37:08Z

Thanks everyone for your feedback and patience. I think this is now ready to go. Let me know if you have any other suggestions.

LKedward added 10 commits November 22, 2021 15:56

Fix: os_is_unix function

22ec97a

Update: mkdir with optional echo argument

d9520ce

Update: run command with optional verbose argument

2654623

Update: backend with verbose argument

30d730f

Update: fpm_compiler objects with verbose field

6ea3493

Update: run command with optional verbose argument

995fb2e

Add: backend_output to manage pretty printing of build progress

bfd9b06

Bump bootstrap fpm version to 0.3.0

5728b54

Fix: backend c_isatty for bootstrapping

229761a

Update: fpm_backend as preprocessed file.

7787632

LKedward marked this pull request as draft November 23, 2021 11:19

LKedward linked an issue Nov 23, 2021 that may be closed by this pull request

Change how output is presented #619

Open

LKedward added 4 commits November 25, 2021 12:03

Fix for checking isatty in MSYS2 mintty.

2e2f0e3

Update: Windows CI to use gfortran 9 from winlibs.

8b4f3a6

Update: run to allow output redirection to file

b628302

Update: fpm_compiler & backend to redirect output to log files

ab7cb42

ivan-pi mentioned this pull request Nov 25, 2021

Add module for handling version information of stdlib fortran-lang/stdlib#579

Merged

LKedward added 2 commits November 26, 2021 17:32

Simplify implementation and cleanup plain mode output

37ba9d7

Add: developer documentation to new files

93b629e

LKedward marked this pull request as ready for review November 27, 2021 18:24

LKedward requested review from awvwgk, certik and urbanjost November 27, 2021 19:02

LKedward changed the title ~~(WIP) Cleanup the backend output~~ Cleanup the backend output Nov 27, 2021

awvwgk reviewed Nov 27, 2021

View reviewed changes

certik approved these changes Nov 27, 2021

View reviewed changes

LKedward added 5 commits November 29, 2021 11:54

Update: backend to print message if up to date.

fc058ec

Apply suggestion: move echo/verbosity into constructors

4556e7a

For compiler_t and archive_t objects

Apply suggestion: don't use TBP for new constructors

6aba40d

Apply suggestion: don't use M_attr, simplify implementation

b0115d1

Apply suggestion: move run to filesystem and use getline

0c561b0

fpm_environment::run is moved to fpm_filesystem so that it can use the getline function to retrieve redirected output from file

awvwgk approved these changes Nov 29, 2021

View reviewed changes

.github/workflows/CI.yml Show resolved Hide resolved

src/fpm_backend.F90 Outdated Show resolved Hide resolved

src/fpm_backend.F90 Outdated Show resolved Hide resolved

src/fpm_backend.F90 Outdated Show resolved Hide resolved

LKedward linked an issue Nov 30, 2021 that may be closed by this pull request

Unexpected windows-gfortran version (8.1) in fpm/GitHub-CI(workflow:CI.yml) #537

Closed

Apply suggestions from code review

b1b6a7b

Co-authored-by: Sebastian Ehlert <28669218+awvwgk@users.noreply.github.com>

urbanjost approved these changes Dec 1, 2021

View reviewed changes

LKedward added 2 commits February 12, 2022 11:16

Merge branch 'main' into backend-output

e8c0854

Fix: for consistent alignment of backend console output.

6cd53f7

Merge branch 'main' into backend-output

d2009f1

LKedward merged commit 2c6148e into fortran-lang:main Feb 14, 2022

LKedward deleted the backend-output branch February 14, 2022 19:04

LKedward mentioned this pull request Feb 14, 2022

Change how output is presented #619

Open

everythingfunctional mentioned this pull request Mar 2, 2022

New output for fpm test and/or fpm run breaks my CI #666

Closed

awvwgk added this to the v0.6.0 milestone Apr 22, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Cleanup the backend output #622

Cleanup the backend output #622

LKedward commented Nov 23, 2021 •

edited

Loading

LKedward commented Nov 27, 2021

awvwgk left a comment •

edited

Loading

certik left a comment

urbanjost commented Nov 28, 2021

certik commented Nov 28, 2021

certik commented Nov 28, 2021

urbanjost commented Nov 28, 2021

awvwgk left a comment

urbanjost commented Nov 30, 2021

LKedward commented Nov 30, 2021

awvwgk commented Dec 4, 2021

zoziha commented Dec 4, 2021 •

edited

Loading

LKedward commented Dec 6, 2021

LKedward commented Dec 6, 2021

awvwgk commented Jan 16, 2022

LKedward commented Jan 17, 2022

awvwgk commented Jan 17, 2022

LKedward commented Feb 6, 2022

awvwgk commented Feb 6, 2022

LKedward commented Feb 12, 2022

Cleanup the backend output #622

Cleanup the backend output #622

Conversation

LKedward commented Nov 23, 2021 • edited Loading

LKedward commented Nov 27, 2021

awvwgk left a comment • edited Loading

Choose a reason for hiding this comment

certik left a comment

Choose a reason for hiding this comment

urbanjost commented Nov 28, 2021

certik commented Nov 28, 2021

certik commented Nov 28, 2021

urbanjost commented Nov 28, 2021

awvwgk left a comment

Choose a reason for hiding this comment

urbanjost commented Nov 30, 2021

LKedward commented Nov 30, 2021

awvwgk commented Dec 4, 2021

zoziha commented Dec 4, 2021 • edited Loading

LKedward commented Dec 6, 2021

LKedward commented Dec 6, 2021

awvwgk commented Jan 16, 2022

LKedward commented Jan 17, 2022

awvwgk commented Jan 17, 2022

LKedward commented Feb 6, 2022

awvwgk commented Feb 6, 2022

LKedward commented Feb 12, 2022

LKedward commented Nov 23, 2021 •

edited

Loading

awvwgk left a comment •

edited

Loading

zoziha commented Dec 4, 2021 •

edited

Loading