eclipse-jdt / eclipse.jdt.core

Eclipse Public License 2.0
164 stars 130 forks source link

The split package org.eclipse.jdt.annotation has jar signature problems: #3027

Closed merks closed 1 week ago

merks commented 1 month ago

The repository analyzer job

https://ci.eclipse.org/oomph/job/repository-analyzer/

reported this problem today:

The package 'org.eclipse.jdt.annotation 0.0.0' is jar-signed differently by the containing IUs

The package 'org.eclipse.jdt.annotation 0.0.0' is jar-signed differently by the containing IUs
org.eclipse.jdt.annotation 1.2.100.v20240927-1034
  {CN=Eclipse.org Foundation, Inc., O=Eclipse.org Foundation, Inc., L=Ottawa, ST=Ontario, from=2024-07-22, to=2025-07-21}
    {CN=DigiCert Trusted G4 Code Signing RSA4096 SHA384 2021 CA1, O=DigiCert, Inc., from=2021-04-29, to=2036-04-28}
      {CN=DigiCert Trusted Root G4, OU=www.digicert.com, O=DigiCert Inc, from=2013-08-01, to=2038-01-15}
org.eclipse.jdt.annotation 2.3.0.v20240111-2306
  {CN=Eclipse.org Foundation, Inc., OU=IT, O=Eclipse.org Foundation, Inc., L=Ottawa, ST=Ontario, from=2022-05-02, to=2024-05-21}
    {CN=DigiCert Trusted G4 Code Signing RSA4096 SHA384 2021 CA1, O=DigiCert, Inc., from=2021-04-29, to=2036-04-28}
      {CN=DigiCert Trusted Root G4, OU=www.digicert.com, O=DigiCert Inc, from=2013-08-01, to=2038-01-15}

This recent commit resulted in a qualifier-only change to org.eclipse.jdt.annotation 1.2.100.v20240927-1034:

https://github.com/eclipse-jdt/eclipse.jdt.core/pull/2994

But I think the error checking for qualifier only changes is disabled by this in the build.properties, which I expect is also the reason for the qualifier change:

image

So now it's a question of what to do? Neither of these two will be published to Maven Central for the next release because only unqualified versions are released. Of course nothing has changed that would argue for a new released version on Maven Central. So while we could force a qualifier-only change for org.eclipse.jdt.annotation 2.3.0.v20240111-2306, that seems odd/wrong because we generally avoid qualifier-only changes and I would expect the baseline comparison to complain about that.

So I seems the proper way to fix this is to increase by +100 the service/micro version of each.

merks commented 1 month ago

@HannesWell

Is it a bit of a short-coming in the pomless approach that while changes to the pom.xml do not result in qualifier changes, changes to the build.properties do result in qualifier changes? Or is this a problem with respect to which files are ignored by the jgit timestamp configuration? I assume that's the problem here and I assume we'd have to be pretty careful if changing the latter to ignore the build.properties that versions do go backwards...

akurtakov commented 1 month ago

I remember discussions about no longer building/shipping o.e.jd.annotation_v1 as it's not supposed to change at all. I don't know how the fact that ecj no longer supports targets bellow 1.8 affects the relevance of v1 (if at all) but it's a relevant question to ask again from releng POV.

stephan-herrmann commented 1 month ago

I'm curious, why package org.eclipse.jdt.annotation appears as a split package. Is it because JDT test bundles require both versions? Other than that nobody should ever see both versions of the same annotations, as that would result in duplicate types of the same qualified name, actually. This is not what split packages are normally about, where classes from different package fragments are merged.

I remember discussions about no longer building/shipping o.e.jd.annotation_v1 as it's not supposed to change at all. I don't know how the fact that ecj no longer supports targets bellow 1.8 affects the relevance of v1 (if at all) but it's a relevant question to ask again from releng POV.

Thanks for raising the question.

Version 2.x of this bundle requires Java 8, but there is no inverse constraint saying that v1 does not work with Java 8.

Users are, however, strongly encouraged to use 2.x.

