[bug#32102] [PATCH] utils: Fix wrap-program filename generation.

[bug#32102] [PATCH] utils: Fix wrap-program filename generation.

[Arun Isaac]

* guix/build/utils.scm (wrap-program): While generating a new filename for the
wrapped program, trim dots from the left of the basename. This prevents
already wrapped files being wrapped again with two or more dots prepended to
them.
---
 guix/build/utils.scm | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/guix/build/utils.scm b/guix/build/utils.scm
index c58a1afd1..0794c658f 100644
--- a/guix/build/utils.scm
+++ b/guix/build/utils.scm
@@ -3,6 +3,7 @@
 ;;; Copyright © 2013 Andreas Enge <andreas@enge.fr>
 ;;; Copyright © 2013 Nikita Karetnikov <nikita@karetnikov.org>
 ;;; Copyright © 2015, 2018 Mark H Weaver <mhw@netris.org>
+;;; Copyright © 2018 Arun Isaac <arunisaac@systemreboot.net>
 ;;;
 ;;; This file is part of GNU Guix.
 ;;;
@@ -1032,7 +1033,8 @@ modules in $GUILE_LOAD_PATH, etc.
 If PROG has previously been wrapped by 'wrap-program', the wrapper is extended
 with definitions for VARS."
   (define wrapped-file
-    (string-append (dirname prog) "/." (basename prog) "-real"))
+    (string-append
+     (dirname prog) "/." (string-trim (basename prog) #\.) "-real"))
 
   (define already-wrapped?
     (file-exists? wrapped-file))
-- 
2.15.1

[bug#32102] [PATCH] utils: Fix wrap-program filename generation.

[Clément Lassieur]

Hi Arun,

Arun Isaac <arunisaac@systemreboot.net> writes:

> * guix/build/utils.scm (wrap-program): While generating a new filename for the
> wrapped program, trim dots from the left of the basename. This prevents
> already wrapped files being wrapped again with two or more dots prepended to
> them.

Why is it a problem that two or more dots are prepended to them?

It means that 'foo' and '.foo' will have the same generated file name
'.foo-real'.  Wrapping a hidden file and its non-hidden counterpart is
something that probably shouldn't happen, but if for some reason it must
happen, we'll face an annoying bug.  WDYT?

Clément

[bug#32102] [PATCH] utils: Fix wrap-program filename generation.

[Arun Isaac]

>> * guix/build/utils.scm (wrap-program): While generating a new filename for the
>> wrapped program, trim dots from the left of the basename. This prevents
>> already wrapped files being wrapped again with two or more dots prepended to
>> them.
>
> Why is it a problem that two or more dots are prepended to them?

(define (wrap-program prog #:rest vars)
  (define wrapped-file
    (string-append
     (dirname prog) "/." (basename prog) "-real"))

  (define already-wrapped?
    (file-exists? wrapped-file))

...)

If wrap-program finds that PROG has previously been wrapped, it extends
the wrapper; it does not create a wrapper around the previously existing
wrapper (a "double wrapper"). wrap-program detects that PROG has
previously been wrapped by comparing the expected wrapped filename (see
code snippet above). Without the string-trim I added, this
already-wrapped? detection fails and a double wrapper ends up being
created.

For a concrete example of what I mean, look at the gajim package. You
will find double wrappers such as "bin/..gajim-real-real". This
shouldn't have happened. Furthermore, many other packages have similar
issues. You might find some if you search through /gnu/store.

find /gnu/store -name "*-real-real"

> It means that 'foo' and '.foo' will have the same generated file name
> '.foo-real'.  Wrapping a hidden file and its non-hidden counterpart is
> something that probably shouldn't happen, but if for some reason it must
> happen, we'll face an annoying bug.  WDYT?

It's true that we could face an annoying bug in the future. But then,
we'll have to find some alternative means to determine
alread-wrapped?. Is that worth it? Any ideas?

[bug#32102] [PATCH] utils: Fix wrap-program filename generation.

[Clément Lassieur]

Arun Isaac <arunisaac@systemreboot.net> writes:

>>> * guix/build/utils.scm (wrap-program): While generating a new filename for the
>>> wrapped program, trim dots from the left of the basename. This prevents
>>> already wrapped files being wrapped again with two or more dots prepended to
>>> them.
>>
>> Why is it a problem that two or more dots are prepended to them?
>
> (define (wrap-program prog #:rest vars)
>   (define wrapped-file
>     (string-append
>      (dirname prog) "/." (basename prog) "-real"))
>
>   (define already-wrapped?
>     (file-exists? wrapped-file))
>
> ...)
>
> If wrap-program finds that PROG has previously been wrapped, it extends
> the wrapper; it does not create a wrapper around the previously existing
> wrapper (a "double wrapper"). wrap-program detects that PROG has
> previously been wrapped by comparing the expected wrapped filename (see
> code snippet above). Without the string-trim I added, this
> already-wrapped? detection fails and a double wrapper ends up being
> created.

If '.gajim-real' exists and (WRAP-PROGRAM '/path/to/gajim' ...) is
called, PROG is '/path/to/gajim', WRAPPED-FILE is '/path/to/.gajim-real'
and ALREADY-WRAPPED? is #t, so I don't think there is a bug with
WRAP-PROGRAM.

The ..gajim-real-real file comes from the WRAP procedure in
python-build-system.scm: that WRAP procedure wraps every file it finds.
It'll wrap '.gajim-real' and 'gajim'.  Wrapping 'gajim' will work well,
it will be modified because it's already a wrapper, i.e. '.gajim-real'
exists.  But wrapping '.gajim-real' will create '..gajim-real-real'
because '.gajim-real' is not a wrapper.  And I think it's normal too.

So the question is: should WRAP (from python-build-system.scm) wrap
files that already have a wrapper?  I think it shouldn't.

Clément

[bug#32102] [PATCH] utils: Fix wrap-program filename generation.

[Arun Isaac]

> should WRAP (from python-build-system.scm) wrap files that already
> have a wrapper?  I think it shouldn't.

I agree with your analysis. How about I send a patch modifying WRAP
(from python-build-system) to only wrap non-hidden files (files whose
name do not begin with a dot) in bin and sbin?

[bug#32102] [PATCH] utils: Fix wrap-program filename generation.

[Clément Lassieur]

Arun Isaac <arunisaac@systemreboot.net> writes:

>> should WRAP (from python-build-system.scm) wrap files that already
>> have a wrapper?  I think it shouldn't.
>
> I agree with your analysis. How about I send a patch modifying WRAP
> (from python-build-system) to only wrap non-hidden files (files whose
> name do not begin with a dot) in bin and sbin?

That sounds good, but I'm not a Python expert, and I don't know if there
are cases where hidden files would need to be wrapped.  I'm CCing
Andreas and Mark because they know better than me.

[bug#32102] [PATCH] utils: Fix wrap-program filename generation.

[Mark H Weaver]

Clément Lassieur <clement@lassieur.org> writes:

> Arun Isaac <arunisaac@systemreboot.net> writes:
>
>>> should WRAP (from python-build-system.scm) wrap files that already
>>> have a wrapper?  I think it shouldn't.
>>
>> I agree with your analysis. How about I send a patch modifying WRAP
>> (from python-build-system) to only wrap non-hidden files (files whose
>> name do not begin with a dot) in bin and sbin?

Sure, sounds reasonable to me.

> That sounds good, but I'm not a Python expert, and I don't know if there
> are cases where hidden files would need to be wrapped.  I'm CCing
> Andreas and Mark because they know better than me.

The change sounds good, but I think it can't be done on master because
it would entail too many rebuilds.  I can't tell you roughly how many,
because as far as I know we don't have tooling to estimate the number of
rebuilds from changing a build system, but I tried adding whitespace to
python-build-system.scm and then running "guix system build -n <config>"
on my GNOME system and the answer was discouraging.

So, I think this is probably a change for core-updates, or possibly for
the next 'staging' cycle if we can come up with a better estimate of how
many builds are needed.

     Thanks,
       Mark

[bug#32102] [PATCH v2 0/2] build-system: python: Only wrap non-hidden executable files

[Arun Isaac]

Please find below the new patch. In addition, I have included another minor
slightly related patch for the gajim package.

Arun Isaac (2):
  build-system: python: Only wrap non-hidden executable files.
  gnu: gajim: Combine wrap-program phases.

 gnu/packages/messaging.scm         | 35 ++++++++++++++++-------------------
 guix/build/python-build-system.scm |  7 ++++++-
 2 files changed, 22 insertions(+), 20 deletions(-)

-- 
2.15.1

[bug#32102] [PATCH v2 1/2] build-system: python: Only wrap non-hidden executable files.

[Arun Isaac]

* guix/build/python-build-system.scm (wrap): Only wrap non-hidden executable
files. This prevents wrapped executables from being wrapped once again.
---
 guix/build/python-build-system.scm | 7 ++++++-
 1 file changed, 6 insertions(+), 1 deletion(-)

diff --git a/guix/build/python-build-system.scm b/guix/build/python-build-system.scm
index 376ea81f1..37e35eec0 100644
--- a/guix/build/python-build-system.scm
+++ b/guix/build/python-build-system.scm
@@ -5,6 +5,7 @@
 ;;; Copyright © 2015, 2018 Mark H Weaver <mhw@netris.org>
 ;;; Copyright © 2016 Hartmut Goebel <h.goebel@crazy-compilers.com>
 ;;; Copyright © 2018 Ricardo Wurmus <rekado@elephly.net>
+;;; Copyright © 2018 Arun Isaac <arunisaac@systemreboot.net>
 ;;;
 ;;; This file is part of GNU Guix.
 ;;;
@@ -189,7 +190,11 @@ when running checks after installing the package."
     (map (cut string-append dir "/" <>)
          (or (scandir dir (lambda (f)
                             (let ((s (stat (string-append dir "/" f))))
-                              (eq? 'regular (stat:type s)))))
+                              (and (eq? 'regular (stat:type s))
+                                   ;; Only wrap non-hidden files. This
+                                   ;; prevents wrapped executables from being
+                                   ;; wrapped again.
+                                   (not (string-prefix? "." f))))))
              '())))
 
   (define bindirs
-- 
2.15.1

[bug#32102] [PATCH v2 1/2] build-system: python: Only wrap non-hidden executable files.

[Clément Lassieur]

Arun Isaac <arunisaac@systemreboot.net> writes:

> * guix/build/python-build-system.scm (wrap): Only wrap non-hidden executable
> files. This prevents wrapped executables from being wrapped once again.
> ---
>  guix/build/python-build-system.scm | 7 ++++++-
>  1 file changed, 6 insertions(+), 1 deletion(-)
>
> diff --git a/guix/build/python-build-system.scm b/guix/build/python-build-system.scm
> index 376ea81f1..37e35eec0 100644
> --- a/guix/build/python-build-system.scm
> +++ b/guix/build/python-build-system.scm
> @@ -5,6 +5,7 @@
>  ;;; Copyright © 2015, 2018 Mark H Weaver <mhw@netris.org>
>  ;;; Copyright © 2016 Hartmut Goebel <h.goebel@crazy-compilers.com>
>  ;;; Copyright © 2018 Ricardo Wurmus <rekado@elephly.net>
> +;;; Copyright © 2018 Arun Isaac <arunisaac@systemreboot.net>
>  ;;;
>  ;;; This file is part of GNU Guix.
>  ;;;
> @@ -189,7 +190,11 @@ when running checks after installing the package."
>      (map (cut string-append dir "/" <>)
>           (or (scandir dir (lambda (f)
>                              (let ((s (stat (string-append dir "/" f))))
> -                              (eq? 'regular (stat:type s)))))
> +                              (and (eq? 'regular (stat:type s))
> +                                   ;; Only wrap non-hidden files. This
> +                                   ;; prevents wrapped executables from being
> +                                   ;; wrapped again.
> +                                   (not (string-prefix? "." f))))))
>               '())))
>  
>    (define bindirs

What about adding a function like 'is-wrapped?'?  Because it could be
used somewhere else, and it would make the code self-descriptive.

It would check that the name looks like .xxx-real, and also check that
xxx exists.  (And check that it's an executable.)

WDYT?
Clément

[bug#32102] [PATCH v2 1/2] build-system: python: Only wrap non-hidden executable files.

[Arun Isaac]

> What about adding a function like 'is-wrapped?'?  Because it could be
> used somewhere else, and it would make the code self-descriptive.
>
> It would check that the name looks like .xxx-real, and also check that
> xxx exists.  (And check that it's an executable.)

Yes, that's a good idea. I'll add `is-wrapped?' to (guix build utils)
and export it. That way, both `wrap-program' from (guix build utils) and
the wrap phase from python-build-system can make use of it. I will send
an updated patchset once I'm done.

[bug#32102] [PATCH v2 2/2] gnu: gajim: Combine wrap-program phases.

[Arun Isaac]

* gnu/packages/messaging.scm (gajim)[arguments]: Combine wrap-program phases
into a single wrap-gajim phase.
---
 gnu/packages/messaging.scm | 35 ++++++++++++++++-------------------
 1 file changed, 16 insertions(+), 19 deletions(-)

diff --git a/gnu/packages/messaging.scm b/gnu/packages/messaging.scm
index cdcd1225f..89946a685 100644
--- a/gnu/packages/messaging.scm
+++ b/gnu/packages/messaging.scm
@@ -9,7 +9,7 @@
 ;;; Copyright © 2016 Andy Patterson <ajpatter@uwaterloo.ca>
 ;;; Copyright © 2016, 2017, 2018 Clément Lassieur <clement@lassieur.org>
 ;;; Copyright © 2017 Mekeor Melire <mekeor.melire@gmail.com>
-;;; Copyright © 2017 Arun Isaac <arunisaac@systemreboot.net>
+;;; Copyright © 2017, 2018 Arun Isaac <arunisaac@systemreboot.net>
 ;;; Copyright © 2017, 2018 Tobias Geerinckx-Rice <me@tobias.gr>
 ;;; Copyright © 2017 Theodoros Foradis <theodoros@foradis.org>
 ;;; Copyright © 2017 Rutger Helling <rhelling@mykolab.com>
@@ -586,17 +586,6 @@ was initially a fork of xmpppy, but uses non-blocking sockets.")
     (arguments
      `(#:phases
        (modify-phases %standard-phases
-         (add-after 'install 'wrap-program
-           (lambda* (#:key outputs #:allow-other-keys)
-             (let ((out (assoc-ref outputs "out")))
-               (for-each
-                (lambda (name)
-                  (let ((file (string-append out "/bin/" name))
-                        (gi-typelib-path (getenv "GI_TYPELIB_PATH")))
-                    (wrap-program file
-                      `("GI_TYPELIB_PATH" ":" prefix (,gi-typelib-path)))))
-                '("gajim" "gajim-remote" "gajim-history-manager")))
-             #t))
          (add-before 'check 'remove-test-resolver
            ;; This test requires network access.
            (lambda _
@@ -628,14 +617,22 @@ was initially a fork of xmpppy, but uses non-blocking sockets.")
                  (symlink adwaita "Adwaita")
                  (copy-recursively hicolor "hicolor")))
              #t))
-         (add-after 'install-icons 'wrap-program
+         (add-after 'install-icons 'wrap-gajim
            (lambda* (#:key inputs outputs #:allow-other-keys)
-             (wrap-program (string-append (assoc-ref outputs "out")
-                                          "/bin/gajim")
-               ;; For GtkFileChooserDialog.
-               `("GSETTINGS_SCHEMA_DIR" =
-                 (,(string-append (assoc-ref inputs "gtk+")
-                                  "/share/glib-2.0/schemas")))))))))
+             (let ((out (assoc-ref outputs "out")))
+               (for-each
+                (lambda (name)
+                  (let ((file (string-append out "/bin/" name))
+                        (gi-typelib-path (getenv "GI_TYPELIB_PATH")))
+                    (wrap-program file
+                      `("GI_TYPELIB_PATH" ":" prefix (,gi-typelib-path)))))
+                '("gajim" "gajim-remote" "gajim-history-manager"))
+               (wrap-program (string-append out "/bin/gajim")
+                 ;; For GtkFileChooserDialog.
+                 `("GSETTINGS_SCHEMA_DIR" =
+                   (,(string-append (assoc-ref inputs "gtk+")
+                                    "/share/glib-2.0/schemas")))))
+             #t)))))
     (native-inputs
      `(("intltool" ,intltool)
        ("xorg-server" ,xorg-server)))
-- 
2.15.1

[bug#32102] [PATCH v2 2/2] gnu: gajim: Combine wrap-program phases.

[Clément Lassieur]

Arun Isaac <arunisaac@systemreboot.net> writes:

> * gnu/packages/messaging.scm (gajim)[arguments]: Combine wrap-program phases
> into a single wrap-gajim phase.
> ---
>  gnu/packages/messaging.scm | 35 ++++++++++++++++-------------------
>  1 file changed, 16 insertions(+), 19 deletions(-)

If the problem is that the phases have the same name, renaming one of
them would be enough?  I'm not sure it's worth merging them, given that
they don't even apply to the same files.

Clément

[bug#32102] [PATCH v2 2/2] gnu: gajim: Combine wrap-program phases.

[Arun Isaac]

>> * gnu/packages/messaging.scm (gajim)[arguments]: Combine wrap-program phases
>> into a single wrap-gajim phase.
>> ---
>>  gnu/packages/messaging.scm | 35 ++++++++++++++++-------------------
>>  1 file changed, 16 insertions(+), 19 deletions(-)
>
> If the problem is that the phases have the same name, renaming one of
> them would be enough?  I'm not sure it's worth merging them, given that
> they don't even apply to the same files.

Ok, I'll rename the phases to wrap-gi-typelib-path and
wrap-gsettings-schema-dir respectively.

[bug#32102] [PATCH v2 2/2] gnu: gajim: Combine wrap-program phases.

[Arun Isaac]

[-- Attachment #1: Type: text/plain, Size: 202 bytes --]


Please find attached the updated patches. Sorry this took so long. I was
working on another project, and couldn't spare the computing time to
build the whole system from scratch due to these patches.


[-- Attachment #2: v3-0001-build-system-python-Do-not-double-wrap-executable.patch --]
[-- Type: text/x-patch, Size: 3267 bytes --]

From 6ee5cf4423109ab64df58c85f4114e456dda098b Mon Sep 17 00:00:00 2001
From: Arun Isaac <arunisaac@systemreboot.net>
Date: Wed, 11 Jul 2018 13:03:33 +0530
Subject: [PATCH v3 1/3] build-system: python: Do not double wrap executables.
To: clement@lassieur.org
Cc: mhw@netris.org,
    andreas@enge.fr,
    32102@debbugs.gnu.org

* guix/build/python-build-system.scm (wrap): Only wrap executables that have
not already been wrapped.
* guix/build/utils.scm (is-wrapped?): New function.
---
 guix/build/python-build-system.scm | 9 ++++-----
 guix/build/utils.scm               | 9 +++++++++
 2 files changed, 13 insertions(+), 5 deletions(-)

diff --git a/guix/build/python-build-system.scm b/guix/build/python-build-system.scm
index 376ea81f1..05e5009a4 100644
--- a/guix/build/python-build-system.scm
+++ b/guix/build/python-build-system.scm
@@ -5,6 +5,7 @@
 ;;; Copyright © 2015, 2018 Mark H Weaver <mhw@netris.org>
 ;;; Copyright © 2016 Hartmut Goebel <h.goebel@crazy-compilers.com>
 ;;; Copyright © 2018 Ricardo Wurmus <rekado@elephly.net>
+;;; Copyright © 2018 Arun Isaac <arunisaac@systemreboot.net>
 ;;;
 ;;; This file is part of GNU Guix.
 ;;;
@@ -186,11 +187,9 @@ when running checks after installing the package."
 
 (define* (wrap #:key inputs outputs #:allow-other-keys)
   (define (list-of-files dir)
-    (map (cut string-append dir "/" <>)
-         (or (scandir dir (lambda (f)
-                            (let ((s (stat (string-append dir "/" f))))
-                              (eq? 'regular (stat:type s)))))
-             '())))
+    (find-files dir (lambda (file stat)
+                      (and (eq? 'regular (stat:type stat))
+                           (not (is-wrapped? file))))))
 
   (define bindirs
     (append-map (match-lambda
diff --git a/guix/build/utils.scm b/guix/build/utils.scm
index c58a1afd1..c310b792c 100644
--- a/guix/build/utils.scm
+++ b/guix/build/utils.scm
@@ -3,6 +3,7 @@
 ;;; Copyright © 2013 Andreas Enge <andreas@enge.fr>
 ;;; Copyright © 2013 Nikita Karetnikov <nikita@karetnikov.org>
 ;;; Copyright © 2015, 2018 Mark H Weaver <mhw@netris.org>
+;;; Copyright © 2018 Arun Isaac <arunisaac@systemreboot.net>
 ;;;
 ;;; This file is part of GNU Guix.
 ;;;
@@ -21,6 +22,7 @@
 
 (define-module (guix build utils)
   #:use-module (srfi srfi-1)
+  #:use-module (srfi srfi-2)
   #:use-module (srfi srfi-11)
   #:use-module (srfi srfi-26)
   #:use-module (srfi srfi-34)
@@ -87,6 +89,7 @@
             patch-/usr/bin/file
             fold-port-matches
             remove-store-references
+            is-wrapped?
             wrap-program
 
             invoke
@@ -1003,6 +1006,12 @@ known as `nuke-refs' in Nixpkgs."
                              (put-u8 out (char->integer char))
                              result))))))
 
+(define (is-wrapped? prog)
+  "Return #t if PROG is already wrapped using wrap-program, else return #f."
+  (with-directory-excursion (dirname prog)
+    (and-let* ((match-record (string-match "^\\.(.*)-real$" (basename prog))))
+      (access? (match:substring match-record 1) X_OK))))
+
 (define* (wrap-program prog #:rest vars)
   "Make a wrapper for PROG.  VARS should look like this:
 
-- 
2.18.0


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #3: v3-0002-gnu-gajim-Rename-wrap-program-phases.patch --]
[-- Type: text/x-patch, Size: 1613 bytes --]

From 1a1762da6e62d7bcf6556df300299d9cfc995d0f Mon Sep 17 00:00:00 2001
From: Arun Isaac <arunisaac@systemreboot.net>
Date: Sat, 28 Jul 2018 17:27:26 +0530
Subject: [PATCH v3 2/3] gnu: gajim: Rename wrap-program phases.
To: clement@lassieur.org
Cc: mhw@netris.org,
    andreas@enge.fr,
    32102@debbugs.gnu.org

* gnu/packages/messaging.scm (gajim)[arguments]: Rename wrap-program phases to
wrap-gi-typelib-path and wrap-gsettings-schema-dir.
---
 gnu/packages/messaging.scm | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/gnu/packages/messaging.scm b/gnu/packages/messaging.scm
index a34f74465..0c6c2f642 100644
--- a/gnu/packages/messaging.scm
+++ b/gnu/packages/messaging.scm
@@ -586,7 +586,7 @@ was initially a fork of xmpppy, but uses non-blocking sockets.")
     (arguments
      `(#:phases
        (modify-phases %standard-phases
-         (add-after 'install 'wrap-program
+         (add-after 'install 'wrap-gi-typelib-path
            (lambda* (#:key outputs #:allow-other-keys)
              (let ((out (assoc-ref outputs "out")))
                (for-each
@@ -628,7 +628,7 @@ was initially a fork of xmpppy, but uses non-blocking sockets.")
                  (symlink adwaita "Adwaita")
                  (copy-recursively hicolor "hicolor")))
              #t))
-         (add-after 'install-icons 'wrap-program
+         (add-after 'install-icons 'wrap-gsettings-schema-dir
            (lambda* (#:key inputs outputs #:allow-other-keys)
              (wrap-program (string-append (assoc-ref outputs "out")
                                           "/bin/gajim")
-- 
2.18.0


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #4: v3-0003-gnu-gajim-Return-t-from-wrap-gsettings-schema-dir.patch --]
[-- Type: text/x-patch, Size: 1181 bytes --]

From 71a7f6e39bd5b68b596bda2a75b1d245179349d0 Mon Sep 17 00:00:00 2001
From: Arun Isaac <arunisaac@systemreboot.net>
Date: Sat, 28 Jul 2018 17:31:44 +0530
Subject: [PATCH v3 3/3] gnu: gajim: Return #t from wrap-gsettings-schema-dir
 phase.
To: clement@lassieur.org
Cc: mhw@netris.org,
    andreas@enge.fr,
    32102@debbugs.gnu.org

* gnu/packages/messaging.scm (gajim)[arguments]: Return #t from
wrap-gsettings-schema-dir phase.
---
 gnu/packages/messaging.scm | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/gnu/packages/messaging.scm b/gnu/packages/messaging.scm
index 0c6c2f642..da0d28325 100644
--- a/gnu/packages/messaging.scm
+++ b/gnu/packages/messaging.scm
@@ -635,7 +635,8 @@ was initially a fork of xmpppy, but uses non-blocking sockets.")
                ;; For GtkFileChooserDialog.
                `("GSETTINGS_SCHEMA_DIR" =
                  (,(string-append (assoc-ref inputs "gtk+")
-                                  "/share/glib-2.0/schemas")))))))))
+                                  "/share/glib-2.0/schemas"))))
+             #t)))))
     (native-inputs
      `(("intltool" ,intltool)
        ("xorg-server" ,xorg-server)))
-- 
2.18.0

[bug#32102] [PATCH v2 2/2] gnu: gajim: Combine wrap-program phases.

[Ludovic Courtès]

Hello Arun,

I’m a bit late to the party, but hey!

Arun Isaac <arunisaac@systemreboot.net> skribis:

> From 6ee5cf4423109ab64df58c85f4114e456dda098b Mon Sep 17 00:00:00 2001
> From: Arun Isaac <arunisaac@systemreboot.net>
> Date: Wed, 11 Jul 2018 13:03:33 +0530
> Subject: [PATCH v3 1/3] build-system: python: Do not double wrap executables.
> To: clement@lassieur.org
> Cc: mhw@netris.org,
>     andreas@enge.fr,
>     32102@debbugs.gnu.org

Hmm, weird!

> * guix/build/python-build-system.scm (wrap): Only wrap executables that have
> not already been wrapped.
> * guix/build/utils.scm (is-wrapped?): New function.

[...]

> --- a/guix/build/python-build-system.scm
> +++ b/guix/build/python-build-system.scm

[...]

>  (define* (wrap #:key inputs outputs #:allow-other-keys)
>    (define (list-of-files dir)
> -    (map (cut string-append dir "/" <>)
> -         (or (scandir dir (lambda (f)
> -                            (let ((s (stat (string-append dir "/" f))))
> -                              (eq? 'regular (stat:type s)))))
> -             '())))
> +    (find-files dir (lambda (file stat)
> +                      (and (eq? 'regular (stat:type stat))
> +                           (not (is-wrapped? file))))))

Something I don’t get is that ‘wrap-program’ itself is supposed to
detect already-wrapped program.  I vaguely remember discussing it before
but I forgot what the conclusions were; do we really need extra
‘wrapped?’ checks?  Can’t we fix ‘wrap-program’ itself?

>    (define bindirs
>      (append-map (match-lambda
> diff --git a/guix/build/utils.scm b/guix/build/utils.scm
> index c58a1afd1..c310b792c 100644
> --- a/guix/build/utils.scm
> +++ b/guix/build/utils.scm

[...]

> +(define (is-wrapped? prog)
> +  "Return #t if PROG is already wrapped using wrap-program, else return #f."
> +  (with-directory-excursion (dirname prog)
> +    (and-let* ((match-record (string-match "^\\.(.*)-real$" (basename prog))))
> +      (access? (match:substring match-record 1) X_OK))))

By convention I’d suggest calling it ‘wrapped?’ rather than
‘is-wrapped?’.  In fact, a more accurate name would be ‘wrapper?’.

Also I’d suggest not using SRFI-2 because IMO it doesn’t bring much and
it’s not used anywhere in Guix currently.  Also, ‘file-exists?’ rather
than ‘access?’, and no need to change directories.  So:

  (define (wrapper? prog)
    "Return #t if PROG is a wrapper as produced by 'wrap-program'."
    (and (file-exists? prog)
         (let ((base (basename prog)))
           (and (string-prefix? "." base)
                (string-suffix? "-real" base)))))

> From 71a7f6e39bd5b68b596bda2a75b1d245179349d0 Mon Sep 17 00:00:00 2001
> From: Arun Isaac <arunisaac@systemreboot.net>
> Date: Sat, 28 Jul 2018 17:31:44 +0530
> Subject: [PATCH v3 3/3] gnu: gajim: Return #t from wrap-gsettings-schema-dir
>  phase.
> To: clement@lassieur.org
> Cc: mhw@netris.org,
>     andreas@enge.fr,
>     32102@debbugs.gnu.org
>
> * gnu/packages/messaging.scm (gajim)[arguments]: Return #t from
> wrap-gsettings-schema-dir phase.

LGTM!

Thanks,
Ludo’.

[bug#32102] [PATCH v2 2/2] gnu: gajim: Combine wrap-program phases.

[Arun Isaac]

ludo@gnu.org (Ludovic Courtès) writes:

>> From 6ee5cf4423109ab64df58c85f4114e456dda098b Mon Sep 17 00:00:00 2001
>> From: Arun Isaac <arunisaac@systemreboot.net>
>> Date: Wed, 11 Jul 2018 13:03:33 +0530
>> Subject: [PATCH v3 1/3] build-system: python: Do not double wrap executables.
>> To: clement@lassieur.org
>> Cc: mhw@netris.org,
>>     andreas@enge.fr,
>>     32102@debbugs.gnu.org
>
> Hmm, weird!

What's weird? Are you referring to the Cc field? The people in the Cc
field were originally referred to by Clement. So, I put them there to
keep them in the loop.

>>  (define* (wrap #:key inputs outputs #:allow-other-keys)
>>    (define (list-of-files dir)
>> -    (map (cut string-append dir "/" <>)
>> -         (or (scandir dir (lambda (f)
>> -                            (let ((s (stat (string-append dir "/" f))))
>> -                              (eq? 'regular (stat:type s)))))
>> -             '())))
>> +    (find-files dir (lambda (file stat)
>> +                      (and (eq? 'regular (stat:type stat))
>> +                           (not (is-wrapped? file))))))
>
> Something I don’t get is that ‘wrap-program’ itself is supposed to
> detect already-wrapped program.  I vaguely remember discussing it before
> but I forgot what the conclusions were; do we really need extra
> ‘wrapped?’ checks?  Can’t we fix ‘wrap-program’ itself?

Could you refer to our earlier discussion on 32102?

https://debbugs.gnu.org/cgi/bugreport.cgi?bug=32102

In the case of Gajim, our current wrapping ends up double wrapping and
creating bin/.gajim-real-real. The original fix I proposed was to modify
`wrap-program` to fix already-wrapped detection. But, after discussion
with Clement, we decided to go with a is-wrapped? check in the python
build system. Do check out our earlier discussion and let us know what
you think.

>> +(define (is-wrapped? prog)
>> +  "Return #t if PROG is already wrapped using wrap-program, else return #f."
>> +  (with-directory-excursion (dirname prog)
>> +    (and-let* ((match-record (string-match "^\\.(.*)-real$" (basename prog))))
>> +      (access? (match:substring match-record 1) X_OK))))
>
> By convention I’d suggest calling it ‘wrapped?’ rather than
> ‘is-wrapped?’.  In fact, a more accurate name would be ‘wrapper?’.

Sure, will do.

> Also I’d suggest not using SRFI-2 because IMO it doesn’t bring much and
> it’s not used anywhere in Guix currently.  Also, ‘file-exists?’ rather
> than ‘access?’, and no need to change directories.  So:
>
>   (define (wrapper? prog)
>     "Return #t if PROG is a wrapper as produced by 'wrap-program'."
>     (and (file-exists? prog)
>          (let ((base (basename prog)))
>            (and (string-prefix? "." base)
>                 (string-suffix? "-real" base)))))

Sure, will do.

[bug#32102] [PATCH v2 2/2] gnu: gajim: Combine wrap-program phases.

[Ludovic Courtès]

Hi Arun,

Sorry for the delay.

Arun Isaac <arunisaac@systemreboot.net> skribis:

> ludo@gnu.org (Ludovic Courtès) writes:
>
>>> From 6ee5cf4423109ab64df58c85f4114e456dda098b Mon Sep 17 00:00:00 2001
>>> From: Arun Isaac <arunisaac@systemreboot.net>
>>> Date: Wed, 11 Jul 2018 13:03:33 +0530
>>> Subject: [PATCH v3 1/3] build-system: python: Do not double wrap executables.
>>> To: clement@lassieur.org
>>> Cc: mhw@netris.org,
>>>     andreas@enge.fr,
>>>     32102@debbugs.gnu.org
>>
>> Hmm, weird!
>
> What's weird? Are you referring to the Cc field? The people in the Cc
> field were originally referred to by Clement. So, I put them there to
> keep them in the loop.

Yes that makes perfect sense.  Sorry for the obscure comment on my side;
I was just surprised to see a Cc: header like this in the patch itself,
but it’s nothing special after all.

>>>  (define* (wrap #:key inputs outputs #:allow-other-keys)
>>>    (define (list-of-files dir)
>>> -    (map (cut string-append dir "/" <>)
>>> -         (or (scandir dir (lambda (f)
>>> -                            (let ((s (stat (string-append dir "/" f))))
>>> -                              (eq? 'regular (stat:type s)))))
>>> -             '())))
>>> +    (find-files dir (lambda (file stat)
>>> +                      (and (eq? 'regular (stat:type stat))
>>> +                           (not (is-wrapped? file))))))
>>
>> Something I don’t get is that ‘wrap-program’ itself is supposed to
>> detect already-wrapped program.  I vaguely remember discussing it before
>> but I forgot what the conclusions were; do we really need extra
>> ‘wrapped?’ checks?  Can’t we fix ‘wrap-program’ itself?
>
> Could you refer to our earlier discussion on 32102?
>
> https://debbugs.gnu.org/cgi/bugreport.cgi?bug=32102
>
> In the case of Gajim, our current wrapping ends up double wrapping and
> creating bin/.gajim-real-real. The original fix I proposed was to modify
> `wrap-program` to fix already-wrapped detection. But, after discussion
> with Clement, we decided to go with a is-wrapped? check in the python
> build system. Do check out our earlier discussion and let us know what
> you think.

Right.  I re-read it and it’s all clear again.  :-)  The issue is that
‘list-of-files’ in the ‘wrap’ phase of python-build-system would pick up
files that are themselves wrappers already.

Because of my slow reaction we missed the train of this ‘core-updates’
cycle.  :-/  So I think it’ll have to be for next time.  Sounds good?

Let’s not forget about it…

Thank you,
Ludo’.

[bug#32102] [PATCH v2 2/2] gnu: gajim: Combine wrap-program phases.

[Arun Isaac]

> Right.  I re-read it and it’s all clear again.  :-)  The issue is that
> ‘list-of-files’ in the ‘wrap’ phase of python-build-system would pick up
> files that are themselves wrappers already.
>
> Because of my slow reaction we missed the train of this ‘core-updates’
> cycle.  :-/  So I think it’ll have to be for next time.  Sounds good?
>
> Let’s not forget about it…

Sure, no problem! Sounds good. Apart from closely following the
guix-devel list, is there anyway I can know when the next core-updates
cycle comes up?

[bug#32102] [PATCH v2 2/2] gnu: gajim: Combine wrap-program phases.

[Ludovic Courtès]

Arun Isaac <arunisaac@systemreboot.net> skribis:

>> Right.  I re-read it and it’s all clear again.  :-)  The issue is that
>> ‘list-of-files’ in the ‘wrap’ phase of python-build-system would pick up
>> files that are themselves wrappers already.
>>
>> Because of my slow reaction we missed the train of this ‘core-updates’
>> cycle.  :-/  So I think it’ll have to be for next time.  Sounds good?
>>
>> Let’s not forget about it…
>
> Sure, no problem! Sounds good. Apart from closely following the
> guix-devel list, is there anyway I can know when the next core-updates
> cycle comes up?

Not really, it’s rather informal, so you have to pay attention to
heads-up messages on guix-devel and to branch creations/deletions on
guix-commits.

Ludo’.

[bug#32102] [PATCH v2 2/2] gnu: gajim: Combine wrap-program phases.

[Arun Isaac]

May I push this patchset to the core-updates branch? If I understand
correctly, a new core-updates cycle has come up.

https://lists.gnu.org/archive/html/guix-devel/2018-11/msg00294.html

[bug#32102] [PATCH v2 2/2] gnu: gajim: Combine wrap-program phases.

[Ludovic Courtès]

Hello Arun,

Arun Isaac <arunisaac@systemreboot.net> skribis:

> May I push this patchset to the core-updates branch? If I understand
> correctly, a new core-updates cycle has come up.
>
> https://lists.gnu.org/archive/html/guix-devel/2018-11/msg00294.html

Unfortunately ‘core-updates’ still hasn’t been merged (hence my call for
help on guix-devel :-)).

But the good news is that you can apply this patch to
‘core-updates-next’.

Thanks for not forgetting about it!

Ludo’.

bug#32102: [PATCH v2 2/2] gnu: gajim: Combine wrap-program phases.

[Arun Isaac]

I pushed "PATCH 1: Do not double wrap executables" to core-updates-next
after making the changes you suggested.

I pushed "PATCH 2: Rename wrap-program phases" to master since it
only causes a rebuild of gajim.

"PATCH 3: Return #t from wrap-gsettings-schema-dir phase" is now
irrelevant due to commit 60c5b4448961ce1745b7f0bfada1e7620f238ea0 by
Clement on master.

[bug#32102] [PATCH v2 2/2] gnu: gajim: Combine wrap-program phases.

[Clément Lassieur]

Arun Isaac <arunisaac@systemreboot.net> writes:

> I pushed "PATCH 1: Do not double wrap executables" to core-updates-next
> after making the changes you suggested.
>
> I pushed "PATCH 2: Rename wrap-program phases" to master since it
> only causes a rebuild of gajim.
>
> "PATCH 3: Return #t from wrap-gsettings-schema-dir phase" is now
> irrelevant due to commit 60c5b4448961ce1745b7f0bfada1e7620f238ea0 by
> Clement on master.

Great, thank you Arun!!