Allow excluding of JDK signature checking in Jakarta EE Platform TCK testing #2

scottmarlow · 2021-04-06T16:22:27Z

First pass of addressing jakartaee/platform-tck#156, especially this part mentioned in the description:

There are some features in the sigtest tool that seem like they should work to limit the recorded signatures to only the API being tested, or to exclude the signatures for JDK classes, but these features have never worked the way I wanted them to work.

I am testing changes on https://github.com/scottmarlow/jakartaee-tck/tree/sigtest_netbeans-script-part2 to pass the Jakarta EE Platform TCK signature tests on JDK11 + JDK16 (with WildFly). One of the changes that I made in the sigtest_netbeans-script-part2 branch is to remove the JDK11 signature map files and only use the JDK8 signature map files for JDK11+ testing.

https://ci.eclipse.org/jakartaee-tck/job/jakartaee-tck-scottmarlow/job/sigtest_netbeans-script-part2/2 is building now and will test my local build of netbeans-apitest with the JDK11 version of GlassFish.

I am going to experiment more to see if I can eliminate the ClassCorrector + ThrowsNormalizer changes. I would prefer to limit changes to SignatureTest and would like to pass the list of JDK packages/classes to exclude as parameters.

I'm almost out of time to make these changes for Jakarta EE 9.1 (would like to finish by end of this week) but am seeing what can be accomplished.

Suggestions or alternatives are welcome.

scottmarlow · 2021-04-06T17:23:30Z

How could/should classes ClassCorrector + ThrowsNormalizer be passed the JDK classes to exclude?

Updated: Should we add a JDKExclude interface to represent the set of JDK packages/classes to be ignored by SignatureTest?

Updated: Should the JDKExclude instance be passed to {MemberCollectionBuilder, ClassCorrector, ThrowsNormalizer} for access to the JDK packages/classes to be ignored?

scottmarlow · 2021-04-06T17:30:46Z

I only updated a few classes to ignore JDK classes based on the actual test failures I saw during Jakarta EE TCK testing but there are likely other classes that also could/should ignore JDK packages/classes.

scottmarlow · 2021-04-06T18:25:36Z

Also note that I didn't regenerate the signature map files as we shouldn't need to actually exclude JDK references from signature map files as they are still needed there (e.g. SPEC API using java.util.Map should be verified by the class name reference).

src/main/java/com/sun/tdk/signaturetest/core/ClassCorrector.java

src/main/java/com/sun/tdk/signaturetest/core/MemberCollectionBuilder.java

src/main/java/com/sun/tdk/signaturetest/core/ThrowsNormalizer.java

src/main/java/com/sun/tdk/signaturetest/core/ClassCorrector.java

jtulach · 2021-04-08T03:56:33Z

Thank you guys for using and contributing to sigtest!

NetBeans project has noticed similar issues as well. A signature file generated on JDK8 differs from one generated with JDK11, etc. It is causing problems and I agree it deserves a fix.

Rather than excluding JDK classes, I'd prefer to use ct.sym approach (as used in javac since JDK9+). E.g. specify the --release of JDK API to compare against. Then it wouldn't matter which JDK is used for running the check - the JDK API used in the check would always be fixed to the specified --release version.

Would such approach work for you? If so, I'll try to investigate what it would take to use ct.sym. If that worked, I'd see it slightly preferable to the exclude JDK classes - but the solutions are orthogonal, so sigtest can support any or both approaches....

scottmarlow · 2021-04-08T13:37:32Z

Thank you guys for using and contributing to sigtest!

Thank you for the warm welcome! :-)

Rather than excluding JDK classes, I'd prefer to use ct.sym approach (as used in javac since JDK9+). E.g. specify the --release of JDK API to compare against. Then it wouldn't matter which JDK is used for running the check - the JDK API used in the check would always be fixed to the specified --release version.

Copy of some text https://openjdk.java.net/jeps/247 on ct.sym:

For JDK N and --release M, M < N, signature data of the documented APIs of release M of the platform is needed. This data is stored in the $JDK_ROOT/lib/ct.sym file, which is similar, but not the same, as the file of the same name in JDK 8. The ct.sym file is a ZIP file containing stripped-down class files corresponding to class files from the target platform versions.

For JDK N and --release N,the JDK's own image is used as the source of the class files to compile against. The list of observable modules is limited, however, to the documented modules and the jdk.unsupported module.

