[PATCH] build-system/qt: Don't include useless inputs in wrapped variables.

  • Done
  • quality assurance status badge
Details
4 participants
  • Hartmut Goebel
  • Hartmut Goebel
  • Jakub K?dzio?ka
  • Ludovic Courtès
Owner
unassigned
Submitted by
Jakub K?dzio?ka
Severity
normal
J
J
Jakub K?dzio?ka wrote on 26 Jul 2020 14:22
(address . guix-patches@gnu.org)
20200726122234.6593-1-kuba@kadziolka.net
* 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(-)

Toggle diff (123 lines)
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
L
L
Ludovic Courtès wrote on 19 Sep 2020 22:36
(name . Jakub K?dzio?ka)(address . kuba@kadziolka.net)(address . 42547@debbugs.gnu.org)
87imc9pkqc.fsf@gnu.org
Hi Jakub,

Jakub K?dzio?ka <kuba@kadziolka.net> skribis:

Toggle quote (8 lines)
> * 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’?

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

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

Toggle quote (21 lines)
> (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’.
H
H
Hartmut Goebel wrote on 11 Jan 2021 17:20
(name . Ludovic Courtès)(address . ludo@gnu.org)
87o8hvfnrt.fsf@lenashee.goebel-consult.de
Hi,

I updated this patch together with other fixes for the qt-build service.

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


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

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


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


Toggle quote (2 lines)
> I’m also not a fan of ‘or-map’.

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

Regards
hartmut
H
H
Hartmut Goebel wrote on 29 Jan 2021 23:08
(no subject)
(name . control)(address . control@debbugs.gnu.org)
d9ed37ed-1160-a652-0c1e-9fd17eeb6bbd@goebel-consult.de
close 45193

close 42547

merge 45193 45785 45784 45786 45787

quit

Pushed to staging as 104151f4f45f4bc3a816e3ad42256452932e0d8d
?