-
Notifications
You must be signed in to change notification settings - Fork 352
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
Skip some tests if gcc is not installed #1510
Conversation
@@ -5,7 +5,7 @@ | |||
analyze mydesign.vhdl | |||
elab myentity | |||
|
|||
if ghdl_has_feature myentity vpi; then | |||
if c_compiler_is_available && ghdl_has_feature myentity vpi; then |
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.
Instead of skipping each vpi test, I would skip the whole (sub)testsuite at the top of https://github.com/LukasVik/ghdl/blob/testsuite_gcc/testsuite/vpi/testsuite.sh
if [ -z $CC ]; then | ||
CC="gcc" | ||
fi | ||
if c_compiler_is_available; then |
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.
Please, echo a message and exit 0
, instead of wrapping all the commands. See https://github.com/ghdl/ghdl/blob/master/testsuite/gna/bug032/testsuite.sh#L3-L7
|
||
# We're just checking that ghwdump doesn't crash on a zero length signal. | ||
./ghwdump -ths dump.ghw > dump.txt | ||
gcc ../../../src/grt/ghwdump.c ../../../src/grt/ghwlib.c -I../../../src/grt/ -o ghwdump |
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.
BTW, maybe use $CC
instead of gcc
, in coherence with other tests.
fi | ||
which $CC | ||
} | ||
|
||
# Check if a feature is present | ||
ghdl_has_feature () |
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.
Might be worth calling c_compiler_is_available
from inside ghdl_has_feature
. That will avoid most of the modifications to individual tests (except bug097 and issue1326).
I am not against that PR, but I'd like to suggest something simpler. If you aim at reducing the size of the docker image, just remove the testsuite (or maybe just keep the sanity/ part). |
That is already the case. The testsuite is not part of the image. The procedure is as follows:
So, the 'production' image (ghdl/ghdl) currently contains the following only:
Lukas' proposal is the following:
I am not sure about Note: I will take care of ensuring that the whole testsuite is executed with mcode backend and GCC (at least once), even if production images don't support the VPI testsuite by default. Note: there are now ghdl/cosim images available, which do provide VPI support by default. Those contain mcode + GCC only, or LLVM + VUnit + cocotb, or + Xyce, or + Octave.... |
But when is the testsuite executed ? You can run the whole testsuite after the build, and simply the sanity check later. |
The point is to run the testsuite in a "clean" environment, not the one with all the build dependencies. So, the testsuite is executed in a container with We do it like that because we want to replicate the experience of a user which installs a pre-built package and attempts to run the full testsuite (or any specific test). Precisely, if we run the tests in the build environment and only the sanity check "outside", then we would not detect the difference of GCC being available on the run environment. |
Thanks! |
Background
Related to ghdl/docker#34.
We want to have a docker image with ghdl that is minimal in size. That implies not including a gcc installation in the image. We do however want to run the testsuite to make sure the docker image "works".
Description
This PR makes it possible to run the testsuite on a system that does not have gcc installed. Tests that require gcc will be skipped (reported as "ok").