Skip to content

Upgrade commons-io from 1.x to 2.6#135

Merged
jburel merged 1 commit into
ome:masterfrom
sbesson:commons-io_2.6
Dec 7, 2021
Merged

Upgrade commons-io from 1.x to 2.6#135
jburel merged 1 commit into
ome:masterfrom
sbesson:commons-io_2.6

Conversation

@sbesson

@sbesson sbesson commented Nov 22, 2021

Copy link
Copy Markdown
Member

Unlike the rest of the OMERO Java components which declare commons-io:commons-io:2.6 as a dependency in build.gradle, this component still inherits the 1.x version of commons-io transitively (via Bio-Formats and its jhdf5 dependency).
As we are trying to upgrade the underlying jhdf5 library, the ScriptRepoHelper class needs to be updated to be compatible with commons-io 2.x

See also ome/bioformats#3745

Unlike the rest of the OMERO Java components which declare
commons-io:commons-io:2.6 as a dependency in build.gradle, this component
still inherits the 1.x version of commons-io transitively (via Bio-Formats and
its jhdf5 dependency).
As we are trying to upgrade the underlying jhdf5 library, the ScriptRepoHelper
class needs to be updated to be compatible with commons-io 2.x
@joshmoore

Copy link
Copy Markdown
Member

Assuming everything stays green, MSTM.

@sbesson

sbesson commented Nov 22, 2021

Copy link
Copy Markdown
Member Author

Retriggered the OMERO suite with this PR included and all tests are still passing which is a good sign - see https://merge-ci.openmicroscopy.org/jenkins/job/OMERO-test-integration/954/

@jburel

jburel commented Nov 23, 2021

Copy link
Copy Markdown
Member

There was an effort to remove commons-io see ome/omero-romio#14

@joshmoore

joshmoore commented Nov 23, 2021

Copy link
Copy Markdown
Member

@jburel: and you're suggesting re-activating that? or to try to replace the usage here with something else?

@jburel

jburel commented Nov 23, 2021

Copy link
Copy Markdown
Member

The concern at the time, was with script/lut (ScriptRepoHelper) See ome/omero-romio#14 (comment). So I think we should reactivate and maybe use Guava equivalent instead of injecting commons-io.

*/
public final static IOFileFilter BASE_SCRIPT_FILTER = new AndFileFilter(Arrays
.asList(new FileFilter[] { EmptyFileFilter.NOT_EMPTY,
.asList(new IOFileFilter[] { EmptyFileFilter.NOT_EMPTY,

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

The fact that all of this is in the public API is less than ideal for dropping the library.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

I assume that's a response to #135 (comment) rather than a review of these changes :)

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Yes. I just wanted to make the comment closer to the actual code.

@sbesson sbesson changed the title Update ScriptRepoHelper to use commons-io:2.6 Upgrade commons-io from 1.x to 2.6 Nov 23, 2021
@sbesson

sbesson commented Nov 23, 2021

Copy link
Copy Markdown
Member Author

So I think we should reactivate and maybe use Guava equivalent instead of injecting commons-io

Slightly pedanticly, I want to point there is no new injection of commons-io. commons-io 1.x was effectively a dependency used in the public API but it was declared transitively. This PR serves two purposes: at the OMERO level, declare the dependency explicitly and upgrade it to 2.x to match the other components and at the Bio-Formats level, unblock the upgrade of jhdf5. I have changed the PR title to be explicit about the scope.

I have no strong opinion regarding using this library or guava. Obviously, implementing the switch would need to respect both the breaking concerns expressed in #135 (comment) as well as the timeline for Bio-Formats 6.8.0.

@jburel

jburel commented Nov 23, 2021

Copy link
Copy Markdown
Member

injecting was not the correct term.
One option could be to remove commons-io from omero-romio since the work is done (obviously no urgency since the PR has been opened for a while)
and update the dependency in this PR

@sbesson

sbesson commented Dec 6, 2021

Copy link
Copy Markdown
Member Author

With the inclusion of ome/bioformats#3745 for the upcoming Bio-Formats release, the latest development stack started failing due to the 1.x/2.x incompatibility - see https://latest-ci.openmicroscopy.org/jenkins/job/OMERO-build-build/896/.

@jburel based on #135 (comment), I assume you have no objections seeing this merged to fix outstanding build issues? Or do you have an alternate mitigation plan in mind?

@sbesson sbesson requested a review from jburel December 6, 2021 08:26
@jburel

jburel commented Dec 7, 2021

Copy link
Copy Markdown
Member

Let's merge it. It is obviously not an urgency since the PR has been opened for a while

@jburel jburel merged commit 33fc60b into ome:master Dec 7, 2021
@sbesson sbesson deleted the commons-io_2.6 branch December 7, 2021 09:55
@sbesson

sbesson commented Dec 7, 2021

Copy link
Copy Markdown
Member Author

Thanks J-M

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.

3 participants