error_stop to stderr and optional returncode

[scivision]

Users will probably expect error messages to be printed to stderr instead of stdout. This is especially important for programs that normally have a lot of stdout output or binary streams on stdout.

This also adds the option for user-specified error code, which can be used to signal a build system that a test should be skipped instead of failing. For example, maybe a skippable test requires external hardware that the build system cannot detect, but the running program does detect.

[certik]

@scivision can you please rebase this on top of the latest master?

[zbeekman]

Changes all LGTM!

[zbeekman]

Small nits: files should end in a single new line character... but looks good to me other than that.

[zbeekman]

LGTM!

[jvdp1]

+1 to merge

[certik]

There are way too many changes in this PR and every time you push more code, I have to review the whole thing and it takes a lot of time.

Can you please split the CMake improvements into a separate PR? We all agree we want that, let's get it merged. The action is also fine to have, plus the loadtxt/savetxt changes for working directory.

I have some discussion about the other stuff. Let's discuss that later, once the agreed upon parts are merged in.

[scivision]

This PR is an atomic change to properly implement error stop in a Fortran 2008 and 2018 compiler-friendly way. I don't think it's feasible to make this PR smaller without breaking the CI or library.

[certik]

Yes it is possible to make it smaller. I am busy today, but I will separate this PR into a smaller one either tonight or tomorrow.

[certik]

My apologies --- this PR should not be split. I think it looks good. I left some comments and suggestions, but overall I approve it.

[certik]

This does not need to be imported here, does it?

[certik]

I tested this patch and it works:

diff --git a/src/f08estop.f90 b/src/f08estop.f90
index d501978..17ea448 100644
--- a/src/f08estop.f90
+++ b/src/f08estop.f90
@@ -1,4 +1,5 @@
 submodule (stdlib_experimental_error) estop
+use, intrinsic :: iso_fortran_env, only: stderr=>error_unit
 
 contains
 
diff --git a/src/f18estop.f90 b/src/f18estop.f90
index ea83de7..6be7900 100644
--- a/src/f18estop.f90
+++ b/src/f18estop.f90
@@ -1,4 +1,5 @@
 submodule (stdlib_experimental_error) estop
+use, intrinsic :: iso_fortran_env, only: stderr=>error_unit
 
 contains
 
diff --git a/src/stdlib_experimental_error.f90 b/src/stdlib_experimental_error.f90
index 3d932d6..135722b 100644
--- a/src/stdlib_experimental_error.f90
+++ b/src/stdlib_experimental_error.f90
@@ -1,5 +1,4 @@
 module stdlib_experimental_error
-use, intrinsic :: iso_fortran_env, only: stderr=>error_unit
 implicit none
 private

[scivision]

it's more of a deduplication issue. The scope of submodule is such that these submodules pickup stderr from the parent module stdlib_experimental_error e.g. imagine there are dozens of submodules (which eventually there will be in this project) they don't all need to each import the same modules, just import once in the parent or ancestor module.
i.e. part of the "point" of using submodules is deduplication, and that can also include deduplicating use statements that many submodules use.

[certik]

Ok, that's fine. I don't use submodules myself, so I don't know what the best way to use them is. I thought the idea is that the main module is sort of like a C++ .h file and the submodule is then like .cpp file. In C++, the accepted practice is to only include things in the .h files that are needed to specify the API. Everything else is included in the .cpp files. In the same spirit, since stderr is not needed for the API, I thought it would make sense to only include it in the submodule.

[scivision]

One could use module/submodule that way, but they are generally not used that way as the scoping rules are different than C++. I use submodules for almost every program for reasons including:

• very fast rebuild if procedure API didn't change, can be 100x or more build wallclock speed, important for big programs that take several minutes to build fully, but only a couple seconds if rebuilding after submodule change
• allows switching in/out capabilities or libraries. Examples
  • file IO with HDF5, NetCDF4 or fallback to raw binary if they're not available, switched by the build systems auto/manual
  • external procedures/modules, maybe some are proprietary or take a long build time or a lot of prereqs, make them optional yet almost instantly switchable in/out via build system
  • procedure switching, maybe the procedures are overloaded in certain ways as defined by a simulation, again making almost instant rebuilds on big programs with build system options

So submodule can be used like one step up from "just in time" compilation, I call it "build on run" since based on options say called from Python API, I can rebuild huge Fortran program based on options user specifies from Python (or directly in CMake) in seconds.

[scivision]

submodule can also be used for build-time polymorphism, avoiding need for preprocessing and again shrinking memory use and build time by only building the required kinds.
I believe a significant part of the preprocessing and generation seen in Fortran could be replaced more cleanly and readably with submodule, select type and select rank. Of course will still be good applications for a small #ifdef foo #endif and generation.

[certik]

What is the purpose of this test? What is it testing? When I run ctest, it outputs:

$ ctest 
Test project /home/ondrej/repos/stdlib
    Start 2: AlwaysFail
    Start 3: ASCII
    Start 4: load_text
    Start 5: save_text
1/5 Test #2: AlwaysFail .......................   Passed    0.01 sec
2/5 Test #3: ASCII ............................   Passed    0.01 sec
3/5 Test #4: load_text ........................   Passed    0.01 sec
4/5 Test #5: save_text ........................   Passed    0.01 sec
    Start 1: AlwaysSkip
