GNU bug report logs - #42547
[PATCH] build-system/qt: Don't include useless inputs in wrapped variables.

Previous Next

Package: guix-patches;

Reported by: Jakub Kądziołka <kuba <at> kadziolka.net>

Date: Sun, 26 Jul 2020 12:23:02 UTC

Severity: normal

Tags: patch

Done: Hartmut Goebel <h.goebel <at> goebel-consult.de>

Bug is archived. No further changes may be made.

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

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

View this report as an mbox folder, status mbox, maintainer mbox


Report forwarded to guix-patches <at> gnu.org:
bug#42547; Package guix-patches. (Sun, 26 Jul 2020 12:23:02 GMT) Full text and rfc822 format available.

Acknowledgement sent to Jakub Kądziołka <kuba <at> kadziolka.net>:
New bug report received and forwarded. Copy sent to guix-patches <at> gnu.org. (Sun, 26 Jul 2020 12:23:02 GMT) Full text and rfc822 format available.

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

From: Jakub Kądziołka <kuba <at> kadziolka.net>
To: guix-patches <at> gnu.org
Subject: [PATCH] build-system/qt: Don't include useless inputs in wrapped
 variables.
Date: Sun, 26 Jul 2020 14:22:34 +0200
* guix/build-system/qt.scm (qt-build)[qt-wrap-excluded-inputs]: New argument.
* guix/build/qt-build-system.scm (variables-for-wrapping): Take the
  output directory as an argument for special handling. Check for
  subdirectories of /share used by Qt before including inputs in
  XDG_DATA_DIRS.
  (wrap-all-programs): Pass the output directory to variables-for-wrapping.
  (wrap-all-programs)[qt-wrap-excluded-inputs]: New argument.
---
 guix/build-system/qt.scm       |  3 +++
 guix/build/qt-build-system.scm | 45 ++++++++++++++++++++++++----------
 2 files changed, 35 insertions(+), 13 deletions(-)

diff --git a/guix/build-system/qt.scm b/guix/build-system/qt.scm
index 118022ec45..3da73e2b58 100644
--- a/guix/build-system/qt.scm
+++ b/guix/build-system/qt.scm
@@ -3,6 +3,7 @@
 ;;; Copyright © 2013 Cyril Roelandt <tipecaml <at> gmail.com>
 ;;; Copyright © 2017 Ricardo Wurmus <rekado <at> elephly.net>
 ;;; Copyright © 2019 Hartmut Goebel <h.goebel <at> crazy-compilers.com>
+;;; Copyright © 2020 Jakub Kądziołka <kuba <at> kadziolka.net>
 ;;;
 ;;; This file is part of GNU Guix.
 ;;;
@@ -123,6 +124,7 @@
                                          "bin" "sbin"))
                    (phases '(@ (guix build qt-build-system)
                                %standard-phases))
+                   (qt-wrap-excluded-inputs ''("qttools"))
                    (qt-wrap-excluded-outputs ''())
                    (system (%current-system))
                    (imported-modules %qt-build-system-modules)
@@ -146,6 +148,7 @@ provides a 'CMakeLists.txt' file as its build system."
                  #:search-paths ',(map search-path-specification->sexp
                                        search-paths)
                  #:phases ,phases
+                 #:qt-wrap-excluded-inputs ,qt-wrap-excluded-inputs
                  #:qt-wrap-excluded-outputs ,qt-wrap-excluded-outputs
                  #:configure-flags ,configure-flags
                  #:make-flags ,make-flags
diff --git a/guix/build/qt-build-system.scm b/guix/build/qt-build-system.scm
index 005157b0a4..1239c9f5fb 100644
--- a/guix/build/qt-build-system.scm
+++ b/guix/build/qt-build-system.scm
@@ -3,6 +3,7 @@
 ;;; Copyright © 2014, 2015 Ludovic Courtès <ludo <at> gnu.org>
 ;;; Copyright © 2018 Mark H Weaver <mhw <at> netris.org>
 ;;; Copyright © 2019, 2020 Hartmut Goebel <h.goebel <at> crazy-compilers.com>
+;;; Copyright © 2020 Jakub Kądziołka <kuba <at> kadziolka.net>
 ;;;
 ;;; This file is part of GNU Guix.
 ;;;
@@ -47,29 +48,44 @@
   (setenv "CTEST_OUTPUT_ON_FAILURE" "1")
   #t)
 
