On 2019-01-30 17:55, Alexandre (Shura) Iline wrote:
On Jan 30, 2019, at 8:53 AM, Erik Joelsson <erik.joels...@oracle.com> wrote:
On 2019-01-30 06:48, Magnus Ihse Bursie wrote:
On 2019-01-29 17:55, Erik Joelsson wrote:
Hello,
On 2019-01-29 03:12, Magnus Ihse Bursie wrote:
If you are going down this road, you'll need to verify that JCOV_IMAGE_DIR is
not empty in RunTests. So keep the ifneq statement, but with an $(error ...)
instead. Otherwise we'll just fail spectacularly and incomprehensible later on.
This has already gone in with my blessing, but to address your concern.
I don't see why we need to check that it's set in RunTest.gmk? In the local
case, we set JCOV_IMAGE_DIR in spec.gmk so it's guaranteed to be set,
... but only if we configure with --with-jcov.
I'd just like the system to present a comprehensible error message if the user does
something like this: "make test TEST=tier1 TEST_OPTS=JCOV=true", but has not
configured using --with-jcov. Prior to this patch, we ran the tests but without coverage.
This is probably not what was intended, but it didn't crash. Now we're going to get a
malformed command line.
"Error: Unable to access jarfile /lib/jcov.jar" is not a very helpful error
message.
When I test now, something is odd with the "jcov-image" target as well. Even if
you have not run configure with --with-jcov, the target seemingly runs and succeeds, but
no jcov image is generated. Expected behavior would be an error message stating that you
need to use --with-jcov.
Ah I see, but the previously existing conditional only checked if the variable
JCOV_IMAGE_DIR was set, not if the directory it pointed to existed, so it
wasn't any better. We set the variable JCOV_IMAGE_DIR unconditionally in
spec.gmk, so that check would always be true anyway.
What you are asking for is adding a new check that JCOV_HOME is set (this is a
consequence of setting --with-jcov) and that JCOV_IMAGE_DIR actually points to
something valid (this is the result of building jcov-image). It would also be
nice if the jcov-image target failed with a reasonable error if JCOV_HOME
wasn't set. I agree that these checks would be useful.
Yes, I think this pretty much sums up what I'm asking for.
I will fix that, unless somebody else really wants to take it.
Thanks Shura!
Re: the "Error: Unable to access jarfile /lib/jcov.jar" error message; I
got that by running configure without --with-jcov (note that you
automatically get --with-jcov if running using jib), and then running
"make jcov-test TEST=tier1" (or whatever test). This tried to start the
jcov reporter, but JCOV_HOME was not set.
Possibly all jcov operations should be checked that the value of
JCOV_HOME is non-empty, and fail with an error message otherwise. And
the testing should verify that the value of JCOV_IMAGE_DIR is non-empty
and contains bin/java$(EXE_SUFFIX).
/Magnus
Shura.
/Erik
/Magnus
and in the prebuilt case, it's the responsibility of RunTestPrebuilt.gmk to
adjust things for the lack of a proper spec.gmk and in RunTestPrebuilt.gmk we
already check that JDK_IMAGE_DIR points to an existing directory.
/Erik
/Magnus
28 jan. 2019 kl. 18:47 skrev Erik Joelsson <erik.joels...@oracle.com>:
Hello Shura,
In RunTest.gmk, you removed the conditional, but the comment still implies that
it's there. Please update the comment. Otherwise this looks good!
/Erik
On 2019-01-25 15:20, Alexandre (Shura) Iline wrote:
Hi,
Please take a look on a change to allow JCov test execution through jib.
Please notice that this fix also changes behavior of RunTest.gmk when
conflicting combination of options is used. JCOV_IMAGE_DIR is now expected to
be set when TEST_OPTS_JCOV is true.
Shura
Bug: https://bugs.openjdk.java.net/browse/JDK-8217761
Webrev: http://cr.openjdk.java.net/~shurailine/8217761/webrev.02/
