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

Previous Next

To add a comment to this bug, you must first unarchive it, by sending
a message to control AT debbugs.gnu.org, with unarchive 39717 in the body.
You can then email your comments to 39717 AT debbugs.gnu.org in the normal way.

Toggle the display of automated, internal messages from the tracker.

Message #5 received at submit <at> debbugs.gnu.org (full text, mbox):

From: Leo Prikler <leo.prikler <at> student.tugraz.at>
To: guix-patches <at> gnu.org
Subject: [PATCH 1/3] gnu: delft-icon-theme: Use the copy-build-system.
Date: Fri, 21 Feb 2020 17:32:13 +0100

* 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(-)

diff --git a/gnu/packages/gnome-xyz.scm b/gnu/packages/gnome-xyz.scm
index 06af41aea6..882bd40c2e 100644
--- a/gnu/packages/gnome-xyz.scm
+++ b/gnu/packages/gnome-xyz.scm
@@ -33,7 +33,8 @@
   #:use-module (gnu packages gtk)
   #:use-module (gnu packages pkg-config)
   #:use-module (gnu packages ruby)
-  #:use-module (gnu packages xml))
+  #:use-module (gnu packages xml)
+  #:use-module (srfi srfi-1))
 
 (define-public matcha-theme
   (package
@@ -95,19 +96,17 @@ like Gnome, Unity, Budgie, Pantheon, XFCE, Mate and others.")
         (base32
          "0vw3yw9f9ygzfd2k3zrfih3r0vkzlhk1bmsk8sapvk7np24i1z9s"))
        (file-name (git-file-name name version))))
-    (build-system trivial-build-system)
+    (build-system copy-build-system)
     (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")))))
     (home-page "https://www.gnome-look.org/p/1199881/")
     (synopsis "Continuation of Faenza icon theme with up to date app icons")
     (description "Delft is a fork of the popular icon theme Faenza with up to
-- 
2.25.1

Message #8 received at 39717 <at> debbugs.gnu.org (full text, mbox):

From: Pierre Neidhardt <mail <at> ambrevar.xyz>
To: Leo Prikler <leo.prikler <at> student.tugraz.at>
Cc: 39717 <at> debbugs.gnu.org
Subject: Re: [bug#39717] [PATCH 1/3] gnu: delft-icon-theme: Use the
 copy-build-system.
Date: Thu, 27 Feb 2020 11:27:49 +0100

[Message part 1 (text/plain, inline)]

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")))

-- 
Pierre Neidhardt
https://ambrevar.xyz/

[signature.asc (application/pgp-signature, inline)]

Message #11 received at 39717 <at> debbugs.gnu.org (full text, mbox):

From: Leo Prikler <leo.prikler <at> student.tugraz.at>
To: Pierre Neidhardt <mail <at> ambrevar.xyz>
Cc: 39717 <at> debbugs.gnu.org
Subject: Re: [bug#39717] [PATCH 1/3] gnu: delft-icon-theme: Use the
 copy-build-system.
Date: Thu, 27 Feb 2020 11:52:50 +0100

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

Message #14 received at 39717 <at> debbugs.gnu.org (full text, mbox):

From: Leo Prikler <leo.prikler <at> student.tugraz.at>
To: Pierre Neidhardt <mail <at> ambrevar.xyz>
Cc: 39717 <at> debbugs.gnu.org
Subject: Re: [bug#39717] [PATCH 1/3] gnu: delft-icon-theme: Use the
 copy-build-system.
Date: Thu, 27 Feb 2020 11:52:50 +0100

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

Message #17 received at 39717 <at> debbugs.gnu.org (full text, mbox):

From: Pierre Neidhardt <mail <at> ambrevar.xyz>
To: Leo Prikler <leo.prikler <at> student.tugraz.at>
Cc: 39717 <at> debbugs.gnu.org
Subject: Re: [bug#39717] [PATCH 1/3] gnu: delft-icon-theme: Use the
 copy-build-system.
Date: Thu, 27 Feb 2020 14:19:53 +0100

[Message part 1 (text/plain, inline)]

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.

-- 
Pierre Neidhardt
https://ambrevar.xyz/

[signature.asc (application/pgp-signature, inline)]

Message #20 received at 39717 <at> debbugs.gnu.org (full text, mbox):

From: Leo Prikler <leo.prikler <at> student.tugraz.at>
To: Pierre Neidhardt <mail <at> ambrevar.xyz>
Cc: 39717 <at> debbugs.gnu.org
Subject: Re: [bug#39717] [PATCH 1/3] gnu: delft-icon-theme: Use the
 copy-build-system.
Date: Thu, 27 Feb 2020 14:44:45 +0100

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

Message #23 received at 39717 <at> debbugs.gnu.org (full text, mbox):

From: Pierre Neidhardt <mail <at> ambrevar.xyz>
To: Leo Prikler <leo.prikler <at> student.tugraz.at>
Cc: 39717 <at> debbugs.gnu.org
Subject: Re: [bug#39717] [PATCH 1/3] gnu: delft-icon-theme: Use the
 copy-build-system.
Date: Thu, 27 Feb 2020 15:02:50 +0100

[Message part 1 (text/plain, inline)]

Thanks, and good catch!

-- 
Pierre Neidhardt
https://ambrevar.xyz/

[signature.asc (application/pgp-signature, inline)]

Message #26 received at 39717 <at> debbugs.gnu.org (full text, mbox):

From: Leo Prikler <leo.prikler <at> student.tugraz.at>
To: Pierre Neidhardt <mail <at> ambrevar.xyz>
Cc: 39717 <at> debbugs.gnu.org
Subject: Re: [bug#39717] [PATCH 1/3] gnu: delft-icon-theme: Use the
 copy-build-system.
Date: Thu, 27 Feb 2020 15:49:08 +0100

[Message part 1 (text/plain, inline)]

Am Donnerstag, den 27.02.2020, 15:02 +0100 schrieb Pierre Neidhardt:
> Thanks, and good catch!
Patch is ready.  I also noticed some inconsistencies with my previous
definition and updated the package to 1.11.

Regards,
Leo

[0001-build-system-copy-build-system-Keep-symlinks-symboli.patch (text/x-patch, attachment)]

[0002-gnu-delft-icon-theme-Use-the-copy-build-system.patch (text/x-patch, attachment)]

[0003-gnu-delft-icon-theme-Update-to-1.11.patch (text/x-patch, attachment)]

Message #29 received at 39717 <at> debbugs.gnu.org (full text, mbox):

From: Pierre Neidhardt <mail <at> ambrevar.xyz>
To: Leo Prikler <leo.prikler <at> student.tugraz.at>
Cc: 39717 <at> debbugs.gnu.org
Subject: Re: [bug#39717] [PATCH 1/3] gnu: delft-icon-theme: Use the
 copy-build-system.
Date: Fri, 28 Feb 2020 09:40:44 +0100

[Message part 1 (text/plain, inline)]

Merged, thanks!

-- 
Pierre Neidhardt
https://ambrevar.xyz/

[signature.asc (application/pgp-signature, inline)]

Previous Next

GNU bug tracking system
Copyright (C) 1999 Darren O. Benham, 1997,2003 nCipher Corporation Ltd, 1994-97 Ian Jackson.