Upgrade to Java 7 #5

plamentotev · 2017-04-30T17:03:25Z

Hi,

As discussed on codehaus-plexus/plexus-archiver#55, Plexus Archiver is going to require Java 7.
I'm using the occasion to purpose an update for Plexus IO as well.

The proposed changes are:

Update the required Java version to 7
Use the new language features introduced in Java 7 ( diamond operators (<>) and try with resources)
And most importantly - Plexus IO uses the ls command to get the file attributes when running on Java versions prior to 7. Remove all legacy code and use only the Java 7 implementation.

Unfortunately the proposed changes are not backward compatible. Instead of having FileAttributes, Java7FileAttributes and Java7AttributeUtils, now we have FileAttributes and AttributeUtils. Java7Reflector, AttributeParserclasses and the public PlexusIoResourceAttributeUtils#getFileAttributesByPath( File dir, boolean recursive, boolean includeNumericUserId ) method are deleted. What do you think? We could remove the legacy code without changing the public interfaces and classes, but I think it's better to just remove the legacy code and interfaces - it will make the maintenance easier in long run.

p.s. If we make backward incompatible changes I guess we need to bump the project version.

michael-o · 2017-04-30T20:35:51Z

I just created milestone 3.0. I think it is best to bump version to 3.0.0 and get rid of all stuff no matter what. This is fine for a major release.

michael-o · 2017-04-30T20:37:26Z

src/test/java/org/codehaus/plexus/components/io/filemappers/ResourcesTest.java

@@ -114,8 +114,7 @@ private void createZipFile( File dest, File dir ) throws IOException
    private void compare( InputStream in, File file )
        throws IOException
    {
-        InputStream fIn = new FileInputStream( file );
-        try
+        try(InputStream fIn = new FileInputStream( file ))


Missing whitespace.

michael-o · 2017-04-30T20:41:39Z

src/main/java/org/codehaus/plexus/components/io/attributes/FileAttributes.java

+        sb.append("\nuid: ");
+        sb.append(hasUserId() ? Integer.toString(userId) : "");
+        sb.append("\ngid: ");
+        sb.append(hasGroupId() ? Integer.toString(groupId) : "");



Please use System.lineSeparator() instead of \n.

michael-o · 2017-04-30T20:44:26Z

src/test/java/org/codehaus/plexus/components/io/attributes/FileAttributesTest.java

-        verifyLsModeSet( "-r--------", new boolean[]{ true, false, false, false, false, false, false, false, false } );
-        verifyLsModeSet( "--w-------", new boolean[]{ false, true, false, false, false, false, false, false, false } );
-    }
+        if ( Os.isFamily( Os.FAMILY_WINDOWS ) || Os.isFamily( Os.FAMILY_WIN9X ) )


I think you can safely drop WIN9X.

michael-o · 2017-04-30T20:45:36Z

...st/java/org/codehaus/plexus/components/io/attributes/PlexusIoResourceAttributeUtilsTest.java

@@ -94,7 +67,7 @@ public void testGetAttributesForThisTestClass()
        assertEquals( System.getProperty( "user.name" ), fileAttrs.getUserName() );
    }

-    public void testFolderJava7()
+    public void testFolder()
            throws IOException, CommandLineException {

        if (Os.isFamily( Os.FAMILY_WINDOWS ) || Os.isFamily( Os.FAMILY_WIN9X )){


Same here probably throughout.

michael-o · 2017-04-30T20:46:11Z

...st/java/org/codehaus/plexus/components/io/attributes/PlexusIoResourceAttributeUtilsTest.java

@@ -200,119 +130,18 @@ public void testSrcResource()

        assertTrue(pr.getOctalMode() > 0);
    }
-    public void testPermissionDenied()
+    public void testNonExistingFolder()


Unix does not know folders, but only directories.

Sorry, I'm not quite sure I follow you. Do you mean that in Unix there are 'directories' not 'folders' and that's why the method name should be'testNonExistingDirectory? I choose this name because it's consistent with the rest of the existing test names (such as testFolderJava7).

Correct, update accordingly since you already doing update.

michael-o · 2017-04-30T20:55:15Z

Completed the review. Looks good to me. Please review the codestyle. In most cases whitespace is missing before and after parens.

plamentotev · 2017-05-02T18:20:17Z

src/main/java/org/codehaus/plexus/components/io/attributes/PlexusIoResourceAttributeUtils.java

@@ -183,141 +174,11 @@ public static PlexusIoResourceAttributes getFileAttributes( File file )
        return o;
    }

-    public static Map<String, PlexusIoResourceAttributes> getFileAttributesByPath( File dir )


Actually I've deleted this one by mistake - it does make sense and it's used by Plexus Archiver. I'll restore it.

michael-o · 2017-04-30T20:51:50Z

README.md

-It is therefore recommended to run mvn -Djava.language.downgrade=true clean install before relasing, to assess
- that features still work with older jdk versions.
-
-
 Version 2.6


This file is incomplete anyway, drop it altogether.

michael-o · 2017-05-03T17:02:21Z

Waiting for your updated PR. Tell me when you are done.

Now the changelog is kept in the Plexus Archiver project and the Java version bit is also going to to outdated after the update to Java 7.

plamentotev · 2017-05-03T18:58:42Z

I have updated the PR.

About the code style - there seems to be quite a lot violations. Maybe I should fix them in separate commit. Have to figure out how to do it though. The InteliJ Idea and Eclipse auto-formatting is long way from perfect.

michael-o · 2017-05-03T19:00:56Z

Agreed. A separate commit makes sense. Let me review the new changes....

michael-o · 2017-05-03T19:06:33Z

