-
Notifications
You must be signed in to change notification settings - Fork 11
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
Dependency updates #38
Conversation
Building this PR in ome/openmicroscopy#6324 (I used my branch since this branch is not built on top of main and the version is old).
|
Have you established that removing/commenting https://github.com/ome/openmicroscopy/blob/f73008bc34d37f6034a21095c55eb2a8b6075e44/ivy.xml#L21 suffices to fix the build issues? i.e. is this component the only source of issues? |
I have rebuilt using only ome/openmicroscopy#6323 and excluding the ZarrReader and it builds fine |
Heres the dependency tree with this PR, other than logback the rest of these are related to jzarr:
|
Using
So most of the dependencies which fail to resolve in the OMERO build system are defined at the low-level It's still not fully clear to me why this is an issue with this repository but not with As things stand, we have evidence that although our coupled CI workflow is sufficient to include the development ZarrReader into a development OMERO.server, this causes serious issue when trying to build OMERO in isolation. In addition to the impact on a developer workfow, a more critical impact is that release builds are likely to fail for similar reasons that caused ZarrReader to be excluded in the past. So solving this is a blocker to the inclusion of this reader into an OMERO.server release. Trying to come up with a list of options to explore/discuss:
|
using this PR in openmicroscopy build fails with
https://github.com/ome/openmicroscopy/runs/7022030936?check_suite_focus=true |
This time
If I remember correctly a bump to testng should fix that one |
https://github.com/ome/openmicroscopy/runs/7023130718?check_suite_focus=true is now green with the last commit |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
One question about overriding vs exclusing the objenesis
dependdency.
Otherwise, this seems to fix the downstream build as per #38 (comment) and potentiall unlock the release. Should we try to get this merged and released (proposing 0.3.0
as per the S3 disabling) and try to consume the released version in the openmicroscopy
build?
pom.xml
Outdated
<dependency> | ||
<groupId>org.objenesis</groupId> | ||
<artifactId>objenesis</artifactId> | ||
<version>2.5.1</version> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is the version shipped in ome-common
(via kryo
4.0.2). From mvn dependency:tree -Dverbose
it looks like it is conflicting with the version from org.mockito:mockito-inline:3.7.7
.
Declaring it in the POM should suffice although I assume an alternative would be to add and exclude
in the mockito
dependency?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
From a quick discussion with @dgault and @jburel, a possible simpler and more effective option would simply be to declare mockito
and testng
as test
dependencies (via scope
). This might suffice to exclude these from the openmicroscopy Ivy resolution and prevent the copying of unnecessary JARs into the lib
folder
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Barring the removal of the S3 functionality (which will likely need to be reviewed at a later date), the proposed change fix some dependency mismatch in the dependency declaration, classify test-only dependency and remove the usage of commons-io
.
I did some minimal testing in our nightly CI server importing a multi-file single image, a WSI and a plate and everything worked as expected.
Under these assumptions, proposing to get this merged, tag the component as 0.3.0
and then bump the version in openmicroscopy
in preparation of the release if no objection
Following on from conversation with @sbesson and @jburel, trying to remove some of the dependencies. Goal of this PR will be to update the below areas:
1 - formats API bumped to latest 6.10.0, this should be straight forward
2 - commons-io removed, only area impacted will be the list of used files. PR aims to keep the reproduce the same list
3 - initially remove netty-nio-client dependency, this looks safe and straight forward
As a follow up I will continue trying to strip out and remove the remaining amazon dependencies which essentially means removing the s3 functionality for the time being.