[Fortran/gfortran] Switch to using the static test configuration files #97
Conversation
Switches the existing build system to use the static test configuration files that were added to the repository in previous commits. The previous method is jettisoned in its entirety. Several tests have been disabled in the process. Some of these are because of limitations in the build system which will be fixed in the future. Others ought to have failed earlier but did not because they were not being built with the correct compiler flags. Now that they are built correctly, they expose limitations of flag which will need to be fixed.
There was a problem hiding this comment.
Disabling the tests that should have failed earlier but did not is a good thing, but I'm a bit worried that we may lose some test coverage with this change, because of the other disabled tests.
You mentioned that some tests were disabled because of limitations in the build system which will be fixed in the future. Does this mean that most tests that currently pass (and are supposed to pass) and were disabled are going to be enabled again in the near future?
| # These files are only intended to be run on AArch64, but we don't currently | ||
| # process the target attribute, so these are disabled everywhere. When the | ||
| # DejaGNU target attribute is handled correctly, these should be removed from | ||
| # here. | ||
| pr101158.f90 | ||
| pr88833.f90 | ||
| pr98974.F90 |
There was a problem hiding this comment.
Were these tests passing by luck on other targets?
There was a problem hiding this comment.
These tests have additional flags that use -march or -mcpu options whose values are not valid on x86-64. These used to work because both the target annotations and the flags were ignored before. I think the target attribute is handled correctly in this patch. These are probably tests that got added by accident (see the note above). I can double check and remove it right now. Otherwise, they should get removed in the next patch which will enable number of tests that now pass.
There was a problem hiding this comment.
I guess it's ok to leave them disabled while we don't support the target attribute. I was just curious to know whether they worked on other targets before, but there is no need to dig further into this.
| # These tests have a -J flag but the build system adds a -J of its own and | ||
| # exactly one is allowed. If the build system is changed, these can be removed | ||
| # from here. |
There was a problem hiding this comment.
By build system do you mean the set of CMake files for the gfortran test suite? And that some adjustments on these are needed to properly support -J?
There was a problem hiding this comment.
Yes.
It has to do with the way the gfortran tests are built. There are several tests which generate .mod files with the same name which causes race conditions in parallel builds. To work around this, there is a unique working directory for each test and I ended up adding a -J flag when building the tests. There might be a way to do it without requiring a -J flag, but I haven't gotten around to looking into it yet.
There was a problem hiding this comment.
I see. As -J was already used before, is this another case of tests that worked by accident?
There was a problem hiding this comment.
I think, in this case, the flags were not being passed while building the test at all because the DejaGNU annotation containing it was not being parsed correctly.
There was a problem hiding this comment.
Ok. I took a look at the include tests below and saw that they expect some warnings to be emitted (or omitted), which is a kind of test that we don't support anyway.
There was a problem hiding this comment.
Yes.
You have identified several limitations with the way we are using the gfortran tests. One of the improvements that can be made is to map gfortran warnings to equivalent flang warnings where possible and check for those. Currently, warnings are completely ignored.
We do check for the presence of an error when a test is expected to produce one, but we do not check if it is an "equivalent" error. That can be improved as well.
Yes. I think with the way things were before, the dependent files were being run as standalone "compile" tests. There were probably other bugs as well. A follow-up patch will enable several failing tests. The number should go up again, but it will still be less than before. |
Does this mean that most tests that currently pass (and are supposed to pass) and were disabled are going to work again? |
A number of tests are setup incorrectly by the current build system. For example, source files that are dependencies of some tests but are currently set up as "compile" tests in their own right. The output is not checked in any way, simply that these files compile. I also suspect that there are some files that are being compiled as both "compile" and "execute" tests but I have not checked. With this PR, those "spurious" tests will not be run, and the overall test count will decrease. What I meant by the previous comment is that there are a number of tests that are currently disabled but can be enabled because whatever was causing them to fail has been fixed (or implemented). Apologies for the confusion. |
Switches the existing build system to use the static test configuration files that were added to the repository in previous commits. The previous method is jettisoned in its entirety. Several tests have been disabled in the process. Some of these are because of limitations in the build system which will be fixed in the future. Others ought to have failed earlier but did not because they were not being built with the correct compiler flags. Now that they are built correctly, they expose limitations of flag which will need to be fixed.
[edit] Notes for reviewers: