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

Update to the newest stable release of kryo #50

Merged
merged 2 commits into from
Jul 24, 2020
Merged

Update to the newest stable release of kryo #50

merged 2 commits into from
Jul 24, 2020

Conversation

ctrueden
Copy link
Member

@ctrueden ctrueden commented May 8, 2020

From 2.24.0 to 3.0.0, the kryo project changed its groupId from com.esotericsoftware.kryo to com.esotericsoftware. Therefore, from Maven's perspective, both of these artifacts can be present on the classpath at the same time, creating duplicate class conflicts. Unfortunately, this scenario is happening when attempting to combine org.openmicroscopy:ome-common:6.0.4 (which wants kryo 2.24.0) with graphics.scenery:scenery:0.7.0-beta-7 (which wants kryo 4.0.2).

One solution to this dilemma is to update the version of kryo here to 4.0.2, which is what this PR does. The code still compiles, and tests pass, although I am not certain how thorough (if at all) the kryo-related functionality is exercised.

What do you think? Is this a good way forward?

This eliminates the following warning from the Maven CLI tool:

  [WARNING] The project org.openmicroscopy:ome-common:jar:6.0.3-SNAPSHOT
  uses prerequisites which is only intended for maven-plugin projects
  but not for non maven-plugin projects. For such purposes you should
  use the maven-enforcer-plugin. See
  https://maven.apache.org/enforcer/enforcer-rules/requireMavenVersion.html
@ctrueden
Copy link
Member Author

ctrueden commented May 8, 2020

Hmm, the CI failures don't appear related to this change. If there is anything I can do to help, let me know.

ctrueden added a commit to scijava/pom-scijava that referenced this pull request May 8, 2020
ctrueden added a commit to scijava/pom-scijava that referenced this pull request May 8, 2020
ctrueden added a commit to scijava/pom-scijava that referenced this pull request May 8, 2020
ctrueden added a commit to scijava/pom-scijava that referenced this pull request May 8, 2020
@sbesson sbesson mentioned this pull request May 9, 2020
@sbesson sbesson closed this May 11, 2020
@sbesson sbesson reopened this May 11, 2020
@sbesson
Copy link
Member

sbesson commented May 11, 2020

@ctrueden thanks for opening this discussion. The Travis issue you reported above was indeed independent and got fixed separately.

We briefly discussed this contribution and how to review it with @ome/formats and @ome/omero. The kryo dependency is crucial at the level of OMERO where initialized Bio-Formats readers are serialized on disk. Due to the deepness of the stack, testing dependency upgrades have revealed to be quite involved in the past.

Immediately, this PR will be included into our next daily CI builds so that we can have a first assessment of its impact across all Bio-Formats readers as well as in a deployed OMERO server.

@ctrueden
Copy link
Member Author

ctrueden commented May 11, 2020

Thanks for pursuing it, @sbesson. I appreciate the importance of testing changes like this carefully. For the time being, I updated pom-scijava to ignore overlap in the com.esotericsoftware classes. This merely hides the problem, not solving it—but at least it enables people to ostensibly combine dependencies relying on both old and new kryo at the same time.

@sbesson
Copy link
Member

sbesson commented May 11, 2020

Unfortunately, while testing a combined build of all Bio-Formats components, I saw that the old 2.24.0 version was still pulled. This is because the kryo dependency is independently declared in Bio-Formats

https://github.com/ome/bioformats/blob/ed992ebc2cb07d04fcb49485540af0babd1baa05/pom.xml#L52
https://github.com/ome/bioformats/blob/ed992ebc2cb07d04fcb49485540af0babd1baa05/components/formats-bsd/pom.xml#L71-L75

and as per the Maven dependency resolution mechanism, it will override the version defined in ome-common.

@ctrueden
Copy link
Member Author

ctrueden commented May 11, 2020

@sbesson I filed ome/bioformats#3557 updating kryo to 4.0.2 there as well.

as per the Maven dependency resolution mechanism, it will override the version defined in ome-common.

No, it won't, because the groupId changed. So you'll get both versions of kryo on the classpath! The solution is to stamp out usage of the old kryo across the entire OME software stack.

ctrueden added a commit to scijava/pom-scijava that referenced this pull request May 11, 2020
ctrueden added a commit to scijava/pom-scijava that referenced this pull request May 11, 2020
ctrueden added a commit to scijava/pom-scijava that referenced this pull request May 23, 2020
@sbesson sbesson merged commit 1ef1a53 into ome:master Jul 24, 2020
@ctrueden ctrueden deleted the update-kryo branch July 24, 2020 20:00
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants