[VOTE] Release Commons Compress 1.6

classic Classic list List threaded Threaded
31 messages Options
12
Reply | Threaded
Open this post in threaded view
|

Re: [VOTE] Release Commons Compress 1.6

Stefan Bodewig
On 2013-10-13, Oliver Heger wrote:

> The artifacts look good, the build works fine with Java 1.7 on Windows
> 7. I tried to build with Java 1.5, but got:

> Tests in error:

> SevenZTestCase.testSevenZArchiveCreationUsingLZMA2:37->testSevenZArchiveCreati
> on:59 ╗ OutOfMemory
>   XZTestCase.testXZCreation:44 ╗ OutOfMemory Java heap space

> Tests run: 374, Failures: 0, Errors: 2, Skipped: 0

> ISTR that I had similar problems with other [compress] releases, so I
> don't consider this as blocking.

I don't recall it, but XZ is pretty heavy on GC pressure.  Given that
our XZ implementation is a very thin layer on top of XZ for Java there
wouldn't be anything we could do - apart from increasing the heap size
for the tests - anyway.

> The title in the release notes says "Apache Commons Compress 1.5 RELEASE
> NOTES"; it refers to the old version. I think, this requires another RC.

Ouch.

> The RC was built using Java 1.6.0_27. Does this version already contain
> the Javadoc security fix?

The javadoc plugin used by the Commons Parent took care of it.

> When I build the site locally, I get a different clirr report showing 9
> errors because some public methods have been made final.

Strange.  Why would Clirr create different reports?  We're supposed to
be using the same versions of Clirr, aren't we?

Can you make your Clirr reports available so we can evaluate whether we
should fix those additional errors in a second RC?

Thanks

        Stefan

---------------------------------------------------------------------
To unsubscribe, e-mail: [hidden email]
For additional commands, e-mail: [hidden email]

Reply | Threaded
Open this post in threaded view
|

Re: [VOTE] Release Commons Compress 1.6

Phil Steitz
In reply to this post by Stefan Bodewig
On 10/13/13 9:05 PM, Stefan Bodewig wrote:

> On 2013-10-13, Phil Steitz wrote:
>
>> I am sorry.  I forgot one other thing to verify.  The clirr report
>> complains about dropping a field.  Is this spurious / not really an
>> issue?
> Ah yes, I should have talked about that.
>
> It is a protected field in the Tar*Stream classes which should have
> never been protected but was for hisorical reasons.
>
> When the change was made a few month ago we concluded it would be
> extremely unlikely that subclasses of said streams existed that used it,
> in particular since the type of the protected field was a package
> private class (TarBuffer).
>
> The implementation has been changed considerably and the TarBuffer class
> has even been removed - so if this change blocks the release the only
> mitigation will be to revert the changes completely.  I'm fully prepared
> to do that, if necessary.

Thanks, Stefan.  This is a good example of the "relaxing" of
backward compat requirements that I think we need to allow.  I am +1
for allowing the break without major version bump / package-munging
in this case.

Phil
>
> Stefan
>
> ---------------------------------------------------------------------
> To unsubscribe, e-mail: [hidden email]
> For additional commands, e-mail: [hidden email]
>
>


---------------------------------------------------------------------
To unsubscribe, e-mail: [hidden email]
For additional commands, e-mail: [hidden email]

Reply | Threaded
Open this post in threaded view
|

[CANCELLED][VOTE] Release Commons Compress 1.6

Stefan Bodewig
In reply to this post by Stefan Bodewig
I think enough issues have been identified to warrant a second RC.

I'll take care of the bad version number in the release notes as well as
the PMD warnings in the ARJ-Package.  ArjArchiveEntry's test coverage
has already been increased in trunk.

We should talk about the Clirr report further and come to a conclusion
what to do about the changes - if anything at all.

For the site's contents I don't see myself working on it before a second
RC.  Help is more than welcome (not only with the site, of course).

Thanks to all who took the time to review the RC

       Stefan

---------------------------------------------------------------------
To unsubscribe, e-mail: [hidden email]
For additional commands, e-mail: [hidden email]

Reply | Threaded
Open this post in threaded view
|

Re: [CANCELLED][VOTE] Release Commons Compress 1.6

David Mollitor
Just a couple of immediate thoughts (just starting to look things over...)

In SevenZFile.java

Constructor...
1) Close file on exception instead of the current technique of keeping
a "succeeded" flag.
2) Move setting the password to the last line of the constructor so
that it isn't stored if an exception is thrown at any point.

General...
1) I see a few instances of
        if (file != null) {
            try {
                file.close();
            } catch (IOException ignored) { // NOPMD
            }
            file = null;
        }
Can we snag the the IoUtils close methods in this project?

