[PATCH 1/3] gnu: delft-icon-theme: Use the copy-build-system.

Done

Details

2 participants

Leo Prikler
Pierre Neidhardt

Owner: unassigned

Submitted by: Leo Prikler

Severity: normal

Debbugs page

Leo Prikler wrote 5 years ago

Recipients:(address . guix-patches@gnu.org)

Message-ID:20200221163215.11008-1-leo.prikler@student.tugraz.at

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

Toggle diff (46 lines)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

Pierre Neidhardt wrote 5 years ago

Recipients:(name . Leo Prikler)(address . leo.prikler@student.tugraz.at)(address . 39717@debbugs.gnu.org)

Message-ID:87mu9472ai.fsf@ambrevar.xyz

Thanks for the patch!

Comments below:

Toggle quote (22 lines)>      (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/

Leo Prikler wrote 5 years ago

Recipients:(name . Pierre Neidhardt)(address . mail@ambrevar.xyz)(address . 39717@debbugs.gnu.org)

Message-ID:635f276d8ce7198de37c5c95387dc656ee80f4ae.camel@student.tugraz.at

Am Donnerstag, den 27.02.2020, 11:27 +0100 schrieb Pierre Neidhardt:

Toggle quote (38 lines)> 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

Leo Prikler wrote 5 years ago

Recipients:(name . Pierre Neidhardt)(address . mail@ambrevar.xyz)(address . 39717@debbugs.gnu.org)

Message-ID:755600da3a603ae885693218b14ee538f2d8295a.camel@student.tugraz.at

Am Donnerstag, den 27.02.2020, 11:27 +0100 schrieb Pierre Neidhardt:

Toggle quote (38 lines)> 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

Pierre Neidhardt wrote 5 years ago

Recipients:(name . Leo Prikler)(address . leo.prikler@student.tugraz.at)(address . 39717@debbugs.gnu.org)

Message-ID:875zfs6ubq.fsf@ambrevar.xyz

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/

Leo Prikler wrote 5 years ago

Recipients:(name . Pierre Neidhardt)(address . mail@ambrevar.xyz)(address . 39717@debbugs.gnu.org)

Message-ID:b50a5c21c5db04d0b9160db2b6ebdbcba50a4114.camel@student.tugraz.at

Am Donnerstag, den 27.02.2020, 14:19 +0100 schrieb Pierre Neidhardt:

Toggle quote (1 lines)

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

Toggle quote (2 lines)

> 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

Pierre Neidhardt wrote 5 years ago

Recipients:(name . Leo Prikler)(address . leo.prikler@student.tugraz.at)(address . 39717@debbugs.gnu.org)

Message-ID:87zhd45drp.fsf@ambrevar.xyz

Thanks, and good catch!

Pierre Neidhardt

https://ambrevar.xyz/

Leo Prikler wrote 5 years ago

Recipients:(name . Pierre Neidhardt)(address . mail@ambrevar.xyz)(address . 39717@debbugs.gnu.org)

Message-ID:0047ad8dbdc65cfab78694c5be996d99ebb734c3.camel@student.tugraz.at

Am Donnerstag, den 27.02.2020, 15:02 +0100 schrieb Pierre Neidhardt:

Toggle quote (1 lines)

> 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

From 7147b298949e17b3b3af857d91437f9012149297 Mon Sep 17 00:00:00 2001
From: Leo Prikler <leo.prikler@student.tugraz.at>
Date: Thu, 27 Feb 2020 14:49:11 +0100
Subject: [PATCH 1/2] build-system: copy-build-system: Keep symlinks symbolic.

guix/build/copy-build-system.scm (install)[install-file]:
Read symlinks as is done in install-simple through copy-recursively.
---
 guix/build/copy-build-system.scm | 8 +++++++-
 1 file changed, 7 insertions(+), 1 deletion(-)

Toggle diff (21 lines)diff --git a/guix/build/copy-build-system.scm b/guix/build/copy-build-system.scm
index 6d9dc8f93b..a86f0cde29 100644
--- a/guix/build/copy-build-system.scm
+++ b/guix/build/copy-build-system.scm
@@ -91,7 +91,13 @@ if TARGET ends with a '/', the source is installed underneath."
                                    file))))
       (format (current-output-port) "`~a' -> `~a'~%" file dest)
       (mkdir-p (dirname dest))
-      (copy-file file dest)))
+      (let ((stat (lstat file)))
+        (case (stat:type stat)
+          ((symlink)
+           (let ((target (readlink file)))
+             (symlink target dest)))
+          (else
+           (copy-file file dest))))))
 
   (define* (make-file-predicate suffixes matches-regexp #:optional (default-value #t))
     "Return a predicate that returns #t if its file argument matches the
-- 
2.25.1

From d51c90a241452ba03f9d2b5038233967bb94e844 Mon Sep 17 00:00:00 2001

* gnu/packages/gnome-xyz.scm (delft-icon-theme): Use the copy-build-system.

---

gnu/packages/gnome-xyz.scm | 22 ++++++++++------------

1 file changed, 10 insertions(+), 12 deletions(-)

Toggle diff (36 lines)diff --git a/gnu/packages/gnome-xyz.scm b/gnu/packages/gnome-xyz.scm
index bedaacf092..b0bc35c4b1 100644
--- a/gnu/packages/gnome-xyz.scm
+++ b/gnu/packages/gnome-xyz.scm
@@ -101,19 +101,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
+       `(("." "share/icons" #:exclude ("README.md" "LICENSE" "logo.jpg")))
+       #:phases
+       (modify-phases %standard-phases
+         (add-after 'unpack 'patch-index.theme
+           (lambda _
+            (substitute* "Delft/index.theme"
+              (("gnome") "Adwaita"))
+            #t)))))
     (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

From c90ab439c7c79350fa783a9f20182a34fea95289 Mon Sep 17 00:00:00 2001

* gnu/packages/gnome-xyz.scm (delft-icon-theme): Update to 1.11.

---

gnu/packages/gnome-xyz.scm | 4 ++--

1 file changed, 2 insertions(+), 2 deletions(-)

Toggle diff (24 lines)diff --git a/gnu/packages/gnome-xyz.scm b/gnu/packages/gnome-xyz.scm
index b0bc35c4b1..b8cd32db08 100644
--- a/gnu/packages/gnome-xyz.scm
+++ b/gnu/packages/gnome-xyz.scm
@@ -90,7 +90,7 @@ like Gnome, Unity, Budgie, Pantheon, XFCE, Mate and others.")
 (define-public delft-icon-theme
   (package
     (name "delft-icon-theme")
-    (version "1.10")
+    (version "1.11")
     (source
      (origin
        (method git-fetch)
@@ -99,7 +99,7 @@ like Gnome, Unity, Budgie, Pantheon, XFCE, Mate and others.")
              (commit (string-append "v" version))))
        (sha256
         (base32
-         "0vw3yw9f9ygzfd2k3zrfih3r0vkzlhk1bmsk8sapvk7np24i1z9s"))
+         "1m3r4i4m3y3xsjb5f4bik0ylmi64amkfyr0y8pjbvv6gyj492mi6"))
        (file-name (git-file-name name version))))
     (build-system copy-build-system)
     (arguments
-- 
2.25.1

Pierre Neidhardt wrote 5 years ago

Recipients:(name . Leo Prikler)(address . leo.prikler@student.tugraz.at)(address . 39717@debbugs.gnu.org)

Message-ID:875zfr5ckz.fsf@ambrevar.xyz

Merged, thanks!

Pierre Neidhardt

https://ambrevar.xyz/

Pierre Neidhardt wrote 5 years ago

control message for bug #39717

Recipients:(address . control@debbugs.gnu.org)

Message-ID:874kvb5ckc.fsf@ambrevar.xyz

close 39717

quit

Your comment

This issue is archived.

To comment on this conversation send an email to 39717@debbugs.gnu.org

To respond to this issue using the mumi CLI, first switch to it

mumi current 39717

Then, you may apply the latest patchset in this issue (with sign off)

mumi am -- -s

Or, compose a reply to this issue

mumi compose

Or, send patches to this issue

mumi send-email *.patch

You may also tag this issue. See list of standard tags. For example, to set the confirmed and easy tags

mumi command -t +confirmed -t +easy

Or, remove the moreinfo tag and set the help tag

mumi command -t -moreinfo -t +help

is:open	open issues
is:done	closed issues
submitter:<who>	search issue submitter
author:<who>	search by message author
date:yesterday..now	search by issue date
mdate:3m..2d	search by message date