[bug#39717,1/3] gnu: delft-icon-theme: Use the copy-build-system.

* gnu/packages/gnome-xyz.scm (delft-icon-theme): Use the copy-build-system.
---
 gnu/packages/gnome-xyz.scm | 25 ++++++++++++-------------
 1 file changed, 12 insertions(+), 13 deletions(-)

Thanks for the patch!
Comments below:

>      (arguments
> -     `(#:modules ((guix build utils))
> -       #:builder
> -       (begin
> -         (use-modules (guix build utils))
> -         (copy-recursively (assoc-ref %build-inputs "source") "icons")
> -         (substitute* "icons/Delft/index.theme"
> -           (("gnome") "Adwaita"))
> -         (delete-file "icons/README.md")
> -         (delete-file "icons/LICENSE")
> -         (delete-file "icons/logo.jpg")
> -         (copy-recursively "icons" (string-append %output "/share/icons")))))
> +     `(#:install-plan
> +       '(,@(append-map (lambda (file)
> +                         `((,file "share/icons/")
> +                           (,(string-append file "-Dark") "share/icons/")
> +                           (,(string-append file "-Darker") "share/icons/")
> +                           (,(string-append file "-Darkest") "share/icons/")))
> +                       '("Delft" "Delft-Amber" "Delft-Aqua" "Delft-Blue"
> +                         "Delft-Gray" "Delft-Green" "Delft-Mint" "Delft-Purple"
> +                         "Delft-Red" "Delft-Teal")))))

Interesting use of install-plan, but wouldn't it be to copy everything
and simply exclude the previous delete files?  E.g. (untested):

#:install-plan
`(("." "./" #:exclude ("README.md" "LICENSE" "logo.jpg")))

Am Donnerstag, den 27.02.2020, 11:27 +0100 schrieb Pierre Neidhardt:
> Thanks for the patch!
> Comments below:
> 
> >      (arguments
> > -     `(#:modules ((guix build utils))
> > -       #:builder
> > -       (begin
> > -         (use-modules (guix build utils))
> > -         (copy-recursively (assoc-ref %build-inputs "source")
> > "icons")
> > -         (substitute* "icons/Delft/index.theme"
> > -           (("gnome") "Adwaita"))
> > -         (delete-file "icons/README.md")
> > -         (delete-file "icons/LICENSE")
> > -         (delete-file "icons/logo.jpg")
> > -         (copy-recursively "icons" (string-append %output
> > "/share/icons")))))
> > +     `(#:install-plan
> > +       '(,@(append-map (lambda (file)
> > +                         `((,file "share/icons/")
> > +                           (,(string-append file "-Dark")
> > "share/icons/")
> > +                           (,(string-append file "-Darker")
> > "share/icons/")
> > +                           (,(string-append file "-Darkest")
> > "share/icons/")))
> > +                       '("Delft" "Delft-Amber" "Delft-Aqua"
> > "Delft-Blue"
> > +                         "Delft-Gray" "Delft-Green" "Delft-Mint"
> > "Delft-Purple"
> > +                         "Delft-Red" "Delft-Teal")))))
> 
> Interesting use of install-plan, but wouldn't it be to copy
> everything
> and simply exclude the previous delete files?  E.g. (untested):
> 
> #:install-plan
> `(("." "./" #:exclude ("README.md" "LICENSE" "logo.jpg")))
It would indeed be nice if it worked that way.  However, Delft makes
heavy use of symbolic links, of which some are even dead, and that
causes install-file to fail.  
I tried patching copy-build-system, but the result was not usable,
probably because symlinks were not resolved correctly.  On top of that,
I don't think putting that much more work into copy-build-system is a
good idea.  copy-build-system should be a simple build-system that just
copies stuff.  Perhaps we can swallow the "file does not exist" errors,
but even that seems kinda wrong to me.
Anyways, since the original used copy-recursively, the only thing I had
to do was to make copy-build-system resort to copy-recursively as well,
hence this interesting use of install-plan.  Alternatively, one could
delete the files in a pre-install phase, but that seems even weirder to
me. 

Regards,
Leo

Am Donnerstag, den 27.02.2020, 11:27 +0100 schrieb Pierre Neidhardt:
> Thanks for the patch!
> Comments below:
> 
> >      (arguments
> > -     `(#:modules ((guix build utils))
> > -       #:builder
> > -       (begin
> > -         (use-modules (guix build utils))
> > -         (copy-recursively (assoc-ref %build-inputs "source")
> > "icons")
> > -         (substitute* "icons/Delft/index.theme"
> > -           (("gnome") "Adwaita"))
> > -         (delete-file "icons/README.md")
> > -         (delete-file "icons/LICENSE")
> > -         (delete-file "icons/logo.jpg")
> > -         (copy-recursively "icons" (string-append %output
> > "/share/icons")))))
> > +     `(#:install-plan
> > +       '(,@(append-map (lambda (file)
> > +                         `((,file "share/icons/")
> > +                           (,(string-append file "-Dark")
> > "share/icons/")
> > +                           (,(string-append file "-Darker")
> > "share/icons/")
> > +                           (,(string-append file "-Darkest")
> > "share/icons/")))
> > +                       '("Delft" "Delft-Amber" "Delft-Aqua"
> > "Delft-Blue"
> > +                         "Delft-Gray" "Delft-Green" "Delft-Mint"
> > "Delft-Purple"
> > +                         "Delft-Red" "Delft-Teal")))))
> 
> Interesting use of install-plan, but wouldn't it be to copy
> everything
> and simply exclude the previous delete files?  E.g. (untested):
> 
> #:install-plan
> `(("." "./" #:exclude ("README.md" "LICENSE" "logo.jpg")))
It would indeed be nice if it worked that way.  However, Delft makes
heavy use of symbolic links, of which some are even dead, and that
causes install-file to fail.  
I tried patching copy-build-system, but the result was not usable,
probably because symlinks were not resolved correctly.  On top of that,
I don't think putting that much more work into copy-build-system is a
good idea.  copy-build-system should be a simple build-system that just
copies stuff.  Perhaps we can swallow the "file does not exist" errors,
but even that seems kinda wrong to me.
Anyways, since the original used copy-recursively, the only thing I had
to do was to make copy-build-system resort to copy-recursively as well,
hence this interesting use of install-plan.  Alternatively, one could
delete the files in a pre-install phase, but that seems even weirder to
me. 

Regards,
Leo

Can you detail what does not work?  Which file, what error message?

In my opinion, the install-plan should be able to install any file,
symlink or not.  Failing to do so seems like a bug to me.

Am Donnerstag, den 27.02.2020, 14:19 +0100 schrieb Pierre Neidhardt:
> Can you detail what does not work?  Which file, what error message?
$ find `guix build --source delft-icon-theme` -xtype l
This returns a list of broken symlinks.  On any of them, install-file
fails with "file not found".

> In my opinion, the install-plan should be able to install any file,
> symlink or not.  Failing to do so seems like a bug to me.
I think I agree with you.  Instead of simply doing (copy-file file
dest), we'd have to check for symlinks as is done in copy-recursively. 
Now that I have a more complete understanding of this bug, I'll try to
come up with a patch.

Regards,
Leo

Thanks, and good catch!