-(define (variables-for-wrapping base-directories)
+;; NOTE: Apart from standard subdirectories of /share, Qt also provides facilities
+;; for per-application data directories, such as /share/quassel. Thus, we include
+;; the output directory even if it doesn't contain any of the standard subdirectories.
+(define (variables-for-wrapping base-directories output-directory)
 
-  (define (collect-sub-dirs base-directories subdirectory)
+  (define (collect-sub-dirs base-directories subdir-spec)
     (filter-map
      (lambda (dir)
-       (let ((directory (string-append dir subdirectory)))
-         (if (directory-exists? directory) directory #f)))
+       (and
+         (match subdir-spec
+           ((subdir) (directory-exists? (string-append dir subdir)))
+           ((subdir children)
+            (or
+              (or-map
+                (lambda (child)
+                  (directory-exists? (string-append dir subdir child)))
+                children)
+              (and (eq? dir output-directory)
+                   (directory-exists? (string-append dir subdir))))))
+         (string-append dir (car subdir-spec))))
      base-directories))
 
   (filter
    (lambda (var-to-wrap) (not (null? (last var-to-wrap))))
    (map
-    (lambda (var-spec)
-      `(,(first var-spec) = ,(collect-sub-dirs base-directories (last var-spec))))
+    (match-lambda
+      ((var . subdir-spec)
+       `(,var = ,(collect-sub-dirs base-directories subdir-spec))))
     (list
      ;; these shall match the search-path-specification for Qt and KDE
      ;; libraries
-     '("XDG_DATA_DIRS" "/share")
+     '("XDG_DATA_DIRS" "/share" ("/applications" "/fonts" "/icons" "/mime"))
      '("XDG_CONFIG_DIRS" "/etc/xdg")
      '("QT_PLUGIN_PATH" "/lib/qt5/plugins")
      '("QML2_IMPORT_PATH" "/lib/qt5/qml")))))
 
 (define* (wrap-all-programs #:key inputs outputs
+                            (qt-wrap-excluded-inputs '("qttools"))
                             (qt-wrap-excluded-outputs '())
                             #:allow-other-keys)
   "Implement phase \"qt-wrap\": look for GSettings schemas and
@@ -90,10 +106,13 @@ add a dependency of that output on Qt."
            (string-append directory "/lib/libexec"))))
 
   (define input-directories
-    ;; FIXME: Filter out unwanted inputs, e.g. cmake
-    (match inputs
-           (((_ . dir) ...)
-            dir)))
+    (filter-map
+      (match-lambda
+        ((name . dir)
+         (if (member name qt-wrap-excluded-inputs)
+           #f
+           dir)))
+      inputs))
 
   (define handle-output
     (match-lambda
@@ -101,8 +120,8 @@ add a dependency of that output on Qt."
       (unless (member output qt-wrap-excluded-outputs)
         (let ((bin-list     (find-files-to-wrap directory))
               (vars-to-wrap (variables-for-wrapping
-                             (append (list directory)
-                                     input-directories))))
+                             (cons directory input-directories)
+                             directory)))
           (when (not (null? vars-to-wrap))
             (for-each (cut apply wrap-program <> vars-to-wrap)
                       bin-list)))))))
-- 
2.27.0





Information forwarded to guix-patches <at> gnu.org:
bug#42547; Package guix-patches. (Sat, 19 Sep 2020 20:37:02 GMT) Full text and rfc822 format available.

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

From: Ludovic Courtès <ludo <at> gnu.org>
To: Jakub Kądziołka <kuba <at> kadziolka.net>
Cc: 42547 <at> debbugs.gnu.org
Subject: Re: [bug#42547] [PATCH] build-system/qt: Don't include useless
 inputs in wrapped variables.
Date: Sat, 19 Sep 2020 22:36:27 +0200
Hi Jakub,

Jakub Kądziołka <kuba <at> kadziolka.net> skribis:

> * guix/build-system/qt.scm (qt-build)[qt-wrap-excluded-inputs]: New argument.
> * guix/build/qt-build-system.scm (variables-for-wrapping): Take the
>   output directory as an argument for special handling. Check for
>   subdirectories of /share used by Qt before including inputs in
>   XDG_DATA_DIRS.
>   (wrap-all-programs): Pass the output directory to variables-for-wrapping.
>   (wrap-all-programs)[qt-wrap-excluded-inputs]: New argument.

This sounds like a good idea.

Do you know what impact this has on the closure size of packages you
looked at?

There are quite a few packages using ‘qt-build-system’.  Probably a
change for ‘staging’ or ‘core-updates’?

> +(define (variables-for-wrapping base-directories output-directory)
>  
> -  (define (collect-sub-dirs base-directories subdirectory)
> +  (define (collect-sub-dirs base-directories subdir-spec)
>      (filter-map
>       (lambda (dir)
> -       (let ((directory (string-append dir subdirectory)))
> -         (if (directory-exists? directory) directory #f)))
> +       (and
> +         (match subdir-spec
> +           ((subdir) (directory-exists? (string-append dir subdir)))
> +           ((subdir children)
> +            (or
> +              (or-map
> +                (lambda (child)
> +                  (directory-exists? (string-append dir subdir child)))
> +                children)
> +              (and (eq? dir output-directory)
> +                   (directory-exists? (string-append dir subdir))))))
> +         (string-append dir (car subdir-spec))))
>       base-directories))

I’d move ‘match’ around ‘and’ to avoid ‘car’ on the next-to-last line.

(eq? dir output-directory) should probably be (string=? dir
output-directory).  (‘eq?’ is pointer equality.)

I’m also not a fan of ‘or-map’.

>    (filter
>     (lambda (var-to-wrap) (not (null? (last var-to-wrap))))
>     (map
> -    (lambda (var-spec)
> -      `(,(first var-spec) = ,(collect-sub-dirs base-directories (last var-spec))))
> +    (match-lambda
> +      ((var . subdir-spec)
> +       `(,var = ,(collect-sub-dirs base-directories subdir-spec))))
>      (list
>       ;; these shall match the search-path-specification for Qt and KDE
>       ;; libraries
> -     '("XDG_DATA_DIRS" "/share")
> +     '("XDG_DATA_DIRS" "/share" ("/applications" "/fonts" "/icons" "/mime"))

So the goal here is to refine what goes to XDG_DATA_DIRS, is that
correct?

Perhaps this should be separate from the patch that removes qttools from
the PATH of wrapped programs?

>  (define* (wrap-all-programs #:key inputs outputs
> +                            (qt-wrap-excluded-inputs '("qttools"))
>                              (qt-wrap-excluded-outputs '())
>                              #:allow-other-keys)
>    "Implement phase \"qt-wrap\": look for GSettings schemas and
> @@ -90,10 +106,13 @@ add a dependency of that output on Qt."
>             (string-append directory "/lib/libexec"))))
>  
>    (define input-directories
> -    ;; FIXME: Filter out unwanted inputs, e.g. cmake
> -    (match inputs
> -           (((_ . dir) ...)
> -            dir)))
> +    (filter-map
> +      (match-lambda
> +        ((name . dir)
> +         (if (member name qt-wrap-excluded-inputs)
> +           #f
> +           dir)))
> +      inputs))

Rather: (filter-map (match-lambda
                      ((label . directory)
                       (and (member label qt-wrap-excluded-inputs)
                            directory)))
                    inputs)

Could you send an updated patch?

Thanks,
Ludo’.




Information forwarded to guix-patches <at> gnu.org:
bug#42547; Package guix-patches. (Mon, 11 Jan 2021 16:22:02 GMT) Full text and rfc822 format available.

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

From: "Hartmut Goebel" <hartmut <at> goebel-consult.de>
To: Ludovic Courtès <ludo <at> gnu.org>
Cc: Jakub Kądziołka <kuba <at> kadziolka.net>,
 42547 <at> debbugs.gnu.org
Subject: Re: [bug#42547] [PATCH] build-system/qt: Don't include useless inputs
 in wrapped variables.
Date: Mon, 11 Jan 2021 17:20:54 +0100
Hi,

I updated this patch together with other fixes for the qt-build service.
See http://issues.guix.gnu.org/45784 and following

TL;DR for this one:
- split refining what goes to XDG_DATA_DIRS into a separate patch
  see http://issues.guix.gnu.org/45787
- most other requested changes applied, see http://issues.guix.gnu.org/45786


Ludovic Courtès <ludo <at> gnu.org> writes:

> Do you know what impact this has on the closure size of packages you
> looked at?

About 220 KB. This is roughly the size of cmake-minimal, which was
references via the env-var.

zeal      before: 1420.7 MiB   after : 1229.1 MiB
quassel   before: 1432.5 MiB   after : 1220.8 MiB


Maybe more important then the reduction of size: There are now much less
variables in the wrapper - it is much cleaner now.


> There are quite a few packages using ‘qt-build-system’.  Probably a
> change for ‘staging’ or ‘core-updates’?

This might still go into staging: Approx. 175 packages use the
qt-build-system as of today. Not checked for dependencies though.


> I’m also not a fan of ‘or-map’.

Lacking an alternative (for my limited scheme knowledge) I kept this.

Regards
hartmut




bug closed, send any further explanations to 42547 <at> debbugs.gnu.org and Jakub Kądziołka <kuba <at> kadziolka.net> Request was from Hartmut Goebel <h.goebel <at> goebel-consult.de> to control <at> debbugs.gnu.org. (Fri, 29 Jan 2021 22:09:01 GMT) Full text and rfc822 format available.

bug archived. Request was from Debbugs Internal Request <help-debbugs <at> gnu.org> to internal_control <at> debbugs.gnu.org. (Sat, 27 Feb 2021 12:24:06 GMT) Full text and rfc822 format available.

This bug report was last modified 3 years and 30 days ago.

Previous Next


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