Some users rely on pre-Java8 semantics, because they are using other (non-jdt) annotations, for which no Java8 variant has been published. This could be irrelevant for JDT, but the mix of old and new semantics is not well supported. Hence users may still opt to use o.e.j.annotation_v1 on order to be compatible with 3rd party annotations (which perhaps are pulled in from libraries which they use).

One reference to o.e.j.annotation_v1 still theoretically exists in JDT/UI, but that is "dead code" now, over to https://github.com/eclipse-jdt/eclipse.jdt.ui/issues/1685.

Summarizing: it is still possible that users use o.e.j.annotation_v1 with latest Eclipse versions, but

Ergo: I wouldn't mind if we stop building that v1 library right now, in which case we only need a way to ensure that JDT/Core tests still find a previously built version.

HannesWell commented 1 month ago

But I think the error checking for qualifier only changes is disabled by this in the build.properties, which I expect is also the reason for the qualifier change:

That's right. And this indeed has the consequence that the actually necessary version bump was not checked/enforced/done.

So I seems the proper way to fix this is to increase by +100 the service/micro version of each.

Agree, #3028 seems the right thing to do.

Instead of disabling the version check entirely, we could also make it more configurable so that the matching artifact can be found in the baseline:

This way the necessary bump would have been reported in the build as failure.

Of course if we don't build it anymore at all, that would also solve this.

stephan-herrmann commented 1 month ago

@brychcy @Bananeweizen as power-users of and contributors to null annotations. Do you want to add anything to my previous comment?

akurtakov commented 1 month ago

@stephan-herrmann I am a bit confused, if https://github.com/eclipse-jdt/eclipse.jdt.core/blob/7cd2e50b007ab39cc0f106cb10c47e5fb6fe8ad5/org.eclipse.jdt.annotation_v1/META-INF/MANIFEST.MF#L8 (done via https://github.com/eclipse-jdt/eclipse.jdt.core/commit/5b44289ca54ef01687bd492c7b058ed8041495ab ) says it requires Java 8, how is it being used in pre Java 8?

stephan-herrmann commented 1 month ago

@stephan-herrmann I am a bit confused, if

https://github.com/eclipse-jdt/eclipse.jdt.core/blob/7cd2e50b007ab39cc0f106cb10c47e5fb6fe8ad5/org.eclipse.jdt.annotation_v1/META-INF/MANIFEST.MF#L8 (done via 5b44289 ) says it requires Java 8, how is it being used in pre Java 8?

Ups, I didn't even know about that change. But, this still doesn't significantly change the situation.

Maybe I should be more specific than saying "pre-Java8 semantics": The distinction is whether or not the annotations specify @Target(TYPE_USE) which was impossible before Java 8, but using Java 8 does not imply @Target(TYPE_USE), ie., in this aspect even projects with compliance 1.8 can still use the "old" semantics for null annotations, i.e., declaration annotations.

merks commented 1 month ago

As a data point, this wouldn't be the first time as far as I can tell:

image

stephan-herrmann commented 1 month ago

If anyone is blocked by the current situation, then I'm fine with the version bump in #3028, but I suggest that we stop building o.e.j.annotation_v1 before the next release, to avoid publishing yet another meaningless update.

akurtakov commented 1 month ago

If anyone is blocked by the current situation, then I'm fine with the version bump in #3028, but I suggest that we stop building o.e.j.annotation_v1 before the next release, to avoid publishing yet another meaningless update.

This is the best step IMO too. Please let me know if/how I can help to make this happen.

merks commented 1 month ago

Nothing is blocked and I'm more than happy to see the PR closed in favor of cleaning up! So please close the PR when there is a better solution.

stephan-herrmann commented 1 week ago

We no longer build & ship the v1 bundle.

Also the split package disappeared from https://download.eclipse.org/oomph/archive/reports/download.eclipse.org/eclipse/updates/4.34-I-builds/https___download.eclipse.org_eclipse_updates_4.34-I-builds_I20241024-1800.html

Closing as fixed by