2) The class JavaDoc comments are wrapped after about 50 characters..
extend to 80?
3) There's a 'leaked resource' warning on line 190.  A
ByteArrayInputStream is not being closed, which makes sense as closing
it does nothing, but for completeness sake perhaps or to protect
against a time that the input-source could change and then it really
is leaking.
4) Non-standard logging.  There are many logging statements in the
code... perhaps delegate that to a known logging framework or remove
it all entirely?
5) Line 870 - magic number

Thanks,
David

On Mon, Oct 14, 2013 at 12:17 AM, Stefan Bodewig <[hidden email]> wrote:

> I think enough issues have been identified to warrant a second RC.
>
> I'll take care of the bad version number in the release notes as well as
> the PMD warnings in the ARJ-Package.  ArjArchiveEntry's test coverage
> has already been increased in trunk.
>
> We should talk about the Clirr report further and come to a conclusion
> what to do about the changes - if anything at all.
>
> For the site's contents I don't see myself working on it before a second
> RC.  Help is more than welcome (not only with the site, of course).
>
> Thanks to all who took the time to review the RC
>
>        Stefan
>
> ---------------------------------------------------------------------
> To unsubscribe, e-mail: [hidden email]
> For additional commands, e-mail: [hidden email]
>

---------------------------------------------------------------------
To unsubscribe, e-mail: [hidden email]
For additional commands, e-mail: [hidden email]

Reply | Threaded
Open this post in threaded view
|

Re: [CANCELLED][VOTE] Release Commons Compress 1.6

David Mollitor
In reply to this post by Stefan Bodewig
org.apache.commons.compress.compressors.lzma.LZMACompressorInputStream#read()

Current: count(ret == -1 ? -1 : 1);
Change: count(ret == -1 ? 0 : 1);

On Mon, Oct 14, 2013 at 12:17 AM, Stefan Bodewig <[hidden email]> wrote:

> I think enough issues have been identified to warrant a second RC.
>
> I'll take care of the bad version number in the release notes as well as
> the PMD warnings in the ARJ-Package.  ArjArchiveEntry's test coverage
> has already been increased in trunk.
>
> We should talk about the Clirr report further and come to a conclusion
> what to do about the changes - if anything at all.
>
> For the site's contents I don't see myself working on it before a second
> RC.  Help is more than welcome (not only with the site, of course).
>
> Thanks to all who took the time to review the RC
>
>        Stefan
>
> ---------------------------------------------------------------------
> To unsubscribe, e-mail: [hidden email]
> For additional commands, e-mail: [hidden email]
>

---------------------------------------------------------------------
To unsubscribe, e-mail: [hidden email]
For additional commands, e-mail: [hidden email]

Reply | Threaded
Open this post in threaded view
|

Re: [CANCELLED][VOTE] Release Commons Compress 1.6

garydgregory
In reply to this post by David Mollitor
On Mon, Oct 14, 2013 at 12:03 PM, dam6923 . <[hidden email]> wrote:

> Just a couple of immediate thoughts (just starting to look things over...)
>
> In SevenZFile.java
>
> Constructor...
> 1) Close file on exception instead of the current technique of keeping
> a "succeeded" flag.
> 2) Move setting the password to the last line of the constructor so
> that it isn't stored if an exception is thrown at any point.
>
> General...
> 1) I see a few instances of
>         if (file != null) {
>             try {
>                 file.close();
>             } catch (IOException ignored) { // NOPMD
>             }
>             file = null;
>         }
> Can we snag the the IoUtils close methods in this project?
>
> 2) The class JavaDoc comments are wrapped after about 50 characters..
> extend to 80?

120 is common in active [commons] projects these days.

Gary

> 3) There's a 'leaked resource' warning on line 190.  A
> ByteArrayInputStream is not being closed, which makes sense as closing
> it does nothing, but for completeness sake perhaps or to protect
> against a time that the input-source could change and then it really
> is leaking.
> 4) Non-standard logging.  There are many logging statements in the
> code... perhaps delegate that to a known logging framework or remove
> it all entirely?
> 5) Line 870 - magic number
>
> Thanks,
> David
>
> On Mon, Oct 14, 2013 at 12:17 AM, Stefan Bodewig <[hidden email]> wrote:
>> I think enough issues have been identified to warrant a second RC.
>>
>> I'll take care of the bad version number in the release notes as well as
>> the PMD warnings in the ARJ-Package.  ArjArchiveEntry's test coverage
>> has already been increased in trunk.
>>
>> We should talk about the Clirr report further and come to a conclusion
>> what to do about the changes - if anything at all.
>>
>> For the site's contents I don't see myself working on it before a second
>> RC.  Help is more than welcome (not only with the site, of course).
>>
>> Thanks to all who took the time to review the RC
>>
>>        Stefan
>>
>> ---------------------------------------------------------------------
>> To unsubscribe, e-mail: [hidden email]
>> For additional commands, e-mail: [hidden email]
>>
>
> ---------------------------------------------------------------------
> To unsubscribe, e-mail: [hidden email]
> For additional commands, e-mail: [hidden email]
>



--
E-Mail: [hidden email] | [hidden email]
Java Persistence with Hibernate, Second Edition
JUnit in Action, Second Edition
Spring Batch in Action
Blog: http://garygregory.wordpress.com
Home: http://garygregory.com/
Tweet! http://twitter.com/GaryGregory

---------------------------------------------------------------------
To unsubscribe, e-mail: [hidden email]
For additional commands, e-mail: [hidden email]

Reply | Threaded
Open this post in threaded view
|

Re: [CANCELLED][VOTE] Release Commons Compress 1.6

David Mollitor
In reply to this post by Stefan Bodewig
org.apache.commons.compress.archivers.ar.ArArchiveInputStream#getNextArEntry()

The code to "skip to the next available entry" looks a bit inefficient
because it reads one byte at a time in a loop.  Consider using
org.apache.commons.compress.utils.IOUtils.skip(InputStream, long)

Consider using org.apache.commons.compress.utils.IOUtils.readFully(InputStream,
byte[], int, int) on line 98 or else there may be an unnecessary
exception thrown.

In fact, everywhere there is a read for header information, consider
using "readFully".  Right now, lines 118-124 are not doing readFully
and the return value is not being checked.

On Mon, Oct 14, 2013 at 12:17 AM, Stefan Bodewig <[hidden email]> wrote:

> I think enough issues have been identified to warrant a second RC.
>
> I'll take care of the bad version number in the release notes as well as
> the PMD warnings in the ARJ-Package.  ArjArchiveEntry's test coverage
> has already been increased in trunk.
>
> We should talk about the Clirr report further and come to a conclusion
> what to do about the changes - if anything at all.
>
> For the site's contents I don't see myself working on it before a second
> RC.  Help is more than welcome (not only with the site, of course).
>
> Thanks to all who took the time to review the RC
>
>        Stefan
>
> ---------------------------------------------------------------------
> To unsubscribe, e-mail: [hidden email]
> For additional commands, e-mail: [hidden email]
>

---------------------------------------------------------------------
To unsubscribe, e-mail: [hidden email]
For additional commands, e-mail: [hidden email]

Reply | Threaded
Open this post in threaded view
|

Re: [VOTE] Release Commons Compress 1.6

Oliver Heger-3
In reply to this post by Stefan Bodewig
Am 14.10.2013 06:11, schrieb Stefan Bodewig:

> On 2013-10-13, Oliver Heger wrote:
>
>> The artifacts look good, the build works fine with Java 1.7 on Windows
>> 7. I tried to build with Java 1.5, but got:
>
>> Tests in error:
>
>> SevenZTestCase.testSevenZArchiveCreationUsingLZMA2:37->testSevenZArchiveCreati
>> on:59 ╗ OutOfMemory
>>   XZTestCase.testXZCreation:44 ╗ OutOfMemory Java heap space
>
>> Tests run: 374, Failures: 0, Errors: 2, Skipped: 0
>
>> ISTR that I had similar problems with other [compress] releases, so I
>> don't consider this as blocking.
>
> I don't recall it, but XZ is pretty heavy on GC pressure.  Given that
> our XZ implementation is a very thin layer on top of XZ for Java there
> wouldn't be anything we could do - apart from increasing the heap size
> for the tests - anyway.
>
>> The title in the release notes says "Apache Commons Compress 1.5 RELEASE
>> NOTES"; it refers to the old version. I think, this requires another RC.
>
> Ouch.
>
>> The RC was built using Java 1.6.0_27. Does this version already contain
>> the Javadoc security fix?
>
> The javadoc plugin used by the Commons Parent took care of it.
>
>> When I build the site locally, I get a different clirr report showing 9
>> errors because some public methods have been made final.
>
> Strange.  Why would Clirr create different reports?  We're supposed to
> be using the same versions of Clirr, aren't we?
>
> Can you make your Clirr reports available so we can evaluate whether we
> should fix those additional errors in a second RC?

I have no clue why Clirr produces different results for me, and the
errors it reports look indeed a bit strange. I uploaded my results of
mvn site:site (using JDK 1.7 - maybe this is the reason for the
difference?) to [1]

Oliver

[1] http://people.apache.org/~oheger/compress-site/

>
> Thanks
>
>         Stefan
>
> ---------------------------------------------------------------------
> To unsubscribe, e-mail: [hidden email]
> For additional commands, e-mail: [hidden email]
>


---------------------------------------------------------------------
To unsubscribe, e-mail: [hidden email]
For additional commands, e-mail: [hidden email]

Reply | Threaded
Open this post in threaded view
|

[compress] Clirr (was Re: [VOTE] Release Commons Compress 1.6)

Stefan Bodewig
On 2013-10-14, Oliver Heger wrote:

> Am 14.10.2013 06:11, schrieb Stefan Bodewig:
>> On 2013-10-13, Oliver Heger wrote:

>>> When I build the site locally, I get a different clirr report showing 9
>>> errors because some public methods have been made final.

>> Strange.  Why would Clirr create different reports?  We're supposed to
>> be using the same versions of Clirr, aren't we?

>> Can you make your Clirr reports available so we can evaluate whether we
>> should fix those additional errors in a second RC?

> I have no clue why Clirr produces different results for me, and the
> errors it reports look indeed a bit strange.

> I uploaded my results of mvn site:site (using JDK 1.7 - maybe this is
> the reason for the difference?) to [1]

> [1] http://people.apache.org/~oheger/compress-site/

Thanks!

For the site build I tend to use OpenJDK 7 - the release artifacts have
been built on a different machine than the site.

All "extra" errors are about the values method of enums.  The code
hasn't changed, maybe your compiler creates final values methods in
enums while the one used to build 1.5 did not?

Stefan

---------------------------------------------------------------------
To unsubscribe, e-mail: [hidden email]
For additional commands, e-mail: [hidden email]

Reply | Threaded
Open this post in threaded view
|

[compress] Dave's Code Review (was Re: [CANCELLED][VOTE] Release Commons Compress 1.6)

Stefan Bodewig
In reply to this post by David Mollitor
Hi

I'm going to address Dave's three mails in a single response

dam6923 . wrote:

> In SevenZFile.java
>
> Constructor...
> 1) Close file on exception instead of the current technique of keeping
> a "succeeded" flag.

This means I have to catch and rethrow the exception.  I don't think I
like this better than the flag.

> 2) Move setting the password to the last line of the constructor so
> that it isn't stored if an exception is thrown at any point.

As the code stands this won't work as readHeaders needs the password
field.  But we probably should store the password as a char[] anyway and
clear it when done - I'll change that before cutting the next release.

> General...
> 1) I see a few instances of
>         if (file != null) {
>             try {
>                 file.close();
>             } catch (IOException ignored) { // NOPMD
>             }
>             file = null;
>         }
> Can we snag the the IoUtils close methods in this project?

yep, but after the 1.6 release.

> 2) The class JavaDoc comments are wrapped after about 50 characters..
> extend to 80?

It may look wierd, but does it hurt?

> 3) There's a 'leaked resource' warning on line 190.  A
> ByteArrayInputStream is not being closed, which makes sense as closing
> it does nothing, but for completeness sake perhaps or to protect
> against a time that the input-source could change and then it really
> is leaking.
> 4) Non-standard logging.  There are many logging statements in the
> code... perhaps delegate that to a known logging framework or remove
> it all entirely?
> 5) Line 870 - magic number

will fix before the release (and remove the logging).

> org.apache.commons.compress.compressors.lzma.LZMACompressorInputStream#read()
>
> Current: count(ret == -1 ? -1 : 1);
> Change: count(ret == -1 ? 0 : 1);

doesn't make a difference if you look at count's implementation.  Will
change it anyway.

> org.apache.commons.compress.archivers.ar.ArArchiveInputStream#getNextArEntry()
>
> The code to "skip to the next available entry" looks a bit inefficient
> because it reads one byte at a time in a loop.  Consider using
> org.apache.commons.compress.utils.IOUtils.skip(InputStream, long)
>
> Consider using org.apache.commons.compress.utils.IOUtils.readFully(InputStream,
> byte[], int, int) on line 98 or else there may be an unnecessary
> exception thrown.
>
> In fact, everywhere there is a read for header information, consider
> using "readFully".  Right now, lines 118-124 are not doing readFully
> and the return value is not being checked.

I agree we could improve this, but I'd prefer to keep the changes before
cutting the next RC down to a minimum.  So "yes, but for 1.7".

Stefan

---------------------------------------------------------------------
To unsubscribe, e-mail: [hidden email]
For additional commands, e-mail: [hidden email]

Reply | Threaded
Open this post in threaded view
|

Re: [compress] Dave's Code Review (was Re: [CANCELLED][VOTE] Release Commons Compress 1.6)

Stefan Bodewig
On 2013-10-15, Stefan Bodewig wrote:

> But we probably should store the password as a char[] anyway

s/char/byte/

Stefan

---------------------------------------------------------------------
To unsubscribe, e-mail: [hidden email]
For additional commands, e-mail: [hidden email]

12