Package: guix-patches;
Reported by: david larsson <david.larsson <at> selfhosted.xyz>
Date: Tue, 30 Mar 2021 07:53:02 UTC
Severity: normal
Tags: patch
Done: Tobias Geerinckx-Rice <me <at> tobias.gr>
Bug is archived. No further changes may be made.
View this message in rfc822 format
From: david larsson <david.larsson <at> selfhosted.xyz> To: Tobias Geerinckx-Rice <me <at> tobias.gr> Cc: 47495 <at> debbugs.gnu.org, guix-patches-bounces+david.larsson=selfhosted.xyz <at> gnu.org Subject: [bug#47495] [PATCH] gnu: vsftpd: Use CentOS version and patches. Date: Tue, 30 Mar 2021 20:38:58 +0200
On 2021-03-30 17:32, Tobias Geerinckx-Rice wrote:
> As indicated on IRC I've made some changes to the patch, mainly to
> avoid hard-coding all patches. The result is attached. Let me know
> what you think.
It looks great! Especially nice to see that you separated the patch and
unpack phases - it looks much better now.
>>
>> * gnu/packages/ftp.scm (vftpd): Use CentOS version and
>> patches.
> ^^^^
>
> This is what happens when you copy commit messages from git and paste
> them right back in :-) In that case, remove the four leading spaces.
Yep, thats what I did :-) will fix next time!
Reg. why to use the significantly patched CentOS variant (asked in your
updated patch's comments): the email passwords thing was a mistake to
mention by me in IRC - that feature was probably already there -
however, the tlsv1.2 was the main reason for switching to the CentOS
version - other features added by the whole patch-set I don't know much
about except from glancing over them and it looks mostly like bug and
security fixes to me.
>
>> + (let ((version "3.0.3")
>
> I renamed this to UPSTREAM-VERSION, so we can show a more specific
> VERSION field in the Guix UI. What we offer isn't ‘3.0.3’ any more.
Ok, I think I understand.
>> + (revision "32")
>
> I subjectively added ‘.el8’ here, mainly to factor it out below.
> Neither of us knows what it means, though...
That is fine with me.
>
>> + (add-after 'unpack 'patch-installation-directory
>> + (lambda* (#:key outputs #:allow-other-keys)
>> + (substitute* "Makefile"
>> + (("/usr") (assoc-ref outputs "out")))
>> + #t))
>
> Moved below the redefined 'unpack phase for clarity.
Great! I had in mind to do the same myself, but didn't due to a
combination of a lack of Guile/Guix coding skills and time.
>> + (replace 'unpack
>> + (lambda* (#:key source #:allow-other-keys)
>> + (let ((version "3.0.3")
>> + (revision "32")
>> + (centos-version "8.3.2011"))
>
> OK, so, as mentioned on IRC this can be avoided by quasiquoting
> <arguments> (as it already was, here) and using ,version instead.
>
> Quoting is probably the most confusing-yet-basic concept in Scheme.
Looks good to me! I am actually quite familiar with unquoting, including
g-exp unquoting things, and I somehow missed that I was in a quasiquote
context from after "arguments"... I intend to improve!
>
>> +
>> + (invoke "7z" "e" source (string-append "-o"
>> "./vsftpd-"
>> + version "-"
>> + revision ".el8.src.cpio"))
>> + (chdir (string-append "./vsftpd-" version "-"
>> + revision ".el8.src.cpio"))
>> + (invoke "cpio" "-idmv" (string-append
>> "--file=./vsftpd-"
>> + version "-"
>> + revision ".el8.src.cpio"))
>> + (invoke "tar" "xvf" (string-append "./vsftpd-"
>> version ".tar.gz"))
>
> This dance had a few steps too many IMO, so I simplified it. It's OK
> to keep the unpacked steps around during the (short) build process;
> they are tiny by today's standards.
Agreed. I was not very happy with this myself. Thanks for fixing!
>
>> + (let ((patches
>
> I understand the reason for this: the patches need to be applied in
> this order, or patching will appear to succeed but result in
> unbuildable source. A simple FIND-FILES is right out.
>
> However, since the order is specified in vsftpd.spec, it's safer,
> shorter, and simply more fun to parse it ourselves.
>
>> + (chdir (string-append "./vsftpd-" version))
>> + (invoke "git" "init" ".")
>> + (invoke "git" "config" "user.email"
>> "you <at> example.com")
>> + (invoke "git" "config" "user.name" "Your Name" )
>> + (invoke "git" "add" ".")
>> + (invoke "git" "commit" "-m" "first")
>> + (map (lambda (x) (invoke "git" "am"
>> (string-append "./" x))) patches)
>> + (map (lambda (x) (invoke "rm" (string-append
>> "./" x))) patches)
>> + (invoke "rm" "-rf" "./.git")
>> + (chdir "../")
>> + (invoke "mv" (string-append "./vsftpd-" version)
>> "../")
>> + (chdir "../")
>> + (invoke "rm" "-rf" (string-append "./vsftpd-"
>> version "-"
>> + revision
>> ".el8.src.cpio"))
>> + (chdir (string-append "./vsftpd-" version)))
>
> You lost me here. Why all the git? I removed all mention of git from
> the package, since it didn't seem necessary, but please correct me if
> needful.
I am, or was, simply unfamiliar with the simplicity of just using
"patch". I tried git am which failed and reported errors that was solved
by the additional git commands. Your replacement is exactly what I need
to learn more about, and looks great, thanks!
>
>> + (native-inputs `(("openssl" ,openssl)
>> + ("linux-pam" ,linux-pam)
>> + ("p7zip" ,p7zip)
>> + ("cpio" ,cpio)
>> + ("git" ,git-minimal)
>> + ("libcap" ,libcap)))
>
> These are *all* new, correct? I removed git and added them all to the
> commit message (check it out).
Yep!
>
> Thanks again for your work!
>
> T G-R
Well..., thank you for your work! You made this patch a lot better! :-)
Best regards,
David Larsson
GNU bug tracking system
Copyright (C) 1999 Darren O. Benham,
1997,2003 nCipher Corporation Ltd,
1994-97 Ian Jackson.