signaturetest/signature-repository/jakarta.el.sig_4.0_se11 has incorrect content

[brideck]

All of the se11 signature files were updated in PR https://github.com/eclipse-ee4j/jakartaee-tck/pull/391 except for the one for jakarta.el.

Per its header, it claims to contain an older version of the se8 signatures instead:

Signature file v4.1

Version 4.0_se8

This causes a failure in Open Liberty's latest attempts to run the signature tests:

[javatest.batch] 03-08-2021 14:01:26: SVR: SignatureTest report [javatest.batch] Base version: 4.0_se8 [javatest.batch] Tested version: 4.0_se11 [javatest.batch] Check mode: src [throws normalized] [javatest.batch] Constant checking: on [javatest.batch] [javatest.batch] Warning: Not found annotation type aQute.bnd.annotation.spi.ServiceConsumer [javatest.batch] [javatest.batch] Missing Classes [javatest.batch] --------------- [javatest.batch] [javatest.batch] jakarta.el.ExpressionFactory [javatest.batch] [javatest.batch] Added Classes [javatest.batch] ------------- [javatest.batch] [javatest.batch] jakarta.el.ExpressionFactory

[brideck]

@alwin-joseph Since you handled the original updates, do you happen to have a corrected version of this file handy that you could put into a PR? If not, I can take a look at performing the steps to generate a new proper file.

Note: That I do not have a current version of GF in my environment, so it would take me a little additional time to get set up to do this myself.

[markt-asf]

This doesn't look like a problem with the Java 11 signatures to me. That looks like the implementation under test has a aQute.bnd.annotation.spi.ServiceConsumer annotation on the ExpressionFactory.

While the use of that annotation is understandable (it is required if bnd is going to generate the correct OSGi meta-data) the annotation isn't present in the EL API project hence it isn't present in the signature tests.

There is a good argument for the signature tests ignoring this annotation (and probably a few other bnd annotations as well).

[brideck]

From looking at our EE9 signature test results, warnings do not typically cause failures in these tests, but it does look like this one may actually be indicating a problem. It does look like we made an update to the EL API in the last month, so I'll have to circle back with our web tier developers.

That doesn't excuse the fact that this file is very clearly not correct, even if my failure here is a complete coincidence. https://github.com/eclipse-ee4j/jakartaee-tck/blob/master/src/com/sun/ts/tests/signaturetest/signature-repository/jakarta.el.sig_4.0_se11

[markt-asf]

The warning is a clue to the likely root cause. The failure is because the ExpressionFactory signatures are not an exact match.

The only thing I can see wrong with that file is a copy/paste error in the comment header. Is that the clear error you are referring to or is there something else that might be wrong?

[brideck]

I don't think it's a copy/paste error. All of the se11 files were updated anew in the PR that I referenced. Even if the differences end up being merely superficial, it's clear that this one was not regenerated and included as part of that PR. Just an oversight, I'm sure.

If nothing else, it leads to this confusing bit of output, which is what led me down this path in the first place.

[javatest.batch] Base version: 4.0_se8 [javatest.batch] Tested version: 4.0_se11

[brideck]

The warning is a clue to the likely root cause. The failure is because the ExpressionFactory signatures are not an exact match.

Open Liberty pulls the EL API from Tomcat, so it looks like your assessment is correct. That annotation was added to ExpressionFactory there in November, but OL just updated to a new rev of the API in the last month.

Regardless, I will maintain that a file update should be made in the TCK.

[markt-asf]

Regardless, I will maintain that a file update should be made in the TCK.

On what basis? Apart from the copy/paste error (and it is a copy/paste error because I made the error), the file is correct.

[brideck]

Regardless, I will maintain that a file update should be made in the TCK.

On what basis? Apart from the copy/paste error (and it is a copy/paste error because I made the error), the file is correct.

Where does the test get the output it's showing for "Base version"? jakarta.el is the only package in the signature test that is showing "se8" there when I run with Java 11.

I can confirm that this does indeed come from the version info in the header of the signature file.

[volosied]

@markt-asf

Following up on this issue... From my understanding, this annotation (added under BZ 64849 which addresses a valid Tomcat Embedded EL bug). However, it created this signature TCK failure.

Tomcat also needs to be TCK-compliant, but I don't imagine there is workaround for Tomcat in this instance?

Basically, as you said in a previous comment, the best way forward is to have the TCK be updated somehow to mark this annotation as optional?

Edit -- Are there examples (or documentation) to ignore annotations? I'm not finding anything? Maybe the logic might need to be added.

[scottmarlow]

IMO, it would be good to get input from others on the Platform TCK mailing list on whether all annotations should be verified on SPEC APIs or only certain ones. Currently, we are using tooling that does verify:

• no super setting.
• no sub setting.
• super classes match.
• field/methods match exactly.
• all annotations signatures match exactly (e.g. do not allow additional annotations to be present in SPEC API).

We do not currently have a way to make the EL signature testing optionally,

Nor do we currently have a way to ignore annotations like aQute.bnd.annotation.spi.ServiceConsumer or whatever other unexpected annotations that have been added by a SPEC API library.

Any volunteers to start an email on https://accounts.eclipse.org/mailing-list/jakartaee-tck-dev asking if our signature testing should continue to verify annotation signatures match exactly?

[markt-asf]

email sent

[gurunrao]

@brideck - generated new signature file using jakarta.el-api.jar present in https://download.eclipse.org/ee4j/glassfish/glassfish-6.1.0-SNAPSHOT-nightly.zip Signature file can be found here - https://github.com/gurunrao/jakartaee-tck/blob/d798c47e52687a32fd03b9e7e508a351475dbb8f/src/com/sun/ts/tests/signaturetest/signature-repository/jakarta.el.sig_4.0_se11 Diffs - https://github.com/gurunrao/jakartaee-tck/commit/d798c47e52687a32fd03b9e7e508a351475dbb8f

Let me know if above signature file helps. CI run with glassfish 6.1 - https://ci.eclipse.org/jakartaee-tck/job/guru/job/jakartaee-tck-guru/job/el-signature-test/9/ CC @scottmarlow @markt-asf

[scottmarlow]

@gurunrao since there are lines moved like CLSS public abstract interface java.io.Serializable it is harder to read the diff as I'm not sure of which lines are different or just moved around.

Do you already know which lines are actually different as apposed to just moved?

[gurunrao]

@gurunrao since there are lines moved like CLSS public abstract interface java.io.Serializable it is harder to read the diff as I'm not sure of which lines are different or just moved around.

Do you already know which lines are actually different as apposed to just moved?

@scottmarlow - since its a generated file, I have added file as generated. Also there are no significant difference. Diff seen are:

• https://github.com/gurunrao/jakartaee-tck/commit/d798c47e52687a32fd03b9e7e508a351475dbb8f#diff-a70b66b744cb8bdfe8d25f15e7fea535b5107cf2dfafe7133d7ccd3f380a71fcR349
• https://github.com/gurunrao/jakartaee-tck/commit/d798c47e52687a32fd03b9e7e508a351475dbb8f#diff-a70b66b744cb8bdfe8d25f15e7fea535b5107cf2dfafe7133d7ccd3f380a71fcR2
• https://github.com/gurunrao/jakartaee-tck/commit/d798c47e52687a32fd03b9e7e508a351475dbb8f#diff-a70b66b744cb8bdfe8d25f15e7fea535b5107cf2dfafe7133d7ccd3f380a71fcR357

[brideck]

The update to the header satisfies my request to fix the misleading test output:

[javatest.batch] 03-30-2021 13:06:38:  SVR: SignatureTest report
[javatest.batch] Base version: 4.0_se11
[javatest.batch] Tested version: 4.0_se11

My actual failure obviously won't be fixed until the ServiceConsumer annotation situation is resolved one way or the other, but I think what you have here is sufficient to resolve this particular issue.

[gurunrao]

@brideck - i have merged header changes - https://github.com/eclipse-ee4j/jakartaee-tck/commit/ad268cb84d05792f83d95ab85e87466d025a3d2d

[scottmarlow]

aQute.bnd.annotation.spi.ServiceConsumer annotation on the ExpressionFactory

As per concern raised on the Platform TCK mailing list about the risk/danger of allowing supersetting via adding (unexpected) annotations to a SPEC API implementation, I added the future label.

[rotty3000]

I'm not sure I understand what possible side effect a CLASS retention annotation could possibly have.

[bjhargrave]

Class retention annotations can not be considered "API" as they cannot have any bearing on runtime behavior. They support tooling generating proper metadata and put that information in the source code rather than in external configuration like pom files.

Signature tests should ignore class retention annotations.

[kwsutter]

Signature tests should ignore class retention annotations.

@bjhargrave and @rotty3000, this is a fair discussion to have. But, we're not going to rush through that type of change late in the 9.1 release. That's why @scottmarlow added the future label. We need a full-fledged discussion on whether any of these annotations should be allowed for signature tests. It would not be proper to make a hasty decision to allow for this exception.

Does this issue need to be broken into two parts? One for the immediate concern with the EL signature tests, which it seems that @gurunrao and @brideck are making progress with. And, one for this "allow extra annotations in APIs" future request?

[scottmarlow]

Does this issue need to be broken into two parts? One for the immediate concern with the EL signature tests, which it seems that @gurunrao and @brideck are making progress with. And, one for this "allow extra annotations in APIs" future request?

The immediate concern with the incorrect Version 4.0_se8 reference in the JDK11 EL signature map file was addressed by https://github.com/eclipse-ee4j/jakartaee-tck/pull/681. No new issue is needed for that PR, it was a minor change and also included a script change to help avoid seeing the same issue again in the future.

:+1: for updating this issue to reflect the pending discussion.