[Pkg-mozext-maintainers] RFS: stylish/1.3.1+git20130115-1 [ITP]

[Benjamin Drung]

Am Freitag, den 01.02.2013, 02:33 +1100 schrieb Dmitry Smirnov:
> On Fri, 1 Feb 2013 02:05:14 Benjamin Drung wrote:
> > I did a review:
> 
> Thanks!
> 
> > 1) get-orig-source generates a tarball that differs in the
> > stylish/ChangeLog file.
> 
> Differs?
> It generates ChangeLog (if not present). This is intentional.
> Where/why do you see a problem?

The problem is that the generated ChangeLog differs (diff attached) and
that causes dpkg-source to complain. I prefer to have a get-orig-source
rules that is reproducible.

> > 2) Please run wrap-and-sort (the build dependencies are strangely
> > formatted).
> 
> No thanks. I found wrap-and-sort to generate incorrect results in some cases.
> I don't trust this tool because sometimes it drops valid fields (#694142) and 

That bug is caused by the underlying library.

> I don't even like it because it produces weird sorting that goes against 
> common sense.

Weird sorting? I sorts the stuff alphabetical.

> As you can see "strange formatting" is intentional and VCS-friendly.
> What's the problem?

I find it strange to have a line beginning with a comma followed by the
build dependency. When wrapping stuff in the normal language, you put
the comma after the word and then wrap the line.

You have "debhelper (>= 9), mozilla-devscripts" on one line, but then
only one build dependency listed per line.

> You can always run wrap-and-sort if I retire from maintenance of this package.
> 
> I think we already had this discussion.
> 
> Please don't make usage of wrap-and-sort requirement for packaging.
> As far as I'm aware no other teams insist on using wrap-and-sort.

wrap-and-sort is no requirement. Having a consistent formatting and a
rule for sorting is recommended.

> > 3) Please add a minimum version for the mozilla-devscripts build
> > dependency: http://wiki.debian.org/mozilla-devscripts
> 
> You meant "mozilla-devscripts (>= 0.22~)" right?

Yes.

> Perhaps that's unnecessary as we have 0.23 in stable.

Having someone trying to build the package with a too old
mozilla-devscripts is very unlikely, but not impossible.

> Besides we didn't do it for https-finder.
> 
> I prefer to leave as is.

Okay.

> > 4) Why do you set --max-parallel=4 in debian/rules?
> 
> Safety. Once I had experience with FTBFS on 6 cores when it was all fine on 4 
> cores. I can't attempt to build on more than 4 cores so I limited to 
> known/proven/working configuration that I actually tested. Perhaps too 
> paranoid in this case but you never know and it's better to be safe than 
> sorry...

Having a FTBFS on six core is a strong indicator for a race condition.
Restricting the number of parallel jobs can work around the issue, but
is no fix in such cases.

-- 
Benjamin Drung
Debian & Ubuntu Developer
-------------- next part --------------
A non-text attachment was scrubbed...
Name: changelog.diff
Type: text/x-patch
Size: 333 bytes
Desc: not available
URL: <http://lists.alioth.debian.org/pipermail/pkg-mozext-maintainers/attachments/20130131/b54ee62e/attachment.bin>