Thank you for your review!  I have attached updated patches.

I have some questions.  I’d like to answer but not in order.  First of
all,

On Fri, Nov 01, 2019 at 03:54:42PM +0100, Ludovic Courtès wrote:
> > +         (modules '((guix build utils)
> > +                    (ice-9 popen)))
> > +         (snippet
> > +          #~(begin
> > +              ;; the nginx source code is part of the module’s source
> > +              (format #t "decompressing nginx source code~%")
> > +              (call-with-output-file "nginx.tar"
> > +                (lambda (out)
> > +                  (let ((pipe (open-pipe* OPEN_READ
> > +                                          #+(file-append gzip "/bin/gzip") "-cd"
> > +                                          #$(package-source nginx))))
> > +                    (dump-port pipe out)
> > +                    (unless (= (status:exit-val (close-pipe pipe)) 0)
> > +                      (error "gzip decompress failed")))))
> > +              (invoke #+(file-append tar "/bin/tar") "xvf" "nginx.tar"
> > +                      "--strip-components=1")
> > +              (delete-file "nginx.tar")
> 
> I’d suggest doing it in a phase.

This changes many things. :)

With a phase, `guix build -S` would only return the source files of
nginx-accept-language-module directly but not of statically linked
nginx.  I have added a further patch to doc/guix.texi warning of `guix
build -S` not always returning complete and corresponding sources, see
attachment.

The good thing is that with a phase I no longer get an import cycle
because I no longer need a reference to the tar package.  So I put
nginx-accept-language-module inside web.scm now and there is no need
for a separate module.


> > +      (license (delete-duplicates
> > +                (cons license:bsd-2 ;license of nginx-mod-accept-language
> > +                      (package-license nginx))))))) ;the module’s code is linked
> 
> To avoid circular dependencies in top-level references, I suggest
> copying the license of ‘nginx’ instead of writing (package-license
> nginx).
> 

I believe this is no longer an issue now that
nginx-accept-language-module is in the same Guile module as nginx, is
it?


> > +++ b/gnu/packages/web-xyz.scm
> > @@ -0,0 +1,175 @@
> > +;;; GNU Guix --- Functional package management for GNU
> > +;;;; TODO should I really add copyright lines for people I copied from??
> > +;;; Copyright © 2014, 2015 Mark H Weaver <mhw@netris.org>
> > +;;; Copyright © 2016 Tobias Geerinckx-Rice <me@tobias.gr>
> > +;;; Copyright © 2017, 2018 Marius Bakke <mbakke@fastmail.com>
> 
> I don’t think you need to add these 3 lines here; the package definition
> is yours.
> 

This does not matter anymore after putting
nginx-accept-language-module in the same Guile module file as nginx.

In general though: IANAL but I have largely copied nginx’ configure
phase, so at least Mark would surely have copyright on parts of it.
However I believe copyright lines have limited legal significance
anyway and adding these authorship lines in a file not added by Mark
seems unreasonable.


> […]
> Perhaps “nginx-accept-language-module”, to match the name of the
> upstream repo?
> 

I agree.  Arch (who have no package for nginx-accept-language-module)
change their various nginx module package names to be more consistent,
I think, but this is not necessary in Guix, I think.


On Fri, Nov 01, 2019 at 03:58:43PM +0100, Ludovic Courtès wrote:
> Regarding the build system of nginx modules, we’ll see when
> we have more than one module packaged.  :-)
> 

Good. This module is not typical and writing a build system would be
difficult now.

Regards,
Florian