I know little about the ct.sym approach. The (early release) JDK17 ct.sym contains a lot of interesting looking folders that have .sig files that as mentioned above are class files. Folders that I see in ct.sym root from JDK17:

ls -1
7
79A
8
87
879
879A
879AB
879ABC
879ABCD
879ABCDE
879ABCDEF
879ABCDEFG
89
89A
89ABC
89ABCDEF
89ABCDEFG
9
9A
9AB
9ABC
9ABCDE
9ABCDEF
9ABCDEFG
A
AB
ABC
ABCD
ABCDEF
ABCDEFG
B
BC
BCD
BCDE
BCDEF
BCDEFG
BCDG
BDEF
C
CD
CDE
CDEF
CDEFG
D
DE
DEF
DEFG
E
EF
EFG
F
FG
G
H

One problem is that currently the Jakarta EE Platform produced TCKs contain JDK specific signature map files (been this way for a long time). The problem is that we are just now releasing Jakarta EE 9.1 (final release target of mid May but we are trying to finish development now). Our EE 9.1 release has JDK 8 + JDK 11 signature map files but we also want to support later JDK versions (assuming the JDK works in a compatible way).

With the current approach, we are able to pass the TCK signature testing on JDK 8, 11, 16, 17 with just using the base Java SE 8 signature map files.

Would such approach work for you?

If I understand correctly, when new JDK versions are released, the existing netbeans-apitest release that we come up with now, would be able to use the ct.sym classes that match the specified --release version. So, for Jakarta EE 9.1, we would likely pass in --release 8

Internally, sigtest_netbeans would load the classes from ct.sym that match the specified --release version, so essentially it would be like we are running on JDK 8, which sounds a lot cleaner!

If so, I'll try to investigate what it would take to use ct.sym. If that worked, I'd see it slightly preferable to the exclude JDK classes - but the solutions are orthogonal, so sigtest can support any or both approaches....

+1 for this new ct.sym approach! Do you know how to map the folders in ct.sym to the specified --release?

scottmarlow · 2021-04-08T14:00:11Z

Ahh, looks like the ct.sym folder names map to the JDK version, except the ct.sym folders are in hexadecimal. Sounds good now!

bstansberry · 2021-04-08T18:26:33Z

This comment is just to note my rough understanding of the ct.sym idea, in case I'm wrong. It sounds promising.

JDKs[1] include the lib/ct.sym file
JDK X where X > 8 includes the JDK 8 signatures in ct.sym
SignatureTest when it runs and needs to inspect a class, before loading the class will first try and access the sigs for 8 in ct.sym
The required signatures (i.e. the Jakarta EE API signatures) will not reference SE classes not in the SE 8 API. (This is a Jakarta EE requirement.) So, the signatures for referenced SE classes will all be available in the '8' folder of the JDK X's ct.sym. So long as the code under tests provides the proper EE APIs, the sigs from referenced SE classes will also come from the '8' folder of ct.sym and will match.

[1] Note the the adopt-openjdk JRE dists don't include a lib/ct.sym, which makes sense as they are not meant for compilation use cases. This does mean sig tests that rely on this approach would require use of a JDK, but Scott tells me the Jakarta EE TCK already has that requirement.

…hanges Signed-off-by: Scott Marlow <[email protected]>

…st tool that doesn't include JDK signatures, includes latest from jtulach/netbeans-apitest#2, add -IgnoreJDKClass option for specifying JDK classes to ignore Signed-off-by: Scott Marlow <[email protected]>

Signed-off-by: Scott Marlow <[email protected]>

…st tool that doesn't include JDK signatures, includes latest from jtulach/netbeans-apitest#2, add -IgnoreJDKClass option for specifying JDK classes to ignore Signed-off-by: Scott Marlow <[email protected]>

scottmarlow · 2021-04-09T13:47:15Z

Would such approach work for you? If so, I'll try to investigate what it would take to use ct.sym. If that worked, I'd see it slightly preferable to the exclude JDK classes - but the solutions are orthogonal, so sigtest can support any or both approaches....

@jtulach
+100 for both approaches.

Would it be possible to have a release soon that includes this PR?

Are there any further concerns about this PR?

Thanks,
Scott

jtulach · 2021-04-10T06:42:27Z

Are there any further concerns about this PR?

SigTest's functionality holds together only with the help of extensive test suite. Please subclass APITest and run the checks with the new functionality enabled. Please add some test(s) to show where the new option makes a difference.
Document the new functionality in README

Would it be possible to have a release soon that includes this PR?

