On 2023-03-30 23:14, Ludovic Courtès wrote:

> Hi,
>
> Timo Wilken <guix@twilken.net> skribis:
>
>> Luarocks is a package manager for Lua modules.
>>
>> It is used by the Prosody XMPP server (already packaged in Guix) to
>> install extensions.
>>
>> * gnu/packages/lua.scm (luarocks): Add variable.
>
> Could resend the two patches merged as one?
>
> Some comments:
>
>> +                      (substitute* '("GNUmakefile" "src/luarocks/fs/unix.lua"
>> +                                     "src/luarocks/core/sysdetect.lua")
>> +                        (("/bin/sh")
>> +                         (string-append (assoc-ref inputs "bash-minimal")
>> +                                        "/bin/sh")))))
>
> Rather (search-input-file inputs "/bin/sh").
>
>> +    (synopsis "A package manager for Lua modules")
>
> I believe ‘guix lint’ will tel you to remove “A”.  :-)
>
>> +    ;; The home page says:
>> +    ;; "LuaRocks is free software and uses the same license as Lua."
>> +    (license license:x11)))
>
> Please double-check the license in the source (and remove the comment).
>
>> +         (add-after 'install 'patch-unzip
>> +           (lambda* (#:key inputs outputs #:allow-other-keys)
>> +             (substitute*
>> +                 (string-append
>> +                  (assoc-ref outputs "out") "/etc/luarocks/config-"
>> +                  ,(substring (package-version lua) 0 3) ".lua") ;e.g. "5.2"
>> +               (("variables = \\{")
>> +                (string-append
>> +                 "variables = {\n"
>> +                 "   AR = \"" (assoc-ref inputs "binutils") "/bin/ar\";\n"
>> +                 "   BUNZIP2 = \"" (assoc-ref inputs "bzip2") "/bin/bunzip2\";\n"
>> +                 "   CC = \"" (assoc-ref inputs "gcc") "/bin/gcc\";\n"
>> +                 "   CHMOD = \"" (assoc-ref inputs "coreutils") "/bin/chmod\";\n"
>> +                 "   CMAKE = \"" (assoc-ref inputs "coreutils") "/bin/cmake\";\n"
>
> These should all use ‘search-input-file’.  However…
>
>> +                 "   CP = \"" (assoc-ref inputs "coreutils") "/bin/cp\";\n"
>> +                 "   CURL = \"" (assoc-ref inputs "curl") "/bin/curl\";\n"
>> +                 "   CVS = \"" (assoc-ref inputs "cvs") "/bin/cvs\";\n"
>> +                 "   FIND = \"" (assoc-ref inputs "findutils") "/bin/find\";\n"
>> +                 "   GIT = \"" (assoc-ref inputs "git") "/bin/git\";\n"
>> +                 "   GPG = \"" (assoc-ref inputs "gnupg") "/bin/gpg\";\n"
>
> [...]
>
>> +                 "   RSYNC = \"" (assoc-ref inputs "rsync") "/bin/rsync\";\n"
>> +                 "   SCP = \"" (assoc-ref inputs "openssh") "/bin/scp\";\n"
>> +                 "   SEVENZ = \"" (assoc-ref inputs "p7zip") "/bin/7z\";\n"
>> +                 "   SVN = \"" (assoc-ref inputs "subversion") "/bin/svn\";\n"
>> +                 "   TAR = \"" (assoc-ref inputs "tar") "/bin/tar\";\n"
>
> Does it really need all these things?  What does ‘guix size luarocks’
> say now?
>
> Maybe it’s OK to assume that some of the rarely-used dependencies (say,
> CVS, SVN, 7zip) will be picked from $PATH and that it will fail
> otherwise?  That would help keep the closure size under control.
>
>> +    (inputs (list lua bash-minimal
>> +                  ;; Executables required by luarocks.
>> +                  binutils bzip2 coreutils curl cvs findutils gcc git gnupg
>> +                  gzip gnu-make mercurial openssh openssl p7zip rsync
>> +                  subversion tar unzip wget zip))
>
> Please one per line since there are many of them (you can run ‘guix
> style luarocks’).
>
> Could you send an updated patch?
>
> Thanks!

Addressed all the comments, pushed as
https://git.savannah.gnu.org/cgit/guix.git/commit/?id=dc8fb56724

-- 
Best regards,
Andrew Tropin