unofficial mirror of guix-patches@gnu.org 
 help / color / mirror / Atom feed
* [bug#42547] [PATCH] build-system/qt: Don't include useless inputs in wrapped variables.
@ 2020-07-26 12:22 Jakub Kądziołka
  2020-09-19 20:36 ` Ludovic Courtès
  0 siblings, 1 reply; 3+ messages in thread
From: Jakub Kądziołka @ 2020-07-26 12:22 UTC (permalink / raw)
  To: 42547

* 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@gmail.com>
 ;;; Copyright © 2017 Ricardo Wurmus <rekado@elephly.net>
 ;;; Copyright © 2019 Hartmut Goebel <h.goebel@crazy-compilers.com>
+;;; Copyright © 2020 Jakub Kądziołka <kuba@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@gnu.org>
 ;;; Copyright © 2018 Mark H Weaver <mhw@netris.org>
 ;;; Copyright © 2019, 2020 Hartmut Goebel <h.goebel@crazy-compilers.com>
+;;; Copyright © 2020 Jakub Kądziołka <kuba@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





^ permalink raw reply	[flat|nested] 3+ messages in thread

* [bug#42547] [PATCH] build-system/qt: Don't include useless inputs in wrapped variables.
  2020-07-26 12:22 [bug#42547] [PATCH] build-system/qt: Don't include useless inputs in wrapped variables Jakub Kądziołka
@ 2020-09-19 20:36 ` Ludovic Courtès
  2021-01-11 16:20   ` Hartmut Goebel
  0 siblings, 1 reply; 3+ messages in thread
From: Ludovic Courtès @ 2020-09-19 20:36 UTC (permalink / raw)
  To: Jakub Kądziołka; +Cc: 42547

Hi Jakub,

Jakub Kądziołka <kuba@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’.




^ permalink raw reply	[flat|nested] 3+ messages in thread

* [bug#42547] [PATCH] build-system/qt: Don't include useless inputs in wrapped variables.
  2020-09-19 20:36 ` Ludovic Courtès
@ 2021-01-11 16:20   ` Hartmut Goebel
  0 siblings, 0 replies; 3+ messages in thread
From: Hartmut Goebel @ 2021-01-11 16:20 UTC (permalink / raw)
  To: Ludovic Courtès; +Cc: Jakub Kądziołka, 42547

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@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




^ permalink raw reply	[flat|nested] 3+ messages in thread

end of thread, other threads:[~2021-01-11 16:22 UTC | newest]

Thread overview: 3+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-07-26 12:22 [bug#42547] [PATCH] build-system/qt: Don't include useless inputs in wrapped variables Jakub Kądziołka
2020-09-19 20:36 ` Ludovic Courtès
2021-01-11 16:20   ` Hartmut Goebel

unofficial mirror of guix-patches@gnu.org 

This inbox may be cloned and mirrored by anyone:

	git clone --mirror https://yhetil.org/guix-patches/1 guix-patches/git/1.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 guix-patches guix-patches/ https://yhetil.org/guix-patches \
		guix-patches@gnu.org
	public-inbox-index guix-patches

Example config snippet for mirrors.
Newsgroup available over NNTP:
	nntp://news.yhetil.org/yhetil.gnu.guix.patches


AGPL code for this site: git clone http://ou63pmih66umazou.onion/public-inbox.git