Yes, once the above concerns are addressed. Btw. consider checking if #3 works for you. If so, you don't need to bother with the testing and documentation. I'll do it. Reviews of #3 welcomed!

jtulach · 2021-04-10T06:45:08Z

src/main/java/com/sun/tdk/signaturetest/SignatureTest.java

@@ -374,7 +383,8 @@ private boolean parseParameters(String[] args) {
        parser.addOption(UPDATE_FILE_OPTION, OptionInfo.option(1), optionsDecoder);
        parser.addOption(MODE_OPTION, OptionInfo.option(1), optionsDecoder);
        parser.addOption(ALLPUBLIC_OPTION, OptionInfo.optionalFlag(), optionsDecoder);
-
+        parser.addOption(EXCLUDE_JDK_CLASS_OPTION, OptionInfo.optionVariableParams(1, OptionInfo.UNLIMITED), optionsDecoder);


This option influences only command line interface. Neither integration with Maven or Ant! Are you running your checks from command line only?

This option influences only command line interface. Neither integration with Maven or Ant! Are you running your checks from command line only?

The Jakarta EE Platform TCK is tightly integrated with sigtest classes for running the signature tests via code like https://github.com/eclipse-ee4j/jakartaee-tck/blob/master/src/com/sun/ts/tests/signaturetest/SigTestDriver.java#L136

I'll give feedback on what I am seeing now with #3 on the pr.

scottmarlow · 2021-04-12T19:46:01Z

2. Document the new functionality in README

I will work on describing the new functionality in README and update the pull request. I'm not yet really sure about the testing as I think two JDKs would be needed (e.g. JDK 8 for the base class and a later JDK).

Would it be possible to have a release soon that includes this PR?

Yes, once the above concerns are addressed. Btw. consider checking if #3 works for you. If so, you don't need to bother with the testing and documentation. I'll do it. Reviews of #3 welcomed!

As mentioned on #3, testing against a base JDK 8 signature with a test class that was loaded by an external classloader (WildFly appclient container) didn't work as the externally loaded test had @deprecated annotations that have additional fields.

Perhaps we need a merge of some of #2 + all of #3?

jtulach · 2021-04-13T02:24:23Z

I'm not yet really sure about the testing as I think two JDKs would be needed (e.g. JDK 8 for the base class and a later JDK).

I thought the goal is to exclude some java.* classes from the generated .sigfile and that could be verified. Possibly that could also be tested on non-JDK classes.

Perhaps we need a merge of some of #2 + all of #3?

I am sad #3 isn't (yet) working for you. I'll try some other approach, but should #3 continue to not deliver on its promise. We need to integrate this PR in some form.

Signed-off-by: Scott Marlow <[email protected]>

scottmarlow · 2021-04-13T17:06:12Z

Are there any further concerns about this PR?

1. SigTest's functionality holds together only with the help of extensive test suite. Please subclass `APITest` and run the checks with the new functionality enabled. Please add some test(s) to show where the new option makes a difference.

I'm getting a Error: Unknown argument: -IgnoreJDKClass failure when building with minimal IgnoreJDKClassTest which only is adding the -IgnoreJDKClass option.

Which classes do I need to update to include the -IgnoreJDKClass option?

2. Document the new functionality in README

done via new commit just pushed today.

Would it be possible to have a release soon that includes this PR?

Yes, once the above concerns are addressed. Btw. consider checking if #3 works for you. If so, you don't need to bother with the testing and documentation. I'll do it. Reviews of #3 welcomed!

I'll go ahead and create an Eclipse CQ request to include this netbeans-apitest in the final changes we make by Thursday for including this change for the Jakarta EE 9.1 release. I'm just mentioning this so you are aware. If we don't complete this pr for EE 9.1, there will be other Jakarta EE releases in the future.

scottmarlow · 2021-04-13T19:39:26Z

👍

jtulach · 2021-04-13T19:41:29Z

Part of 1.4 release.

scottmarlow · 2021-04-13T19:45:52Z

Thank you @jtulach!

…st tool that doesn't include JDK signatures, includes latest from jtulach/netbeans-apitest#2, add -IgnoreJDKClass option for specifying JDK classes to ignore Signed-off-by: Scott Marlow <[email protected]>

…'t include JDK signatures, includes latest from jtulach/netbeans-apitest#2, add -IgnoreJDKClass option for specifying JDK classes to ignore Signed-off-by: Scott Marlow <[email protected]>

scottmarlow mentioned this pull request Apr 6, 2021

Need improved signature test tool that doesn't include JDK signatures jakartaee/platform-tck#156

Closed

scottmarlow force-pushed the sigtest_netbeans-script-part2 branch 3 times, most recently from 550dfe4 to 733b265 Compare April 7, 2021 19:29

bstansberry reviewed Apr 7, 2021

View reviewed changes

src/main/java/com/sun/tdk/signaturetest/core/ClassCorrector.java Show resolved Hide resolved

bstansberry reviewed Apr 7, 2021

View reviewed changes

src/main/java/com/sun/tdk/signaturetest/core/MemberCollectionBuilder.java Outdated Show resolved Hide resolved

bstansberry reviewed Apr 7, 2021

View reviewed changes

src/main/java/com/sun/tdk/signaturetest/core/ThrowsNormalizer.java Show resolved Hide resolved

bstansberry reviewed Apr 7, 2021

View reviewed changes

src/main/java/com/sun/tdk/signaturetest/core/ClassCorrector.java Show resolved Hide resolved

scottmarlow added a commit to scottmarlow/jakartaee-tck that referenced this pull request Apr 8, 2021

Add -IgnoreJDKClass and sync with latest jtulach/netbeans-apitest#2 c…

6c86b88

…hanges Signed-off-by: Scott Marlow <[email protected]>

scottmarlow added a commit to scottmarlow/jakartaee-tck that referenced this pull request Apr 8, 2021

Add -IgnoreJDKClass and sync with latest jtulach/netbeans-apitest#2 c…

0389de9

…hanges Signed-off-by: Scott Marlow <[email protected]>

Exclude use of JDK classes in SignatureTest via -IgnoreJDKClass option

a3bb665

Signed-off-by: Scott Marlow <[email protected]>

scottmarlow force-pushed the sigtest_netbeans-script-part2 branch from 61d09e9 to a3bb665 Compare April 8, 2021 19:29

scottmarlow marked this pull request as ready for review April 8, 2021 20:42

jtulach mentioned this pull request Apr 10, 2021

Use ct.sym to isolate sigtest from underlaying JDK #3

Merged

jtulach reviewed Apr 10, 2021

View reviewed changes

scottmarlow mentioned this pull request Apr 13, 2021

Review and promote Jakarta Transactions 2.0.1 TCK for JakartaEE 9.1 release jakartaee/platform-tck#700

Closed

4 tasks

Update README.md to describe the IgnoreJDKClass option

72e39bb

Signed-off-by: Scott Marlow <[email protected]>

jtulach merged commit 9d77e0e into jtulach:master Apr 13, 2021

scottmarlow mentioned this pull request Mar 1, 2022

HV-1887 Make Java 11 the base Java version hibernate/hibernate-validator#1235

Merged

scottmarlow mentioned this pull request Jul 2, 2024

issue 39 - Introduce -IgnoreJDKClasses option for ignoring JDK classes eclipse-ee4j/jakartaee-tck-tools#40

Merged

Allow excluding of JDK signature checking in Jakarta EE Platform TCK testing #2

Allow excluding of JDK signature checking in Jakarta EE Platform TCK testing #2

Uh oh!

Conversation

scottmarlow commented Apr 6, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

scottmarlow commented Apr 6, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

scottmarlow commented Apr 6, 2021

Uh oh!

scottmarlow commented Apr 6, 2021

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

jtulach commented Apr 8, 2021

Uh oh!

scottmarlow commented Apr 8, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

scottmarlow commented Apr 8, 2021

Uh oh!

bstansberry commented Apr 8, 2021

Uh oh!

scottmarlow commented Apr 9, 2021

Uh oh!

jtulach commented Apr 10, 2021

Uh oh!

jtulach Apr 10, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

scottmarlow Apr 12, 2021

Choose a reason for hiding this comment

Uh oh!

scottmarlow commented Apr 12, 2021

Uh oh!

jtulach commented Apr 13, 2021

Uh oh!

scottmarlow commented Apr 13, 2021

Uh oh!

scottmarlow commented Apr 13, 2021

Uh oh!

jtulach commented Apr 13, 2021

Uh oh!

scottmarlow commented Apr 13, 2021

Uh oh!

Uh oh!

scottmarlow commented Apr 6, 2021 •

edited

Loading

scottmarlow commented Apr 6, 2021 •

edited

Loading

scottmarlow commented Apr 8, 2021 •

edited

Loading

jtulach Apr 10, 2021 •

edited

Loading