Skip to content

Bye bye commons-io#14

Open
chris-allan wants to merge 3 commits intoome:masterfrom
chris-allan:bye-bye-commons-io
Open

Bye bye commons-io#14
chris-allan wants to merge 3 commits intoome:masterfrom
chris-allan:bye-bye-commons-io

Conversation

@chris-allan
Copy link
Copy Markdown
Member

Remove use of commons-io. Most critical code paths were able to be replaced with Java 8 nio Paths or Files usage. Where that was not possible equivalent Guava functions were used.

@mtbc @rgozim @jburel @sbesson @joshmoore

if (writerFile != null) {
try {
FileUtils.moveFile(writerFile, readerFile);
Files.move(writerFile.toPath(), readerFile.toPath());
Copy link
Copy Markdown
Member

@rgozim rgozim Jun 6, 2019

Choose a reason for hiding this comment

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

Just a caveat, the docs for nio.files state The move fails if the target file exists, unless the REPLACE_EXISTING option is specified. Might need to check that is expected behaviour .

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.

Yep, I checked that before I made the change. That specific behaviour is identical to that of FileUtils.moveFile():

There are likely other behavioral differences but I don't think they're particularly relevant.

@jburel
Copy link
Copy Markdown
Member

jburel commented Jun 6, 2019

Excluding, other repositories e.g. omero-renderer will require some TLC.
See https://merge-ci.openmicroscopy.org/jenkins/job/OMERO-build-build/72/
We can review in a point release

}

configurations.all {
exclude group: 'commons-io'
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.

Running gradle dependencies shows quite a lot of inclusions of commons-io. Would excluding it entirely not trigger an error when a dependency attempts to use any commons-io APIs?

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.

that's what is currently happening omero-renderer for example uses commons-io cf. build link in the previous comment

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.

Yes it would, however that's highly situational. Let's take one commons-io example from the dependency tree of this library:

...
|         +--- org.hibernate:hibernate-core:3.6.10.Final
|         |    +--- antlr:antlr:2.7.6
|         |    +--- commons-collections:commons-collections:3.1
|         |    +--- dom4j:dom4j:1.6.1
|         |    +--- org.hibernate:hibernate-commons-annotations:3.2.0.Final
|         |    |    \--- org.slf4j:slf4j-api:1.5.8 -> 1.7.22
|         |    +--- org.hibernate.javax.persistence:hibernate-jpa-2.0-api:1.0.1.Final
|         |    +--- javax.transaction:jta:1.1
|         |    \--- org.slf4j:slf4j-api:1.6.1 -> 1.7.22
...

This is brought in because omero-romio depends on omero-common which by extension depends on omero-model. Hibernate annotations are used there so nearly all the Hibernate JARs are brought in. However, omero-romio doesn't actually use any of these classes itself.

It's hard to build things in such a way where dependencies do not unexpectedly leak. Serious software engineering care and attention has to be paid to interface, concrete class, and dependency construction to avoid it and we have not historically spent a lot of time doing so.

Are you talking about omero-server, @jburel? There are no omero-renderer build failure references that I can see in the aforementioned build 72 failure. Rather the issues seem to be in omero-server, no?

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 sorry I mean omero-server.

@joshmoore
Copy link
Copy Markdown
Member

@jburel : I propose to remove your --exclude comment from #14 (comment) Thoughts?

@jburel
Copy link
Copy Markdown
Member

jburel commented Dec 19, 2019

removed --exclude

@jburel
Copy link
Copy Markdown
Member

jburel commented Dec 19, 2019

Looking a bit more carefully at the usage of commons-io in omero-server. We will have to review the way scripts/lut are handled.
Without changes in omero-server we will need to add the dependency at that level
I don't think we are ready to remove it yet

@jburel
Copy link
Copy Markdown
Member

jburel commented Apr 4, 2024

removing exclude to test it again

Some changes required in omero-blitz
--exclude

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.

4 participants