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

[david larsson]

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