commons-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Niall Pemberton" <niall.pember...@gmail.com>
Subject Re: VOTE: Release commons-fileupload 1.2
Date Mon, 27 Nov 2006 04:13:22 GMT
On 11/27/06, Jochen Wiedmann <jochen.wiedmann@gmail.com> wrote:
> Hi, Niall,
>
> On 11/26/06, Niall Pemberton <niall.pemberton@gmail.com> wrote:
>
> > I would have preferred the release be cut using maven1 rather than m2.
> > The maven1 build is tried and tested and the gripes of those of us
> > that checked out previous releases have been fixed in the maven1
> > build. I guess that doesn't matter if the release is up to scratch but
> > would be interested to know if others think we're ready for releases
> > using m2 yet?
>
> See for example
>
>     http://marc.theaimsgroup.com/?l=jakarta-commons-dev&m=115714503628112&w=2
>     http://marc.theaimsgroup.com/?t=115719054100004
>
> > The first issue I have with checking out this RC is that you've only
> > posted the "tar.gz" source and binary distros. I would have liked to
> > see the full set - zip versions and md5 checksums (the maven1 build
> > for fileupload creates the md5 checksums for you).
>
> Are you actually going to tell me, that this is an issue? (The archive
> type, not the checksum, which must of course, be added to
> distributables. As are .sha1 and .asc files, btw.) If so, changing it
> is as simple as adding one line to the assembly descriptor. But who do
> you believe is unable to use tar.gz in 2006?

I actually assumed you just hadn't uploaded them. But I'm glad now I
raised it since zip distros are artifacts the release should have
(whether you think they're a waste of time or not) - as the build
wasn't actually generating them then obviously it was a flaw which
needed fixing - which I'm glad you now have.

The real underlying issue here though is that I haven't seen you do a
release before - so I have nothing to give me confidence. The way to
starting building that is to at least produce all the artifacts for
the release. I'd have been happier if you'd followed what people
usually do - tagged the repository, cut the release candidate, made it
available for review and then called a vote.  You don't have to do any
of this, but you're more likely to get my vote if you do - and its
what I and quite a few others do:

   http://people.apache.org/~niallp/validator-1.3.1-rc1/
   http://people.apache.org/~rahul/commons/digester/
   http://people.apache.org/~bayard/commons-dbutils/1.1-RC1/

> > I think there are three serious issues with this RC:
> > 1) It doesn't comply with the new "ASF Source Header and Copyright
> > Notice Policy":
> >     http://www.apache.org/legal/src-headers.html
>
> I wasn't aware of a change in policy. You are right, that must be
> honoured. However, you already did it, so it cannot be counted as a
> reason for a -1, right?

Its a -1 for RC1 - now its fixed though should mean its not an issue
if you cut a RC2

> > 2) According to the jar's manifest file its been built using JDK
> > 1.6.0-rc. Even if JDK 1.6 had been (just) released using it for a
> > release would make me nervous - but using a RC version of Java to cut
> > the fileupload release is bad news IMO.
>
> Ok, I'll keep that in mind for the next approach.
>
>
> > 3) The clirr report you produced: which shows the following
> > incompatibilities with the previous fileupload version:
> >
> > 1) FileUploadBase - public constant MAX_HEADER_SIZE removed.
> > 2) FileUploadBase - protected method createItem removed
> > 3) FileUploadBase - two public constructors for
> > SizeLimitExceededException removed
> > 4) FileUploadBase - public static class UnknownSizeException removed
> > 5) MulitpartStream - gone from public to package visibility
>
>    1) A maximum header size no longer exists. See FILEUPLOAD-108.
>    2)+5) See
>      http://marc.theaimsgroup.com/?l=jakarta-commons-dev&m=114988352104258&w=2
>    4) This exception can no longer be thrown, because content-length=-1
>        is allowed now.
>
> All in all, do you really think that's as big an issue as you make it?

I think breaking backwards compatibility in a widely used library like
fileupload is a serious issue and as no-one else had reviewed your
release candidate I think highlighting it was important.  I agree the
issues don't look big - but in terms of compatibility one small change
could cause jar hell in a widely used library. Have you at least done
anything to verify whether any of these changes cause a problem with
any of the frameworks that use fileupload? I only went by the clirr
report and am not famailiar with why you removed these - but its
obvious that at least some of them (if not all) could have been
deprecated rather than removed. If there are good reasons why breaking
binary compatibility is preferable (or necessary) to deprecating then
IMO it would be good for you to state them and then it needs to be
decided whether putting out an incompatible release is acceptable or
the package name name should be changed:

http://www.mail-archive.com/commons-dev@jakarta.apache.org/msg85642.html

> > One thing thats disappointing is that the last fileupload release had
> > only 5 checkstyle issues. This one has 376 of which 200 are for tab
> > characters which I personally dislike in source.
>
> The reason is, IMO, that checkstyle is quite outdated. Examples:
>
> - Why should I add a comment to a field called serialalversionuid?
> - Why should I add a comment to an implementation or overwritten
>   superclass method, when I know that the doclet will simply copy
>   the interface or
> - It's ridiculous to require comments for private fields, even if they are
>   simply storage for getters and setters.
> - What's the problem with trailing blanks? (I can understand your
>   sentiment against tabs, btw.)
>
> Please note, that I have enabled almost any warning that Eclipse
> can trigger. And, believe me, these are more and more serious matters
> than checkstyle detects. Nevertheless, I have eliminated almost all
> warnings today, except those I cannot omit and I refuse to remove,
> because I know better than checkstyle that it makes sense to keep
> them. The code I have added contains *no* Eclipse warning. Elder
> code does.

I agree that checkstyle highlights things that are not an issue (e.g.
private fields) - mostly its just a case of changing the checkstyle
config to not emit the warnings for them. Having alot of trivial
issues reported hides the bigger ones though. The way commons works is
that it usually needs developers who are not working on a component to
vote to get a release out. While you may know there are no issues -
those of use not working on the code only have the reports to go by.
Having said that, AFAIK its not actually necessary to even have a
checkstyle report to do a release - but I'm happier if there is one
and that it is either clear or has very little on it. Thanks for
cleaning it up though :-)

Niall


> Jochen

---------------------------------------------------------------------
To unsubscribe, e-mail: commons-dev-unsubscribe@jakarta.apache.org
For additional commands, e-mail: commons-dev-help@jakarta.apache.org


Mime
View raw message