5/5 Test #1: AlwaysSkip .......................***Skipped   0.01 sec

100% tests passed, 0 tests failed out of 5

Total Test time (real) =   0.02 sec

The following tests did not run:
	  1 - AlwaysSkip (Skipped)

Which seems confusing.

[scivision]

It's testing that I can pass arbitrary return codes. There's a de facto standard that 77 means to skip a test, so 77 was a convenient alternative to the oft-used returncode 1 that was tested in AlwaysFail

[certik]

I see. If the test returns 0 instead of 77 because some compiler doesn't support it (let's say) then it will "pass" even though it should fail. I think a more robust solution would be to actually test that the test returns the exit code 77, and pass the ctest test, otherwise fail it.

…

On Tue, Dec 31, 2019, at 8:59 PM, Michael Hirsch, Ph.D. wrote: ***@***.**** commented on this pull request. In src/tests/CMakeLists.txt <#53 (comment)>: > @@ -1,3 +1,12 @@ add_subdirectory(ascii) add_subdirectory(loadtxt) +add_executable(test_skip test_skip.f90) +target_link_libraries(test_skip fortran_stdlib) +add_test(NAME AlwaysSkip COMMAND $<TARGET_FILE:test_skip>) +set_tests_properties(AlwaysSkip PROPERTIES SKIP_RETURN_CODE 77) It's testing that I can pass arbitrary return codes. There's a de facto standard that 77 means to skip a test, so 77 was a convenient alternative to the oft-used returncode 1 that was tested in AlwaysFail — You are receiving this because you commented. Reply to this email directly, view it on GitHub <#53?email_source=notifications&email_token=AAAFAWAXI6X2NCH5SNFCYSTQ3QILVA5CNFSM4KBP64WKYY3PNVWWK3TUL52HS4DFWFIHK3DMKJSXC5LFON2FEZLWNFSXPKTDN5WW2ZLOORPWSZGOCQOWHSQ#discussion_r362301437>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAAFAWDRAOJZ3KL666YSZB3Q3QILVANCNFSM4KBP64WA>.

[scivision]

Yes the AlwaysSkip has that downside. I think to do that sort of test in a cross-platform way would most expediently be done by a Python script. Otherwise a per-platform approach would be needed. I think it's a test that would be unlikely to have any problem like that.

[certik]

Yes, Bash probably isn't the most multiplatform, so it would have to be a Python script. The advantage is that cmake would show all tests as passed, none skipped.

…

On Tue, Dec 31, 2019, at 9:41 PM, Michael Hirsch, Ph.D. wrote: Yes the AlwaysSkip has that downside. I think to do that sort of test in a cross-platform way would most expediently be done by a Python script. Otherwise a per-platform approach would be needed. I think it's a test that would be unlikely to have any problem like that. — You are receiving this because you commented. Reply to this email directly, view it on GitHub <#53?email_source=notifications&email_token=AAAFAWGTCG6O4EYG24GXQPDQ3QNIFA5CNFSM4KBP64WKYY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOEH45DLI#issuecomment-570020269>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAAFAWECGUSWHLDSNGZPU33Q3QNIFANCNFSM4KBP64WA>.

[zbeekman]

Yes, Bash probably isn't the most multiplatform, so it would have to be a Python script. The advantage is that cmake would show all tests as passed, none skipped.

Suggestion: We could also use a CMake script as the test for return code by running cmake in script mode. That way users won't need python. You can achieve this by checking the RESULT_VARIABLE of execute_process().

But, perhaps a bigger question is: Should we check and require that compilers support return codes? I love using them, but do we want to rely on them vs behavior explicitly defined by the standard? (I.e., we look for specific test output to trigger test pass and fail status with CTest instead of return code.)

[certik]

If it can be done in CMake itself, then I would prefer that over Python.

Regarding return codes --- that's what ctest expects, but unfortunately I agree it is not standard in Fortran. But is there even an alternative?

[zbeekman]

Regarding return codes --- that's what ctest expects, but unfortunately I agree it is not standard in Fortran. But is there even an alternative?

Yes, there are the test properties PASS_REGULAR_EXPRESSION and FAIL_REGULAR_EXPRESSION. I typically set both in my projects, and then use print or write to explicitly indicate success and failure.

[certik]

@zbeekman I didn't know about that. We could use it.

Although I think it's useful and perhaps simpler to use return codes --- as long as all compilers that we want to support (#15) work with it. We should definitely test that error_stop returns the correct return code, and if there is a compiler that does not support it, we might need to figure out some workaround (perhaps calling into the system C API for that). Users would expect that error_stop will break their bash script, CI script, etc. So I think we have to do this anyway. And in that case, we might as well use return codes for the tests also.

[certik]

To move ahead with this PR, I am fine to merge as is, and improve upon it later. For example to use the CMake script to check for the non-zero return code, so that ctest does not print the ugly **Skipped.

[certik]

@milancurcic would you mind reviewing this also please? It looks like we all agree to merge it as is, and improve upon it later.

[certik]

@milancurcic ping.