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

David,

david larsson writes:
> Hi,
> the attached patch updates vsftpd so it can use tlsv1.2 etc.

Wow.  Thanks!

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.

Further random comments below:

>  From: methuselah-0 <david.larsson@selfhosted.xyz>
> Date: Tue, 30 Mar 2021 11:18:09 +0200
> Subject: [PATCH] gnu: vsftpd: Use CentOS version and patches.
>
>      * 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.

> +  (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.

> +        (revision "32")

I subjectively added ‘.el8’ here, mainly to factor it out below. 
Neither of us knows what it means, though...

> +           (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.

> +           (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.

> +
> +                   (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.

> +                   (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.

> +                   #t)))

Whilst Guix on master still complains about ‘missing’ #Ts, they 
are a moribund relic and I've secretly started forgetting the odd 
#t on master already...

> +      (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).

Thanks again for your work!

T G-R

Tobias Geerinckx-Rice forgot to write:
> I've also added a copyright line for you.

Kind regards,

T G-R

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

David,

> +      (native-inputs `(("openssl" ,openssl)

Not sure how I missed this -- actually I do, considering the three 
empty champagne bottles now adorning our wall -- but the first 
three should be regular inputs, not native, as they are legitimate 
references of the resulting package ($ guix gc --references).

Native inputs run only during the build.  The distinction matters 
during cross-compilation, when the build-time native-inputs may be 
a different (say, x86_64) architecture from the output package and 
its inputs (both identical: say, aarch64).

> It looks great! Especially nice to see that you separated the 
> patch
> and unpack phases - it looks much better now.

Thank you :-)  Pushed as 634d9845a6b4e362f32ba369ae42851719455ba3.

Kind regards,

T G-R