[Pkg-javascript-devel] review of jquery-lazyload version 1.9.3

[Emilien Klein]

Hi Jeremy and team,

2014-09-09 22:54 GMT+02:00 Jérémy Lal <kapouer at melix.org>:
> Hi,
>
> * first you need to learn how to properly repackage an upstream tarball,
> because the "good" way is so easy and fun !
> Just add a Files-Excluded section in debian/copyright as explained in
> https://wiki.debian.org/UscanEnhancements
> then run
> uscan --force-download
> and voilà - a nice repackaged tarball.
> Then it's only a matter of renaming it (sometimes it doesn't add +dfsg
> to the upstream version in the tarball file name), and
> git-import-orig ../thattarball_1.2.3+dfsg.orig.tar.gz

Yes, that is indeed much cleaner.
As part of my involvement with the Debian Med team, Andreas Tille has
made me aware of this nice mechanism (Andreas pushed for this, see
#685787).
Since the 1.9.3 version is already in the git repo, it doesn't make
much sense to modify this now, but I will transition to Files-Excluded
when the next upstream version is released. That will also allow to
remove d/download_repackage_dfsg.sh

> * do not call uupdate in uscan, it gets in the way of git-buildpackage

Done.

> * note that directly modifying upstream files manually in a
> git-buildpackage-maintained repository is NOT a good idea.
> You're supposed to modify only debian/ dir, and use git-import-orig for
> anything else.

There are several branches in this git repo:
* upstream: the unmodified upstream files, containing e.g. the
minified js files that get removed in the DFSG-ing process
* pristine-tar: used to keep the deltas from generated by pristine-tar
* dfsg_clean: that gets refreshed to the latest upstream version from
the upstream branch, and then the non-DFSG-approved files get removed.
After that action, the branch is in the same state as Files-Excluded
would bring it to, if it was already being used (upstream files, no
non-DFSG files, no /debian directory)
* master: that's where the package is build from. It is the content of
dfsg_clean, and contains the debian repository.

The changes to the debian source package are only performed in the
master branch.
Do you have concerns with that for now (until the next release and the
move to Files-Excluded, which will result in loosing the actual
untouched upstream content)?

> * debian/README.source is interesting... but anything inside it is
> already documented on wiki.debian.org or other manuals. Switching to
> Files-Excluded and uscan allows you to get rid of that file.

As this package doesn't get updated often, I like to keep the full
instructions in a local text file (it's a reminder to my future self,
to allow for reproducible builds).
The one time I felt smart enough to not look at it before packaging
the new version from a new development machine, I forgot to checkout
the non-master branches, which resulted in conflicts when pushing
upstream. That forced me to create a new repo on our Git server, as I
mentioned in [0].
I will leave this file for now, and will update it when switching to
Files-Excluded.
[0] http://lists.alioth.debian.org/pipermail/pkg-javascript-devel/2014-February/007044.html

> * debian/README.Debian: you should remove everything that is already in
> README.textile. You should mention javascript-common.

Done.

> * debian/control: recommend javascript-common,
>   see https://wiki.debian.org/Javascript/Policy

Done (used to be a Suggest, not sure why I removed it in commit 2f93988b)

> * debian/download_repackage_dfsg.sh: no longer needed

Agreed.
See above, will be removed when the next upstream version is released.

> * debian/rules: README.textile is not a changelog,
>  simply add debian/docs containing "README.textile"

70.5% of the lines (86 of 122) are changelog information, hence my
call to override_dh_installchangelogs.
But I've changed that and marked it as purely a doc.

> * debian/patches: please use DEP3 patch format. Example:
> http://anonscm.debian.org/cgit/collab-maint/nodejs.git/tree/debian/patches/1005_restore_sh_javascript_nonminified.patch

Done. Thanks for pointing that out, I was familiar with adding a
description using `dquilt header -e`, but wasn't familiar about DEP-3
itself.


I've also addressed the new Lintian warning
W: jquery-lazyload source: space-in-std-shortname-in-dep5-copyright
cc-by 3.0 (paragraph at line 49)


> Thank you for your contribution to Debian !

Thanks for your review.
I've pushed my changes to pkg-javascript/jquery-lazyload.git on
Alioth. Let me know if there is anything else you'd like adapted.
    +Emilien