Skip to content

Bye bye commons-io #14

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.

Sign up for GitHub

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

Open
chris-allan wants to merge 3 commits into
base: master
Choose a base branch
from
Open

Bye bye commons-io #14

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
947dc7d
Remove and exclude commons-io from dependency tree
chris-allan Jun 6, 2019
cd87270
Remove use of FilenameUtils
chris-allan Jun 6, 2019
5d922e8
Remove use of FileUtils
chris-allan Jun 6, 2019
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 4 additions & 1 deletion build.gradle
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,10 @@ repositories {
jcenter()
}

configurations.all {
exclude group: 'commons-io'
Copy link
Copy Markdown
Member

@rgozim rgozim Jun 7, 2019

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

@jburel jburel Jun 7, 2019

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

@chris-allan chris-allan Jun 7, 2019

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

@jburel jburel Jun 7, 2019

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.

}

java {
sourceCompatibility = JavaVersion.VERSION_1_8
targetCompatibility = JavaVersion.VERSION_1_8
Expand All @@ -23,7 +27,6 @@ dependencies {
api("org.openmicroscopy:omero-common:5.5.0")

// Keep from being exposed to child projects
implementation("commons-io:commons-io:2.6")
implementation("commons-lang:commons-lang:2.6")
}

Expand Down
4 changes: 2 additions & 2 deletions src/main/java/ome/io/bioformats/BfPyramidPixelBuffer.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,7 @@
import java.nio.ByteOrder;
import java.nio.channels.FileLock;
import java.nio.channels.OverlappingFileLockException;
import java.nio.file.Files;
import java.util.List;

import loci.formats.FormatException;
Expand All @@ -37,7 +38,6 @@
import ome.xml.model.enums.EnumerationException;
import ome.xml.model.primitives.PositiveInteger;

import org.apache.commons.io.FileUtils;
import org.slf4j.Logger;
import org.slf4j.LoggerFactory;

Expand Down Expand Up @@ -440,7 +440,7 @@ protected void closeWriter() throws IOException
try {
if (writerFile != null) {
try {
FileUtils.moveFile(writerFile, readerFile);
Files.move(writerFile.toPath(), readerFile.toPath());
Copy link
Copy Markdown
Member

@rgozim rgozim Jun 6, 2019 •
edited
Loading

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

@chris-allan chris-allan Jun 6, 2019

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.

} finally {
writerFile = null;
}
Expand Down
6 changes: 3 additions & 3 deletions src/main/java/ome/io/nio/AbstractFileSystemService.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,9 +6,9 @@
package ome.io.nio;

import java.io.File;
import java.nio.file.Paths;
import java.util.Formatter;

import org.apache.commons.io.FilenameUtils;
import org.slf4j.Logger;
import org.slf4j.LoggerFactory;

Expand Down Expand Up @@ -79,7 +79,7 @@ protected void createSubpath(String path) {
* @return the path relative to the root
*/
public String getPixelsDirectory() {
return FilenameUtils.concat(root, PIXELS_PATH);
return Paths.get(root, PIXELS_PATH).toString();
}

/**
Expand Down Expand Up @@ -138,6 +138,6 @@ private String getPath(String prefix, Long id) {
}
}
}
return FilenameUtils.concat(root, prefix + suffix + id);
return Paths.get(root, prefix + suffix + id).toString();
}
}
5 changes: 3 additions & 2 deletions src/main/java/ome/io/nio/PixelsService.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -40,14 +40,15 @@
import ome.model.stats.StatsInfo;
import ome.util.PixelData;

import org.apache.commons.io.FileUtils;
import org.apache.commons.lang.StringUtils;
import org.slf4j.Logger;
import org.slf4j.LoggerFactory;
import org.springframework.beans.FatalBeanException;
import org.springframework.context.ApplicationEventPublisher;
import org.springframework.context.ApplicationEventPublisherAware;

import com.google.common.io.Files;

/**
* @author <br>
* Chris Allan&nbsp;&nbsp;&nbsp;&nbsp; <a
Expand Down Expand Up @@ -482,7 +483,7 @@ public void run(int z, int c, int t, int x, int y, int w,
try
{
pixelsPyramidFile.delete();
FileUtils.touch(pixelsPyramidFile); // ticket:5189
Files.touch(pixelsPyramidFile); // ticket:5189
}
catch (Exception e2)
{
Expand Down
6 changes: 3 additions & 3 deletions src/test/java/ome/io/nio/utests/AbstractPyramidPixelBufferUnitTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,9 +6,9 @@
*/
package ome.io.nio.utests;

import java.io.File;
import java.io.IOException;
import java.nio.ByteBuffer;
import java.nio.file.Paths;
import java.util.List;

import loci.formats.FormatTools;
Expand All @@ -24,7 +24,7 @@
import ome.util.checksum.ChecksumProviderFactoryImpl;
import ome.util.checksum.ChecksumType;

import org.apache.commons.io.FileUtils;
import com.google.common.io.MoreFiles;

/**
* Tests the logic for creating {@link BfPyramidPixelBuffer} instances.
Expand Down Expand Up @@ -83,7 +83,7 @@ public boolean requiresPixelsPyramid(Pixels pixels) {
}

protected void deleteRoot() throws IOException {
FileUtils.deleteDirectory(new File(root));
MoreFiles.deleteRecursively(Paths.get(root));
}

protected short writeTiles(final List<String> hashDigests) throws FailedTileLoopException {
Expand Down
7 changes: 4 additions & 3 deletions src/test/java/ome/io/nio/utests/BfPixelBufferUnitTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,21 +5,22 @@
package ome.io.nio.utests;


import java.io.File;
import java.io.IOException;
import java.nio.file.Paths;

import ome.io.nio.PixelBuffer;
import ome.io.nio.PixelsService;
import ome.io.nio.RomioPixelBuffer;
import ome.model.core.Pixels;
import ome.model.enums.PixelsType;

import org.apache.commons.io.FileUtils;
import org.testng.Assert;
import org.testng.annotations.AfterClass;
import org.testng.annotations.BeforeClass;
import org.testng.annotations.Test;

import com.google.common.io.MoreFiles;

/**
* Tests the logic for creating {@link BfPixelBuffer} instances.
* @since 4.3
Expand Down Expand Up @@ -64,7 +65,7 @@ private void setup() {

@AfterClass
public void tearDown() throws IOException {
FileUtils.deleteDirectory(new File(root));
MoreFiles.deleteRecursively(Paths.get(root));
}

@Test
Expand Down
7 changes: 5 additions & 2 deletions src/test/java/ome/io/nio/utests/HelperUnitTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,11 +6,14 @@

import java.io.File;
import java.io.IOException;
import java.nio.file.Paths;

import org.apache.commons.io.FileUtils;
import org.testng.Assert;
import org.testng.annotations.AfterClass;
import org.testng.annotations.Test;

import com.google.common.io.MoreFiles;

import ome.io.nio.PixelsService;

public class HelperUnitTest {
Expand All @@ -28,7 +31,7 @@ private String p(String path) {

@AfterClass
public void tearDown() throws IOException {
FileUtils.deleteDirectory(new File(ROOT));
MoreFiles.deleteRecursively(Paths.get(ROOT));
}

//
Expand Down
8 changes: 4 additions & 4 deletions src/test/java/ome/io/nio/utests/HugePixelBufferUnitTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,16 +4,16 @@
*/
package ome.io.nio.utests;

import java.io.File;
import java.io.IOException;
import java.nio.file.Paths;


import org.apache.commons.io.FileUtils;
import org.testng.Assert;
import org.testng.annotations.AfterClass;
import org.testng.annotations.BeforeMethod;
import org.testng.annotations.Test;

import com.google.common.io.MoreFiles;

import ome.io.nio.DimensionsOutOfBoundsException;
import ome.io.nio.PixelBuffer;
import ome.io.nio.PixelsService;
Expand All @@ -36,7 +36,7 @@ public class HugePixelBufferUnitTest {

@AfterClass
public void tearDown() throws IOException {
FileUtils.deleteDirectory(new File(ROOT));
MoreFiles.deleteRecursively(Paths.get(ROOT));
}

@BeforeMethod
Expand Down
7 changes: 4 additions & 3 deletions src/test/java/ome/io/nio/utests/LargePixelBufferUnitTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,15 +5,16 @@
package ome.io.nio.utests;


import java.io.File;
import java.io.IOException;
import java.nio.file.Paths;

import org.apache.commons.io.FileUtils;
import org.testng.Assert;
import org.testng.annotations.AfterClass;
import org.testng.annotations.BeforeMethod;
import org.testng.annotations.Test;

import com.google.common.io.MoreFiles;

import ome.io.nio.DimensionsOutOfBoundsException;
import ome.io.nio.PixelBuffer;
import ome.io.nio.PixelsService;
Expand All @@ -36,7 +37,7 @@ public class LargePixelBufferUnitTest {

@AfterClass
public void tearDown() throws IOException {
FileUtils.deleteDirectory(new File(ROOT));
MoreFiles.deleteRecursively(Paths.get(ROOT));
}

@BeforeMethod
Expand Down
7 changes: 4 additions & 3 deletions src/test/java/ome/io/nio/utests/NormalPixelBufferUnitTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,15 +5,16 @@
package ome.io.nio.utests;


import java.io.File;
import java.io.IOException;
import java.nio.file.Paths;

import org.apache.commons.io.FileUtils;
import org.testng.Assert;
import org.testng.annotations.AfterClass;
import org.testng.annotations.BeforeMethod;
import org.testng.annotations.Test;

import com.google.common.io.MoreFiles;

import ome.io.nio.DimensionsOutOfBoundsException;
import ome.io.nio.PixelBuffer;
import ome.io.nio.PixelsService;
Expand All @@ -36,7 +37,7 @@ public class NormalPixelBufferUnitTest {

@AfterClass
public void tearDown() throws IOException {
FileUtils.deleteDirectory(new File(ROOT));
MoreFiles.deleteRecursively(Paths.get(ROOT));
}

@BeforeMethod
Expand Down
7 changes: 4 additions & 3 deletions src/test/java/ome/io/nio/utests/OutOfBoundsUnitTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,21 +5,22 @@
package ome.io.nio.utests;


import java.io.File;
import java.io.IOException;
import java.nio.file.Paths;

import ome.io.nio.DimensionsOutOfBoundsException;
import ome.io.nio.PixelBuffer;
import ome.io.nio.PixelsService;
import ome.model.core.Pixels;
import ome.model.enums.PixelsType;

import org.apache.commons.io.FileUtils;
import org.testng.Assert;
import org.testng.annotations.AfterClass;
import org.testng.annotations.BeforeMethod;
import org.testng.annotations.Test;

import com.google.common.io.MoreFiles;

/**
* @author callan
*
Expand All @@ -34,7 +35,7 @@ public class OutOfBoundsUnitTest {

@AfterClass
public void tearDown() throws IOException {
FileUtils.deleteDirectory(new File(ROOT));
MoreFiles.deleteRecursively(Paths.get(ROOT));
}

@BeforeMethod
Expand Down
7 changes: 4 additions & 3 deletions src/test/java/ome/io/nio/utests/PixelServiceCreatesDirectoryUnitTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,20 +6,21 @@



import java.io.File;
import java.io.IOException;
import java.nio.file.Paths;

import ome.io.nio.PixelBuffer;
import ome.io.nio.PixelsService;
import ome.model.core.Pixels;
import ome.model.enums.PixelsType;

import org.apache.commons.io.FileUtils;
import org.testng.Assert;
import org.testng.annotations.AfterClass;
import org.testng.annotations.BeforeMethod;
import org.testng.annotations.Test;

import com.google.common.io.MoreFiles;

public class PixelServiceCreatesDirectoryUnitTest {
private Pixels pixels;

Expand All @@ -30,7 +31,7 @@ public class PixelServiceCreatesDirectoryUnitTest {

@AfterClass
public void tearDown() throws IOException {
FileUtils.deleteDirectory(new File(ROOT));
MoreFiles.deleteRecursively(Paths.get(ROOT));
}

@BeforeMethod
Expand Down