-
Notifications
You must be signed in to change notification settings - Fork 164
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.
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
error_stop to stderr and optional returncode #53
Conversation
@scivision can you please rebase this on top of the latest master? |
b302640
to
6b2e4e4
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Changes all LGTM!
e24675c
to
c011b1e
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Small nits: files should end in a single new line character... but looks good to me other than that.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
+1 to merge
f74eafc
to
0364305
Compare
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 I have some discussion about the other stuff. Let's discuss that later, once the agreed upon parts are merged in. |
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. |
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. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
My apologies --- this PR should not be split. I think it looks good. I left some comments and suggestions, but overall I approve it.
@@ -1,41 +1,34 @@ | |||
module stdlib_experimental_error | |||
use, intrinsic :: iso_fortran_env, only: stderr=>error_unit |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This does not need to be imported here, does it?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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 kind
s.
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.
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) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
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>.
|
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. |
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>.
|
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 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.) |
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? |
Yes, there are the test properties |
@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 |
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 |
@milancurcic would you mind reviewing this also please? It looks like we all agree to merge it as is, and improve upon it later. |
@milancurcic ping. |
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.