[bug#47495] [PATCH] gnu: vsftpd: Use CentOS version and patches.

[david larsson <david.larsson@selfhosted.xyz>]

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@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