Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Bump Jackson to 2.13.0 #91

Closed
wants to merge 2 commits into from
Closed

Conversation

basil
Copy link
Member

@basil basil commented Sep 3, 2021

Amends #94.

dependabot bot and others added 2 commits October 1, 2021 07:14
Bumps `jackson.version` from 2.12.4 to 2.13.0.

Updates `jackson-datatype-jsr310` from 2.12.4 to 2.13.0

Updates `jackson-datatype-jdk8` from 2.12.4 to 2.13.0

Updates `jackson-datatype-json-org` from 2.12.4 to 2.13.0
- [Release notes](https://github.com/FasterXML/jackson-datatypes-misc/releases)
- [Commits](FasterXML/jackson-datatypes-misc@jackson-datatypes-misc-parent-2.12.4...jackson-datatypes-misc-parent-2.13.0)

Updates `jackson-module-parameter-names` from 2.12.4 to 2.13.0
- [Release notes](https://github.com/FasterXML/jackson-modules-java8/releases)
- [Commits](FasterXML/jackson-modules-java8@jackson-modules-java8-2.12.4...jackson-modules-java8-2.13.0)

Updates `jackson-core` from 2.12.4 to 2.13.0
- [Release notes](https://github.com/FasterXML/jackson-core/releases)
- [Commits](FasterXML/jackson-core@jackson-core-2.12.4...jackson-core-2.13.0)

Updates `jackson-annotations` from 2.12.4 to 2.13.0
- [Release notes](https://github.com/FasterXML/jackson/releases)
- [Commits](https://github.com/FasterXML/jackson/commits)

Updates `jackson-module-jaxb-annotations` from 2.12.4 to 2.13.0
- [Release notes](https://github.com/FasterXML/jackson-modules-base/releases)
- [Commits](FasterXML/jackson-modules-base@jackson-modules-base-2.12.4...jackson-modules-base-2.13.0)

Updates `jackson-dataformat-xml` from 2.12.4 to 2.13.0
- [Release notes](https://github.com/FasterXML/jackson-dataformat-xml/releases)
- [Commits](FasterXML/jackson-dataformat-xml@jackson-dataformat-xml-2.12.4...jackson-dataformat-xml-2.13.0)

Updates `jackson-dataformat-yaml` from 2.12.4 to 2.13.0
- [Release notes](https://github.com/FasterXML/jackson-dataformats-text/releases)
- [Commits](FasterXML/jackson-dataformats-text@jackson-dataformats-text-2.12.4...jackson-dataformats-text-2.13.0)

---
updated-dependencies:
- dependency-name: com.fasterxml.jackson.datatype:jackson-datatype-jsr310
  dependency-type: direct:production
  update-type: version-update:semver-minor
- dependency-name: com.fasterxml.jackson.datatype:jackson-datatype-jdk8
  dependency-type: direct:production
  update-type: version-update:semver-minor
- dependency-name: com.fasterxml.jackson.datatype:jackson-datatype-json-org
  dependency-type: direct:production
  update-type: version-update:semver-minor
- dependency-name: com.fasterxml.jackson.module:jackson-module-parameter-names
  dependency-type: direct:production
  update-type: version-update:semver-minor
- dependency-name: com.fasterxml.jackson.core:jackson-core
  dependency-type: direct:production
  update-type: version-update:semver-minor
- dependency-name: com.fasterxml.jackson.core:jackson-annotations
  dependency-type: direct:production
  update-type: version-update:semver-minor
- dependency-name: com.fasterxml.jackson.module:jackson-module-jaxb-annotations
  dependency-type: direct:production
  update-type: version-update:semver-minor
- dependency-name: com.fasterxml.jackson.dataformat:jackson-dataformat-xml
  dependency-type: direct:production
  update-type: version-update:semver-minor
- dependency-name: com.fasterxml.jackson.dataformat:jackson-dataformat-yaml
  dependency-type: direct:production
  update-type: version-update:semver-minor
...

Signed-off-by: dependabot[bot] <support@github.com>
….activation-api:1.2.1 paths to dependency are:

+-org.jenkins-ci.plugins:jackson2-api:2.12.5-SNAPSHOT
  +-com.fasterxml.jackson.module:jackson-module-jaxb-annotations:2.13.0
    +-jakarta.activation:jakarta.activation-api:1.2.1
and
+-org.jenkins-ci.plugins:jackson2-api:2.12.5-SNAPSHOT
  +-com.fasterxml.jackson.module:jackson-module-jaxb-annotations:2.13.0
    +-jakarta.xml.bind:jakarta.xml.bind-api:2.3.3
      +-jakarta.activation:jakarta.activation-api:1.2.2
@basil basil changed the title Bump Jackson to 2.13.0-rc2 Bump Jackson to 2.13.0 Oct 1, 2021
@basil basil marked this pull request as ready for review October 1, 2021 15:42
Copy link

@MarkEWaite MarkEWaite left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I confirmed that earlier versions of the plugin were bundling jakarta.activation-api 1.2.1 as a transitive dependency. The added dependency on jakarta.activation-api 1.2.2 makes it clear that it is intentionally bundled.

Supersedes #94 and #95

@timja timja added the dependencies Pull requests that update a dependency file label Oct 1, 2021
@timja timja requested a review from a team October 1, 2021 17:11
@jtnord
Copy link
Member

jtnord commented Oct 1, 2021

The added dependency on jakarta.activation-api 1.2.2 makes it clear that it is intentionally bundled.

there is no added dependency - it is a dependencyManageent core (2.303.1ships 1.2.2 and as such if this is bundled without using mask-classes or plugin-first classloaded then it will not be used correctly and can even cause weird classloading issues if there are newly introduced classes.
If the dependency is needed then the classloader needs to be tweaked, otherwise it should be excluded, unless I am missing something

Comment on lines +97 to +101
<dependency>
<groupId>jakarta.activation</groupId>
<artifactId>jakarta.activation-api</artifactId>
<version>1.2.2</version>
</dependency>
Copy link
Member

@jtnord jtnord Oct 1, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

jenkins ships with a lesser version (of jakarta.activation which itself bundles the same classes as the API 😱 ). if this is bundled in the plugin it is never used as the plugin is not using a plugin first classloader, so the fix should be to exclude the version instead.
if it is not bundled - then this means the test classpaths are more likely to more even invalid that normal.

@MarkEWaite
Copy link

MarkEWaite commented Oct 1, 2021

The added dependency on jakarta.activation-api 1.2.2 makes it clear that it is intentionally bundled.

there is no added dependency - it is a dependencyManageent core (2.303.1ships 1.2.2 and as such if this is bundled without using mask-classes or plugin-first classloaded then it will not be used correctly and can even cause weird classloading issues if there are newly introduced classes. If the dependency is needed then the classloader needs to be tweaked, otherwise it should be excluded, unless I am missing something

I can't speak to the question whether the dependency is needed or not. I've confirmed that WEB-INF/lib/jakarta.activation-api-1.2.1.jar is included in jackson2-api plugin 2.12.4. The inclusion of WEB-INF/lib/jakarta.activation-api-1.2.2.jar as a transitive dependency in the jackson2-api plugin 2.13.0 release seems consistent with the previous release.

The build log for the jackson2-api plugin master branch (before this change) reports that the activation api is being bundled as a transitive dependency.

@jtnord
Copy link
Member

jtnord commented Oct 1, 2021

https://projects.eclipse.org/projects/ee4j.jaf/releases/1.2.2

First release going through the JESP. No changes from the previous release (1.2.1).

@@ -68,7 +68,7 @@
<changelist>-SNAPSHOT</changelist>
<java.level>8</java.level>
<jenkins.version>2.222.4</jenkins.version>
<jackson.version>2.12.4</jackson.version>
<jackson.version>2.13.0</jackson.version>
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

should also change revision to keep in step (not sure why we do not set that itself to $jackon.version but likely it does not work when it comes to release time.

@basil
Copy link
Member Author

basil commented Oct 1, 2021

As @MarkEWaite wrote, this is a preëxisting issue on the main branch that is unrelated to this PR. Get the main branch in order before you request changes of others.

@basil basil closed this Oct 1, 2021
@jtnord
Copy link
Member

jtnord commented Oct 1, 2021

Get the main branch in order before you request changes of others.

is offensive to me.

I was about to send a PR to your fork with the fixes, but how this is coming across is not helpful to a healthy community.

@basil basil deleted the jackson-2.13.0-rc2 branch October 1, 2021 18:29
@timja
Copy link
Member

timja commented Oct 1, 2021

What was the suggested fix? excluding the dependency, upgrading it in jenkins core? or something else?

@jtnord
Copy link
Member

jtnord commented Oct 1, 2021

What was the suggested fix? excluding the dependency, upgrading it in jenkins core? or something else?

it would depend on what changed API wise. As per the eclipse release notes, what I am doing is excluding it (given if it currently works when bundled, it must work when not bundled as the plugin is not using plugin-first classloader or masking classes).
the version was bumped from the old activation-hudson-1.1.1 in jenkinsci/jenkins#4660 so this is also bumping the baseline to we get the 1.2.1 version so there can not be conflicts between the 1.1 and 1.2 lines).

But anyway - the version of the plugin can not be 2.12.5-SNAPSHOT when the api is at 2.13.0 there is a PR incoming anyway to fix the bundling - but that is a 1:1 like for like and not bumping jackson in anyway shape or form

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
dependencies Pull requests that update a dependency file
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants