You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@commons.apache.org by Damjan Jovanovic <da...@gmail.com> on 2013/10/21 16:25:30 UTC
Re: [VOTE] Release Imaging 1.0 from RC4
On Tue, Sep 25, 2012 at 3:22 PM, Gary Gregory <ga...@gmail.com> wrote:
> Hi All,
>
> I happy to see another RC come along! :) This is large code base so I
> appreciate that the reports generate a lot of potential work.
I planned to send this email out just over a year ago, shortly after
yours, but since we might actually get to release this time let me
send it out now for reference.
> -1: Unapproved license:
>
> src/main/java/org/apache/commons/imaging/formats/jpeg/segments/App14Segment.java
Fixed.
> Fix obvious FindBugs (to me only?):
>
> - Possible NPE with
> NP_IMMEDIATE_DEREFERENCE_OF_READLINE<http://findbugs.sourceforge.net/bugDescriptions.html#NP_IMMEDIATE_DEREFERENCE_OF_READLINE>@
> https://people.apache.org/~damjan/imaging-1.0rc4/xref/org/apache/commons/imaging/formats/rgbe/RgbeInfo.html#103
Findbugs false positive: it believes every method named "readLine()"
behaves like BufferedReader's. I've renamed it to "readNextLine()".
> - Is this useless or an incomplete impl?: Useless control flow in
> org.apache.commons.imaging.palette.PaletteFactory.makePaletteFancy(BufferedImage)
> STYLE UCF_USELESS_CONTROL_FLOW<http://findbugs.sourceforge.net/bugDescriptions.html#UCF_USELESS_CONTROL_FLOW>
> 54<https://people.apache.org/%7Edamjan/imaging-1.0rc4/xref/org/apache/commons/imaging/palette/PaletteFactory.html#54>
Incomplete impl, fixed now.
> -
> If we want non-standard class name (starts with a lower case, then Javadoc
> would be nice to understand why this is justified:
> public static class tEXt extends PngText
> public static class zTXt extends PngText
> public static class iTXt extends PngText
Because that's the case used for PNG field markers. Converted to
standard names anyway.
> - What about the
> RCN_REDUNDANT_NULLCHECK_OF_NONNULL_VALUE<http://findbugs.sourceforge.net/bugDescriptions.html#RCN_REDUNDANT_NULLCHECK_OF_NONNULL_VALUE>?
The only reason that value is not null, is because the method
providing it hasn't been overridden - and that method is neither
private nor final so it can be overridden. Fixed anyway.
> - What about the
> FE_FLOATING_POINT_EQUALITY<http://findbugs.sourceforge.net/bugDescriptions.html#FE_FLOATING_POINT_EQUALITY>?
Probably a false positive (comparison of variables to the maximum
amongst themselves) but I worked around it.
> - IIRC, the EI_EXPOSE_REP<http://findbugs.sourceforge.net/bugDescriptions.html#EI_EXPOSE_REP>issues
> are just a normal by-product of the design? Please confirm.
Yes they are.
> - PMD
> - do the easy clean ups of "Unnecessary final modifier in final class".
Done.
> - "Avoid empty catch blocks": Add a comment as to why the block is empty,
> for example "This exception cannot be thrown because...". This is important
> for anyone who tries to grok the code.
The name I give to the exception variable (ignore, cannotHappen)
should make it obvious.
> - "Avoid unused method/fields/etc": Is this unimplemented code or cruft?
> If cruft, it should be removed before we give ourselves unneeded BC
> headaches.
>
> - CDP: lots of those but I do not know the code base enough to tell how
> easy it would be to refactor duplications and if refactoring is justified.
>
> - Code coverage: overall, is not great.
>
> These packages have 0%.code coverage.
> - org.apache.commons.imaging.formats.transparencyfilters
> - org.apache.commons.imaging.formats.psd.dataparsers
> - org.apache.commons.imaging.formats.psd.datareaders
> - org.apache.commons.imaging.icc
>
> Oddly org.apache.commons.imaging.util has only 17% which I would think
> would be higher since it is a util package.
Even to get that low test coverage, it already takes 40+ minutes to
run "mvn site" due to Corbertura. I've added some tests so it's > 0
now. The real "util" package is org.apache.commons.imaging.common.
> - Process
>
> This might be a case of RM process but when I RM I set the release date in
> changes.xml to the date I cut the RC. If the RC passes, then you are done,
> otherwise you edit it again for the next RC.
Ok thank you.
> - Javadoc
>
> Only 3 out of ~40 packages have comments, which makes it harder to find
> your way around.
Added a few more package comments, but the API is largely designed to
be used from the org.apache.commons.imaging.Imaging class alone.
> - Checkstyle is not strict enough
> - IMO should always complain about missing { } in blocks.
I thought there was no official coding style for commons? But I've
added it anyway.
> There also a lot of generics warnings (in Eclipse, any IDE will tell you
> these days.)
Upgrades from Java 1.4 are never fun. These were largely fixed in
code, but tests might have a few.
> Sorry to make it a long laundry list, but there is only one 1.0 ;)
You should see what was already fixed :).
> There is probably more of course, but I have to go now...
>
> Gary
Thank you very much
Damjan
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@commons.apache.org
For additional commands, e-mail: dev-help@commons.apache.org
Re: [VOTE] Release Imaging 1.0 from RC4
Posted by sebb <se...@gmail.com>.
On 21 October 2013 18:49, Andreas Lehmkuehler <an...@lehmi.de> wrote:
> Hi,
>
> Am 21.10.2013 19:33, schrieb sebb:
>
>> On 21 October 2013 18:27, Andreas Lehmkuehler <an...@lehmi.de> wrote:
>>>
>>> Hi,
>>>
>>> Am 21.10.2013 16:25, schrieb Damjan Jovanovic:
>>>>
>>>>
>>>> On Tue, Sep 25, 2012 at 3:22 PM, Gary Gregory <ga...@gmail.com>
>>>> wrote:
>>>>>
>>>>>
>>>>> Hi All,
>>>>>
>>>>> I happy to see another RC come along! :) This is large code base so I
>>>>> appreciate that the reports generate a lot of potential work.
>>>>
>>>>
>>>>
>>>> I planned to send this email out just over a year ago, shortly after
>>>> yours, but since we might actually get to release this time let me
>>>> send it out now for reference.
>>>>
>>>> SNIP
>>>
>>>
>>> I ran a dry maven test (mvn release:prepare -DdryRun=true) and everything
>>> looks good. But I got a lot of redundant artifacts
>>>
>>>
>>> commons-imaging-1.0-SNAPSHOT.jar
>>> commons-imaging-1.0-SNAPSHOT-bin.zip
>>> commons-imaging-1.0-SNAPSHOT-bin.tar.gz
>>>
>>> commons-imaging-1.0-SNAPSHOT-source-release.zip
>>> commons-imaging-1.0-SNAPSHOT-sources.jar
>>> commons-imaging-1.0-SNAPSHOT-src.zip
>>> commons-imaging-1.0-SNAPSHOT-src.tar.gz
>>>
>>> I guess those are not created with intent, are they?
>>
>>
>> The bin and src zip and targz archives _are_ needed; these are
>> released to the ASF mirrors.
>> [That they are attached to the project and end up in Nexus staging is
>> a long story.]
>
> Is this a commons policy?
The duplicate zip and tar archives are common to all commons components (AFAIK).
I think the reason for this is that not all Windows systems support
tar archives, and until recently many Unix systems did not support
Zip; also zip is bigger.
Maven releases include binary jars (obviously) as well as source and
javadocs and test jars.
The source and javadoc jars are useful for IDE debugging.
The test jar is useful for compatibility testing.
Have a look at the other components under
http://www.apache.org/dist/commons/
and
http://repo1.maven.org/maven2/org/apache/commons/
I think you'll find quite a lot of other projects follow much the same strategy.
> PDFBox [1] has one source archive and a couple
> of precompiled jars (one for each component and 2 bundling all neede
> components
> for 2 typical usecases).
>
>
>> source-release.zip may be spurious.
>>
>> jar and sources.jar look like standard Maven jars.
>>
>>>
>>>> Thank you very much
>>>> Damjan
>>>
>>>
>>> Thanks for the ongoing effort to release/work on imaging
>>>
>>> BR
>>> Andreas Lehmkühler
>
>
> BR
> Andreas Lehmkühler
> [1] http://www.apache.org/dyn/closer.cgi/pdfbox/1.8.2
>
>
>
> ---------------------------------------------------------------------
> To unsubscribe, e-mail: dev-unsubscribe@commons.apache.org
> For additional commands, e-mail: dev-help@commons.apache.org
>
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@commons.apache.org
For additional commands, e-mail: dev-help@commons.apache.org
Re: [VOTE] Release Imaging 1.0 from RC4
Posted by Andreas Lehmkuehler <an...@lehmi.de>.
Hi,
Am 21.10.2013 19:33, schrieb sebb:
> On 21 October 2013 18:27, Andreas Lehmkuehler <an...@lehmi.de> wrote:
>> Hi,
>>
>> Am 21.10.2013 16:25, schrieb Damjan Jovanovic:
>>>
>>> On Tue, Sep 25, 2012 at 3:22 PM, Gary Gregory <ga...@gmail.com>
>>> wrote:
>>>>
>>>> Hi All,
>>>>
>>>> I happy to see another RC come along! :) This is large code base so I
>>>> appreciate that the reports generate a lot of potential work.
>>>
>>>
>>> I planned to send this email out just over a year ago, shortly after
>>> yours, but since we might actually get to release this time let me
>>> send it out now for reference.
>>>
>>> SNIP
>>
>> I ran a dry maven test (mvn release:prepare -DdryRun=true) and everything
>> looks good. But I got a lot of redundant artifacts
>>
>>
>> commons-imaging-1.0-SNAPSHOT.jar
>> commons-imaging-1.0-SNAPSHOT-bin.zip
>> commons-imaging-1.0-SNAPSHOT-bin.tar.gz
>>
>> commons-imaging-1.0-SNAPSHOT-source-release.zip
>> commons-imaging-1.0-SNAPSHOT-sources.jar
>> commons-imaging-1.0-SNAPSHOT-src.zip
>> commons-imaging-1.0-SNAPSHOT-src.tar.gz
>>
>> I guess those are not created with intent, are they?
>
> The bin and src zip and targz archives _are_ needed; these are
> released to the ASF mirrors.
> [That they are attached to the project and end up in Nexus staging is
> a long story.]
Is this a commons policy? PDFBox [1] has one source archive and a couple
of precompiled jars (one for each component and 2 bundling all neede components
for 2 typical usecases).
> source-release.zip may be spurious.
>
> jar and sources.jar look like standard Maven jars.
>
>>
>>> Thank you very much
>>> Damjan
>>
>> Thanks for the ongoing effort to release/work on imaging
>>
>> BR
>> Andreas Lehmkühler
BR
Andreas Lehmkühler
[1] http://www.apache.org/dyn/closer.cgi/pdfbox/1.8.2
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@commons.apache.org
For additional commands, e-mail: dev-help@commons.apache.org
Re: [VOTE] Release Imaging 1.0 from RC4
Posted by Bernd Eckenfels <ec...@zusammenkunft.net>.
Am 22.10.2013, 13:20 Uhr, schrieb sebb <se...@gmail.com>:
> Ah yes, the Apache parent pom has a release profile, but it does not
> quite do what Commons needs
> That's why the CP release profile was created.
There is a "useReleaseProfile" property which you can set to "false" in
the pom/configuration to make sure it will not activate "release-profile"
and there is a releaseProfile= list of the profiles you want to enforce.
Using both in the POM makes the build more self contained.
Gruss
Bernd
--
http://www.zusammenkunft.net
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@commons.apache.org
For additional commands, e-mail: dev-help@commons.apache.org
Re: [VOTE] Release Imaging 1.0 from RC4
Posted by sebb <se...@gmail.com>.
On 22 October 2013 07:50, Henning Schmiedehausen
<he...@schmiedehausen.org> wrote:
> On Mon, Oct 21, 2013 at 7:33 AM, sebb <se...@gmail.com> wrote:
>
> [...]
>
> source-release.zip may be spurious.
>>
>
> These files get created when running
>
> mvn -DdryRun release:prepare
>
> instead of
>
> mvn -Prelease -DdryRun release:prepare
>
> The first one picks up the sourceReleaseAssemblyDescriptor from the apache
> parent.
Ah yes, the Apache parent pom has a release profile, but it does not
quite do what Commons needs
That's why the CP release profile was created.
> ("spurious" is expert speak for "I have no clue". :-) )
>
> -h
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@commons.apache.org
For additional commands, e-mail: dev-help@commons.apache.org
Re: [VOTE] Release Imaging 1.0 from RC4
Posted by Henning Schmiedehausen <he...@schmiedehausen.org>.
On Mon, Oct 21, 2013 at 7:33 AM, sebb <se...@gmail.com> wrote:
[...]
source-release.zip may be spurious.
>
These files get created when running
mvn -DdryRun release:prepare
instead of
mvn -Prelease -DdryRun release:prepare
The first one picks up the sourceReleaseAssemblyDescriptor from the apache
parent.
("spurious" is expert speak for "I have no clue". :-) )
-h
Re: [VOTE] Release Imaging 1.0 from RC4
Posted by sebb <se...@gmail.com>.
On 21 October 2013 18:27, Andreas Lehmkuehler <an...@lehmi.de> wrote:
> Hi,
>
> Am 21.10.2013 16:25, schrieb Damjan Jovanovic:
>>
>> On Tue, Sep 25, 2012 at 3:22 PM, Gary Gregory <ga...@gmail.com>
>> wrote:
>>>
>>> Hi All,
>>>
>>> I happy to see another RC come along! :) This is large code base so I
>>> appreciate that the reports generate a lot of potential work.
>>
>>
>> I planned to send this email out just over a year ago, shortly after
>> yours, but since we might actually get to release this time let me
>> send it out now for reference.
>>
>> SNIP
>
> I ran a dry maven test (mvn release:prepare -DdryRun=true) and everything
> looks good. But I got a lot of redundant artifacts
>
>
> commons-imaging-1.0-SNAPSHOT.jar
> commons-imaging-1.0-SNAPSHOT-bin.zip
> commons-imaging-1.0-SNAPSHOT-bin.tar.gz
>
> commons-imaging-1.0-SNAPSHOT-source-release.zip
> commons-imaging-1.0-SNAPSHOT-sources.jar
> commons-imaging-1.0-SNAPSHOT-src.zip
> commons-imaging-1.0-SNAPSHOT-src.tar.gz
>
> I guess those are not created with intent, are they?
The bin and src zip and targz archives _are_ needed; these are
released to the ASF mirrors.
[That they are attached to the project and end up in Nexus staging is
a long story.]
source-release.zip may be spurious.
jar and sources.jar look like standard Maven jars.
>
>> Thank you very much
>> Damjan
>
> Thanks for the ongoing effort to release/work on imaging
>
> BR
> Andreas Lehmkühler
>
>
>
> ---------------------------------------------------------------------
> To unsubscribe, e-mail: dev-unsubscribe@commons.apache.org
> For additional commands, e-mail: dev-help@commons.apache.org
>
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@commons.apache.org
For additional commands, e-mail: dev-help@commons.apache.org
Re: [VOTE] Release Imaging 1.0 from RC4
Posted by Andreas Lehmkuehler <an...@lehmi.de>.
Hi,
Am 21.10.2013 16:25, schrieb Damjan Jovanovic:
> On Tue, Sep 25, 2012 at 3:22 PM, Gary Gregory <ga...@gmail.com> wrote:
>> Hi All,
>>
>> I happy to see another RC come along! :) This is large code base so I
>> appreciate that the reports generate a lot of potential work.
>
> I planned to send this email out just over a year ago, shortly after
> yours, but since we might actually get to release this time let me
> send it out now for reference.
>
> SNIP
I ran a dry maven test (mvn release:prepare -DdryRun=true) and everything looks
good. But I got a lot of redundant artifacts
commons-imaging-1.0-SNAPSHOT.jar
commons-imaging-1.0-SNAPSHOT-bin.zip
commons-imaging-1.0-SNAPSHOT-bin.tar.gz
commons-imaging-1.0-SNAPSHOT-source-release.zip
commons-imaging-1.0-SNAPSHOT-sources.jar
commons-imaging-1.0-SNAPSHOT-src.zip
commons-imaging-1.0-SNAPSHOT-src.tar.gz
I guess those are not created with intent, are they?
> Thank you very much
> Damjan
Thanks for the ongoing effort to release/work on imaging
BR
Andreas Lehmkühler
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@commons.apache.org
For additional commands, e-mail: dev-help@commons.apache.org