Bug#847603: RFS: mod_pagespeed/1.11.33.4 [ITP] -- Apache module for rewriting web pages to reduce latency and bandwidth
On Fri, 30 Dec 2016 07:47:09 +0000 Sean Whitton
<spwhitton@spwhitton.name> wrote:
> control: tag -1 +moreinfo
>
> Dear Jeff,
>
> On Fri, Dec 09, 2016 at 02:43:32PM -0500, Jeff Kaufman wrote:
> > I am looking for a sponsor for my package "mod_pagespeed":
Hi... I am one of Jeff's colleagues, he redirected this to me
(now that I am back from vacation)
> This looks cool. Here are some initial comments:
Thank you for your feedback.
> - I'm not prepared to fully review a package that is not available in
> git. We might need multiple rounds of review, and git-diff(1) is
> invaluable for that. Please consider `gbp import-dsc` or `dgit
> import-dsc` (the former is more popular; I prefer the latter)
Uploaded using gbp into
https://github.com/pagespeed/mod_pagespeed-debian-packaging,
along with a few follow up changes. (I am a bit unclear as to whether
I should be messing
around with the package revision field at this point, though).
> - Your Vcs-* headers point to the upstream repository, which does not
> contain a debian/ directory. Vcs-* headers are meant to point at a
> packaging repository. Do you have one available somewhere?
Now that it is available, thanks to gbp, I made them point to this.
> - You have a very long list of quilt patches. Have you considered
> merging some of them? For example, all the -native patches could be a
> single patch. Quilt patches can be a pain to manage when there are
> new upstream releases.
No, perhaps because I don't have any experience with quilt before this
--- so they
are structured the way one would do commits, as logical changes. What usually
goes wrong?
> - I note that you have '#ifdef USE_SYSTEM_FOO' but your patch just
> strips off the whole conditional. Wouldn't it be easier to use those
> USE_SYSTEM_* flags, instead of patching?
Most patches cover things that simply don't have those. The long-term
proper upstream support would indeed involve having more of those, but
that felt needlessly indirect for package patches, since one would basically
have to add a whole bunch of conditionals to all the library .gyp files (making
for some especially hard-to-read diffs, given the .gyp syntax), a
bunch of ifdefs
to the source, and then pass them in as -D flags to gyp from rules. Basically
like 3x the changes to have a path that wouldn't apply anyway.
> - generate.sh runs a copy of gyp in the source tree. But gyp is
> packaged for Debian. Please add a build-dependency on the packaged
> gyp, and run that instead (you might need another patch...)
Working on that, but running in some difficulties, so responding
before resolving this.
I am not sure it's the right choice, though, since that may blow up in
some other way
whenever .gyp is next updated...
> - The package doesn't build in a clean sid chroot. Log attached.
Was missing a couple of build dependencies. Should be fixed in the git version.
sbuild seems like a very helpful tool for getting those right.
Thanks,
Maks
Reply to: