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

  • Done
  • quality assurance status badge
Details
4 participants
  • Arun Isaac
  • Clément Lassieur
  • Ludovic Courtès
  • Mark H Weaver
Owner
unassigned
Submitted by
Arun Isaac
Severity
normal
A
A
Arun Isaac wrote on 9 Jul 2018 03:31
(address . guix-patches@gnu.org)(name . Arun Isaac)(address . arunisaac@systemreboot.net)
20180709013103.26091-1-arunisaac@systemreboot.net
* 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(-)

Toggle diff (24 lines)
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
C
C
Clément Lassieur wrote on 9 Jul 2018 09:35
(name . Arun Isaac)(address . arunisaac@systemreboot.net)(address . 32102@debbugs.gnu.org)
87k1q4j1zk.fsf@lassieur.org
Hi Arun,

Arun Isaac <arunisaac@systemreboot.net> writes:

Toggle quote (5 lines)
> * 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
A
A
Arun Isaac wrote on 9 Jul 2018 12:49
(name . Clément Lassieur)(address . clement@lassieur.org)(address . 32102@debbugs.gnu.org)
cu7efgcn0pg.fsf@systemreboot.net
Toggle quote (7 lines)
>> * 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"

Toggle quote (5 lines)
> 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?
C
C
Clément Lassieur wrote on 9 Jul 2018 16:12
(name . Arun Isaac)(address . arunisaac@systemreboot.net)(address . 32102@debbugs.gnu.org)
87r2kc8pm8.fsf@lassieur.org
Arun Isaac <arunisaac@systemreboot.net> writes:

Toggle quote (25 lines)
>>> * 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
A
A
Arun Isaac wrote on 10 Jul 2018 07:16
(name . Clément Lassieur)(address . clement@lassieur.org)(address . 32102@debbugs.gnu.org)
cu7bmbfn00f.fsf@systemreboot.net
Toggle quote (3 lines)
> 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?
C
C
Clément Lassieur wrote on 10 Jul 2018 10:57
(name . Arun Isaac)(address . arunisaac@systemreboot.net)
87h8l7ii2f.fsf@lassieur.org
Arun Isaac <arunisaac@systemreboot.net> writes:

Toggle quote (7 lines)
>> 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.
M
M
Mark H Weaver wrote on 10 Jul 2018 20:13
(name . Clément Lassieur)(address . clement@lassieur.org)
87bmbfyn68.fsf@netris.org
Clément Lassieur <clement@lassieur.org> writes:

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

Toggle quote (4 lines)
> 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
A
A
Arun Isaac wrote on 11 Jul 2018 21:26
[PATCH v2 0/2] build-system: python: Only wrap non-hidden executable files
(address . clement@lassieur.org)
20180711192652.20186-1-arunisaac@systemreboot.net
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
A
A
Arun Isaac wrote on 11 Jul 2018 21:26
[PATCH v2 2/2] gnu: gajim: Combine wrap-program phases.
(address . clement@lassieur.org)
20180711192652.20186-3-arunisaac@systemreboot.net
* 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(-)

Toggle diff (63 lines)
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
A
A
Arun Isaac wrote on 11 Jul 2018 21:26
[PATCH v2 1/2] build-system: python: Only wrap non-hidden executable files.
(address . clement@lassieur.org)
20180711192652.20186-2-arunisaac@systemreboot.net
* 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(-)

Toggle diff (27 lines)
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
C
C
Clément Lassieur wrote on 13 Jul 2018 09:42
(name . Arun Isaac)(address . arunisaac@systemreboot.net)
87a7qvmvjp.fsf@lassieur.org
Arun Isaac <arunisaac@systemreboot.net> writes:

Toggle quote (32 lines)
> * 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
A
A
Arun Isaac wrote on 13 Jul 2018 10:35
(name . Clément Lassieur)(address . clement@lassieur.org)
cu77elzttws.fsf@systemreboot.net
Toggle quote (6 lines)
> 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.
C
C
Clément Lassieur wrote on 13 Jul 2018 10:38
Re: [PATCH v2 2/2] gnu: gajim: Combine wrap-program phases.
(name . Arun Isaac)(address . arunisaac@systemreboot.net)
87zhyv1qe8.fsf@lassieur.org
Arun Isaac <arunisaac@systemreboot.net> writes:

Toggle quote (6 lines)
> * 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
A
A
Arun Isaac wrote on 13 Jul 2018 11:45
(name . Clément Lassieur)(address . clement@lassieur.org)
cu74lh3tqnc.fsf@systemreboot.net
Toggle quote (10 lines)
>> * 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.
A
A
Arun Isaac wrote on 28 Jul 2018 22:42
(name . Clément Lassieur)(address . clement@lassieur.org)(address . 32102@debbugs.gnu.org)
cu7h8kjjdm5.fsf@systemreboot.net
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.
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(-)

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

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

Toggle diff (16 lines)
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
L
L
Ludovic Courtès wrote on 29 Jul 2018 16:20
Re: [bug#32102] [PATCH v2 2/2] gnu: gajim: Combine wrap-program phases.
(name . Arun Isaac)(address . arunisaac@systemreboot.net)
87y3dudsz0.fsf@gnu.org
Hello Arun,

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

Arun Isaac <arunisaac@systemreboot.net> skribis:

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

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

[...]

Toggle quote (3 lines)
> --- a/guix/build/python-build-system.scm
> +++ b/guix/build/python-build-system.scm

[...]

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

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

[...]

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

Toggle quote (13 lines)
> 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’.
A
A
Arun Isaac wrote on 30 Jul 2018 02:30
(name . Ludovic Courtès)(address . ludo@gnu.org)
cu7tvohd0pn.fsf@systemreboot.net
ludo@gnu.org (Ludovic Courtès) writes:

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

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


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.

Toggle quote (9 lines)
>> +(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.

Toggle quote (11 lines)
> 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.
L
L
Ludovic Courtès wrote on 27 Aug 2018 13:37
(name . Arun Isaac)(address . arunisaac@systemreboot.net)
87mut8avme.fsf@gnu.org
Hi Arun,

Sorry for the delay.

Arun Isaac <arunisaac@systemreboot.net> skribis:

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

Toggle quote (27 lines)
>>> (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’.
A
A
Arun Isaac wrote on 28 Aug 2018 10:37
(name . Ludovic Courtès)(address . ludo@gnu.org)
cu77ekayjin.fsf@systemreboot.net
Toggle quote (9 lines)
> 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?
L
L
Ludovic Courtès wrote on 29 Aug 2018 22:42
(name . Arun Isaac)(address . arunisaac@systemreboot.net)
87wos89a6i.fsf@gnu.org
Arun Isaac <arunisaac@systemreboot.net> skribis:

Toggle quote (13 lines)
>> 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’.
A
L
L
Ludovic Courtès wrote on 23 Nov 2018 10:09
(name . Arun Isaac)(address . arunisaac@systemreboot.net)
87zhu088ji.fsf@gnu.org
Hello Arun,

Arun Isaac <arunisaac@systemreboot.net> skribis:

Toggle quote (5 lines)
> 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’.
A
A
Arun Isaac wrote on 27 Nov 2018 11:43
(name . Ludovic Courtès)(address . ludo@gnu.org)
cu71s76ol5k.fsf@systemreboot.net
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.
Closed
C
C
Clément Lassieur wrote on 27 Nov 2018 13:24
(name . Arun Isaac)(address . arunisaac@systemreboot.net)
87zhtuyahg.fsf@lassieur.org
Arun Isaac <arunisaac@systemreboot.net> writes:

Toggle quote (10 lines)
> 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!!
Closed
?