Re: RFR JDK-8217761: Enhance run-test-prebuilt jib profile to support running tests with JCov
On 2019-01-30 17:55, Alexandre (Shura) Iline wrote: On Jan 30, 2019, at 8:53 AM, Erik Joelsson 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 : 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/
Re: RFR JDK-8217761: Enhance run-test-prebuilt jib profile to support running tests with JCov
> On Jan 30, 2019, at 6:48 AM, 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. Do you have the exact command to reproduce that? > > /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 : 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/ >
Re: RFR JDK-8217761: Enhance run-test-prebuilt jib profile to support running tests with JCov
> On Jan 30, 2019, at 8:53 AM, Erik Joelsson 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. I will fix that, unless somebody else really wants to take it. 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 : > > 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/ >>
Re: RFR JDK-8217761: Enhance run-test-prebuilt jib profile to support running tests with JCov
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. /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 : 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/
Re: RFR JDK-8217761: Enhance run-test-prebuilt jib profile to support running tests with JCov
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. /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 : 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/
Re: RFR JDK-8217761: Enhance run-test-prebuilt jib profile to support running tests with JCov
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, 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 : 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/
Re: RFR JDK-8217761: Enhance run-test-prebuilt jib profile to support running tests with JCov
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. /Magnus > 28 jan. 2019 kl. 18:47 skrev Erik Joelsson : > > 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/
Re: RFR JDK-8217761: Enhance run-test-prebuilt jib profile to support running tests with JCov
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/
RFR JDK-8217761: Enhance run-test-prebuilt jib profile to support running tests with JCov
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/