src/test/java/org/codehaus/plexus/components/io/attributes/FileAttributesTest.java

 /*
- * Copyright 2016 Plexus developers.
+ * Copyright 2011 The Codehaus Foundation.


This copyright change is weird...

Actually FileAttributesTest is not modified at all. It's deleted and Java7FileAttributesTest is renamed to FileAttributesTest (the same goes for FileAttributes and Java7FileAttributes). Git shows those as diffs but they are actually delete + rename. Maybe I should state that explicitly in the commit message as it's not obvious from looking at the diffs.

Good idea. Please go ahead.

ok. I've modified the commit message.

The classes related to file attributes are depending on features introduced in Java 7. To support Java version prior to 7, plexus IO uses reflection check to choose between two different implementations. The legacy one that parses the output from the `ls` command and the Java 7 one that is pure Java. Because now Java 7 is the minimum required version, remove all the reflection Java version checks and remove any legacy code that executes only for Java versions below 7. There were two version of `FileAttributes` - `FileAttributes` and `Java7FileAttributes`. Remove the legacy `FileAttributes` and rename `Java7FileAttributes` to `FileAttributes`. Rename `Java7AttributeUtils` to `AttributeUtils` as there is no longer Java version specific implementations for attributes related classes. NOTE: The commit diff shows `FileAttributes` as changed but it's actually deleted and `Java7FileAttributes` is renamed to `FileAttributes`. The same goes for `FileAttributesTest` and `Java7FileAttributesTest` Also remove `PlexusIoResourceAttributeUtils#getFileAttributesByPath( File dir, boolean recursive, boolean includeNumericUserId )`. `includeNumericUserId` is related to the `ls` command and is no longer needed. Update the tests. Delete no longer needed resources and tests. All tests and resources related to the `ls` output parsing are no longer needed. Also replace `PlexusIoResourceAttributeUtilsTest#testPermissionDenied` with `testNonExistingFolder` - when folder is missing we no longer get `cannot access` response from the `ls` command and the new test method name is more suitable.

We could quite safely assume that the tests are not going to be executed on Windows 9x.

Unix uses the term 'Directory' and the test runs only on Unix-like OSes. Change the test name accordingly.

plamentotev · 2017-05-04T20:13:14Z

I've fixed the code style. Or at least I've tried 😄 .

michael-o · 2017-05-04T20:31:49Z

You did very well...
I will go on and create a branch for it

michael-o · 2017-05-04T21:05:06Z

FYI: http://www.mail-archive.com/[email protected]/msg113665.html

plamentotev requested review from krosenvold and michael-o April 30, 2017 17:03

michael-o self-assigned this Apr 30, 2017

michael-o added the enhancement label Apr 30, 2017

michael-o added this to the 3.0 milestone Apr 30, 2017

michael-o reviewed Apr 30, 2017

View reviewed changes

plamentotev commented May 2, 2017

View reviewed changes

michael-o requested changes May 2, 2017

View reviewed changes

michael-o approved these changes May 3, 2017

View reviewed changes

plamentotev added 3 commits May 3, 2017 21:17

Bump version for the upcoming breaking changes

c39ff14

Delete the outdated README file

f49d200

Now the changelog is kept in the Plexus Archiver project and the Java version bit is also going to to outdated after the update to Java 7.

Upgrade the minimum required Java version to Java 7

2bbfb10

plamentotev force-pushed the upgrade-to-java-7 branch from fc7cd6c to eb7996f Compare May 3, 2017 18:53

michael-o reviewed May 3, 2017

View reviewed changes

plamentotev added 5 commits May 4, 2017 22:59

Refactor the code to use diamond operators (<>) and try with resources

3c7cc25

Use the system line separator instead of \n

c5faa41

Drop the check for Windows 9x

aed65df

We could quite safely assume that the tests are not going to be executed on Windows 9x.

Code cleanup - rename test from testFolder to testDirectory

88cda20

Unix uses the term 'Directory' and the test runs only on Unix-like OSes. Change the test name accordingly.

plamentotev force-pushed the upgrade-to-java-7 branch from eb7996f to 9ccf45d Compare May 4, 2017 20:02

Reformat to a common code style

55773f5

plamentotev force-pushed the upgrade-to-java-7 branch from 9ccf45d to 55773f5 Compare May 4, 2017 20:10

michael-o closed this in b07738a May 9, 2017

plamentotev deleted the upgrade-to-java-7 branch May 9, 2017 16:47

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Upgrade to Java 7 #5

Upgrade to Java 7 #5

plamentotev commented Apr 30, 2017

michael-o commented Apr 30, 2017

michael-o Apr 30, 2017

michael-o Apr 30, 2017

michael-o Apr 30, 2017

michael-o Apr 30, 2017

michael-o Apr 30, 2017

plamentotev May 2, 2017

michael-o May 2, 2017

michael-o commented Apr 30, 2017

plamentotev May 2, 2017

michael-o Apr 30, 2017

michael-o commented May 3, 2017

plamentotev commented May 3, 2017

michael-o commented May 3, 2017

michael-o May 3, 2017

plamentotev May 3, 2017 •

edited

Loading

michael-o May 3, 2017

plamentotev May 4, 2017

plamentotev commented May 4, 2017

michael-o commented May 4, 2017

michael-o commented May 4, 2017

Upgrade to Java 7 #5

Upgrade to Java 7 #5

Conversation

plamentotev commented Apr 30, 2017

michael-o commented Apr 30, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

michael-o commented Apr 30, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

michael-o commented May 3, 2017

plamentotev commented May 3, 2017

michael-o commented May 3, 2017

Choose a reason for hiding this comment

plamentotev May 3, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

plamentotev commented May 4, 2017

michael-o commented May 4, 2017

michael-o commented May 4, 2017

plamentotev May 3, 2017 •

edited

Loading