[bug#39102] [PATCH 1/2] gnu: xdg-utils: Don't use propagated inputs.

[bug#39102] [PATCH 1/2] gnu: xdg-utils: Don't use propagated inputs.

[Jakub Kądziołka]

* gnu/packages/freedesktop.scm (xdg-utils)
  [propagated-inputs, inputs]: Add awk, coreutils, grep, inetutils and
  sed; make the dependencies not propagated.
  [arguments](patch-hardcoded-paths): Move to after 'install to make
  wrap-program function correctly. Wrap the installed programs.
---
 gnu/packages/freedesktop.scm | 43 +++++++++++++++++++++++++++---------
 1 file changed, 33 insertions(+), 10 deletions(-)

diff --git a/gnu/packages/freedesktop.scm b/gnu/packages/freedesktop.scm
index 7066685dee..06006ed53d 100644
--- a/gnu/packages/freedesktop.scm
+++ b/gnu/packages/freedesktop.scm
@@ -16,6 +16,7 @@
 ;;; Copyright © 2018 Stefan Stefanović <stefanx2ovic@gmail.com>
 ;;; Copyright © 2019 Reza Alizadeh Majd <r.majd@pantherx.org>
 ;;; Copyright © 2019 Guillaume Le Vaillant <glv@posteo.net>
+;;; Copyright © 2020 Jakub Kądziołka <kuba@kadziolka.net>
 ;;;
 ;;; This file is part of GNU Guix.
 ;;;
@@ -47,6 +48,7 @@
   #:use-module (gnu packages acl)
   #:use-module (gnu packages admin)
   #:use-module (gnu packages autotools)
+  #:use-module (gnu packages base)
   #:use-module (gnu packages bash)
   #:use-module (gnu packages boost)
   #:use-module (gnu packages check)
@@ -55,6 +57,7 @@
   #:use-module (gnu packages disk)
   #:use-module (gnu packages docbook)
   #:use-module (gnu packages documentation)
+  #:use-module (gnu packages gawk)
   #:use-module (gnu packages gettext)
   #:use-module (gnu packages ghostscript)
   #:use-module (gnu packages gl)
@@ -107,20 +110,40 @@
        ("libxslt" ,libxslt)
        ("w3m" ,w3m)
        ("xmlto" ,xmlto)))
-    (propagated-inputs
-     `(("perl-file-mimeinfo" ,perl-file-mimeinfo) ; for mimeopen fallback
-       ("xprop" ,xprop) ; for Xfce detecting
-       ("xset" ,xset))) ; for xdg-screensaver
+    (inputs
+     `(("PATH:awk" ,gawk)
+       ("PATH:coreutils" ,coreutils)
+       ("PATH:grep" ,grep)
+       ("PATH:inetutils" ,inetutils) ; xdg-screensaver uses `hostname'
+       ("PATH:perl-file-mimeinfo" ,perl-file-mimeinfo) ; for mimeopen fallback
+       ("PATH:sed" ,sed)
+       ("PATH:xprop" ,xprop) ; for Xfce detecting
+       ("PATH:xset" ,xset))) ; for xdg-screensaver
     (arguments
      `(#:tests? #f   ; no check target
        #:phases
        (modify-phases %standard-phases
-         (add-after 'unpack 'patch-hardcoded-paths
-           (lambda _
-             (substitute* "scripts/xdg-mime.in"
-               (("/usr/bin/file") (which "file")))
-             (substitute* "scripts/xdg-open.in"
-               (("/usr/bin/printf") (which "printf")))
+         (add-after 'install 'patch-hardcoded-paths
+           (lambda* (#:key inputs outputs #:allow-other-keys)
+             (with-directory-excursion (string-append (assoc-ref outputs "out")
+                                                      "/bin")
+               (substitute* "xdg-mime"
+                 (("/usr/bin/file") (which "file")))
+               (substitute* "xdg-open"
+                 (("/usr/bin/printf") (which "printf")))
+               (let ((path-ext
+                       (map (lambda (package)
+                              (string-append package "/bin"))
+                            (cons (assoc-ref outputs "out")
+                              (map cdr
+                                (filter (lambda (package)
+                                          (string-prefix? "PATH:" (car package)))
+                                        inputs))))))
+                 (for-each
+                   (lambda (script)
+                     (wrap-program script
+                        `("PATH" ":" prefix ,path-ext)))
+                   (find-files "."))))
              #t))
          (add-before 'build 'locate-catalog-files
            (lambda* (#:key inputs #:allow-other-keys)
-- 
2.24.1

[bug#39102] [PATCH 2/2 staging] gnu: qtbase: Open links properly without xdg-utils in profile

[Jakub Kądziołka]

* gnu/packages/patches/qtbase-use-xdg-open-in-store.patch: New file.
* gnu/packages/qt.scm (qtbase)[source][patches]: Apply the patch.
  [inputs]: Add a dependency on xdg-utils to get its store path.
  [arguments]: Add a new phase to patch the path into the source code.
---
 .../qtbase-use-xdg-open-in-store.patch        | 103 ++++++++++++++++++
 gnu/packages/qt.scm                           |  10 +-
 2 files changed, 112 insertions(+), 1 deletion(-)
 create mode 100644 gnu/packages/patches/qtbase-use-xdg-open-in-store.patch

diff --git a/gnu/packages/patches/qtbase-use-xdg-open-in-store.patch b/gnu/packages/patches/qtbase-use-xdg-open-in-store.patch
new file mode 100644
index 0000000000..505fff13c2
--- /dev/null
+++ b/gnu/packages/patches/qtbase-use-xdg-open-in-store.patch
@@ -0,0 +1,103 @@
+To make Qt respect the user's settings for the preferred browser, use xdg-open
+even if it is not installed in the profile. The patch makes Qt always use
+xdg-open, and makes the path used easy to substitute when building.
+========================================================================
+--- qtbase-everywhere-src-5.12.6.orig/src/platformsupport/services/genericunix/qgenericunixservices.cpp	2020-01-01 20:47:16.775671802 +0100
++++ qtbase-everywhere-src-5.12.6/src/platformsupport/services/genericunix/qgenericunixservices.cpp	2020-01-01 20:56:22.167662035 +0100
+@@ -117,47 +117,6 @@
+     return QByteArrayLiteral("UNKNOWN");
+ }
+ 
+-static inline bool checkExecutable(const QString &candidate, QString *result)
+-{
+-    *result = QStandardPaths::findExecutable(candidate);
+-    return !result->isEmpty();
+-}
+-
+-static inline bool detectWebBrowser(const QByteArray &desktop,
+-                                    bool checkBrowserVariable,
+-                                    QString *browser)
+-{
+-    const char *browsers[] = {"google-chrome", "firefox", "mozilla", "opera"};
+-
+-    browser->clear();
+-    if (checkExecutable(QStringLiteral("xdg-open"), browser))
+-        return true;
+-
+-    if (checkBrowserVariable) {
+-        QByteArray browserVariable = qgetenv("DEFAULT_BROWSER");
+-        if (browserVariable.isEmpty())
+-            browserVariable = qgetenv("BROWSER");
+-        if (!browserVariable.isEmpty() && checkExecutable(QString::fromLocal8Bit(browserVariable), browser))
+-            return true;
+-    }
+-
+-    if (desktop == QByteArray("KDE")) {
+-        // Konqueror launcher
+-        if (checkExecutable(QStringLiteral("kfmclient"), browser)) {
+-            browser->append(QLatin1String(" exec"));
+-            return true;
+-        }
+-    } else if (desktop == QByteArray("GNOME")) {
+-        if (checkExecutable(QStringLiteral("gnome-open"), browser))
+-            return true;
+-    }
+-
+-    for (size_t i = 0; i < sizeof(browsers)/sizeof(char *); ++i)
+-        if (checkExecutable(QLatin1String(browsers[i]), browser))
+-            return true;
+-    return false;
+-}
+-
+ static inline bool launch(const QString &launcher, const QUrl &url)
+ {
+     const QString command = launcher + QLatin1Char(' ') + QLatin1String(url.toEncoded());
+@@ -297,6 +256,8 @@
+     return result;
+ }
+ 
++static QString xdg_open = QStringLiteral("@@GUIX:XDG-OPEN@@");
++
+ bool QGenericUnixServices::openUrl(const QUrl &url)
+ {
+     if (url.scheme() == QLatin1String("mailto")) {
+@@ -320,11 +281,7 @@
+     }
+ #endif
+ 
+-    if (m_webBrowser.isEmpty() && !detectWebBrowser(desktopEnvironment(), true, &m_webBrowser)) {
+-        qWarning("Unable to detect a web browser to launch '%s'", qPrintable(url.toString()));
+-        return false;
+-    }
+-    return launch(m_webBrowser, url);
++    return launch(xdg_open, url);
+ }
+ 
+ bool QGenericUnixServices::openDocument(const QUrl &url)
+@@ -337,11 +294,7 @@
+     }
+ #endif
+ 
+-    if (m_documentLauncher.isEmpty() && !detectWebBrowser(desktopEnvironment(), false, &m_documentLauncher)) {
+-        qWarning("Unable to detect a launcher for '%s'", qPrintable(url.toString()));
+-        return false;
+-    }
+-    return launch(m_documentLauncher, url);
++    return launch(xdg_open, url);
+ }
+ 
+ #else
+diff -ur qtbase-everywhere-src-5.12.6.orig/src/platformsupport/services/genericunix/qgenericunixservices_p.h qtbase-everywhere-src-5.12.6/src/platformsupport/services/genericunix/qgenericunixservices_p.h
+--- qtbase-everywhere-src-5.12.6.orig/src/platformsupport/services/genericunix/qgenericunixservices_p.h	2020-01-01 20:47:16.775671802 +0100
++++ qtbase-everywhere-src-5.12.6/src/platformsupport/services/genericunix/qgenericunixservices_p.h	2020-01-01 20:54:12.135664364 +0100
+@@ -65,10 +65,6 @@
+ 
+     bool openUrl(const QUrl &url) override;
+     bool openDocument(const QUrl &url) override;
+-
+-private:
+-    QString m_webBrowser;
+-    QString m_documentLauncher;
+ };
+ 
+ QT_END_NAMESPACE
diff --git a/gnu/packages/qt.scm b/gnu/packages/qt.scm
index 514577678e..0b1d372d8b 100644
--- a/gnu/packages/qt.scm
+++ b/gnu/packages/qt.scm
@@ -14,6 +14,7 @@
 ;;; Copyright © 2019, 2020 Marius Bakke <mbakke@fastmail.com>
 ;;; Copyright © 2018 John Soo <jsoo1@asu.edu>
 ;;; Copyright © 2020 Mike Rosset <mike.rosset@gmail.com>
+;;; Copyright © 2020 Jakub Kądziołka <kuba@kadziolka.net>
 ;;;
 ;;; This file is part of GNU Guix.
 ;;;
@@ -349,7 +350,8 @@ developers using C++ or QML, a CSS & JavaScript like language.")
               (base32
                "09wz7zs1x5mpgs2y4xnl2zv3naqls4sz6v2arwl1fz2dsx4jddba"))
              ;; Use TZDIR to avoid depending on package "tzdata".
-             (patches (search-patches "qtbase-use-TZDIR.patch"))
+             (patches (search-patches "qtbase-use-TZDIR.patch"
+                                      "qtbase-use-xdg-open-in-store.patch"))
              (modules '((guix build utils)))
              (snippet
                ;; corelib uses bundled harfbuzz, md4, md5, sha3
@@ -407,6 +409,7 @@ developers using C++ or QML, a CSS & JavaScript like language.")
        ("xcb-util-keysyms" ,xcb-util-keysyms)
        ("xcb-util-renderutil" ,xcb-util-renderutil)
        ("xcb-util-wm" ,xcb-util-wm)
+       ("xdg-utils" ,xdg-utils)
        ("zlib" ,zlib)))
     (native-inputs
      `(("bison" ,bison)
@@ -428,6 +431,11 @@ developers using C++ or QML, a CSS & JavaScript like language.")
                             "qmake/library/qmakebuiltins.cpp")
                           (("/bin/sh") (which "sh")))
              #t))
+         (add-after 'configure 'patch-xdg-open
+           (lambda _
+             (substitute* '("src/platformsupport/services/genericunix/qgenericunixservices.cpp")
+                          (("@@GUIX:XDG-OPEN@@") (which "xdg-open")))
+             #t))
          (replace 'configure
            (lambda* (#:key outputs #:allow-other-keys)
              (let ((out (assoc-ref outputs "out")))
-- 
2.24.1

[bug#39102] [PATCH v2 1/2] gnu: xdg-utils: Don't use propagated inputs.

[Jakub Kądziołka]

Sending a revision to the first patch after feedback on IRC. Changes:
 - Remove the `string-prefix? "PATH:"' hack, list the packages
   explicitly.
 - Use cute to make the code easier to read.

---
* gnu/packages/freedesktop.scm (xdg-utils)
  [propagated-inputs, inputs]: Add awk, coreutils, grep and sed, make
  the dependencies not propagated.
  [arguments](patch-hardcoded-paths): Move to after 'install to make
  wrap-program function correctly. Wrap the installed programs.
---
 gnu/packages/freedesktop.scm | 40 ++++++++++++++++++++++++++++--------
 1 file changed, 32 insertions(+), 8 deletions(-)

diff --git a/gnu/packages/freedesktop.scm b/gnu/packages/freedesktop.scm
index 7066685dee..9e1bf730d2 100644
--- a/gnu/packages/freedesktop.scm
+++ b/gnu/packages/freedesktop.scm
@@ -16,6 +16,7 @@
 ;;; Copyright © 2018 Stefan Stefanović <stefanx2ovic@gmail.com>
 ;;; Copyright © 2019 Reza Alizadeh Majd <r.majd@pantherx.org>
 ;;; Copyright © 2019 Guillaume Le Vaillant <glv@posteo.net>
+;;; Copyright © 2020 Jakub Kądziołka <kuba@kadziolka.net>
 ;;;
 ;;; This file is part of GNU Guix.
 ;;;
@@ -47,6 +48,7 @@
   #:use-module (gnu packages acl)
   #:use-module (gnu packages admin)
   #:use-module (gnu packages autotools)
+  #:use-module (gnu packages base)
   #:use-module (gnu packages bash)
   #:use-module (gnu packages boost)
   #:use-module (gnu packages check)
@@ -55,6 +57,7 @@
   #:use-module (gnu packages disk)
   #:use-module (gnu packages docbook)
   #:use-module (gnu packages documentation)
+  #:use-module (gnu packages gawk)
   #:use-module (gnu packages gettext)
   #:use-module (gnu packages ghostscript)
   #:use-module (gnu packages gl)
@@ -107,20 +110,41 @@
        ("libxslt" ,libxslt)
        ("w3m" ,w3m)
        ("xmlto" ,xmlto)))
-    (propagated-inputs
-     `(("perl-file-mimeinfo" ,perl-file-mimeinfo) ; for mimeopen fallback
+    (inputs
+     `(("awk" ,gawk)
+       ("coreutils" ,coreutils)
+       ("grep" ,grep)
+       ("inetutils" ,inetutils) ; xdg-screensaver uses `hostname'
+       ("perl-file-mimeinfo" ,perl-file-mimeinfo) ; for mimeopen fallback
+       ("sed" ,sed)
        ("xprop" ,xprop) ; for Xfce detecting
        ("xset" ,xset))) ; for xdg-screensaver
     (arguments
      `(#:tests? #f   ; no check target
+       #:modules ((srfi srfi-26)
+                  (guix build utils)
+                  (guix build gnu-build-system))
        #:phases
        (modify-phases %standard-phases
-         (add-after 'unpack 'patch-hardcoded-paths
-           (lambda _
-             (substitute* "scripts/xdg-mime.in"
-               (("/usr/bin/file") (which "file")))
-             (substitute* "scripts/xdg-open.in"
-               (("/usr/bin/printf") (which "printf")))
+         (add-after 'install 'patch-hardcoded-paths
+           (lambda* (#:key inputs outputs #:allow-other-keys)
+             (with-directory-excursion (string-append (assoc-ref outputs "out")
+                                                      "/bin")
+               (substitute* "xdg-mime"
+                 (("/usr/bin/file") (which "file")))
+               (substitute* "xdg-open"
+                 (("/usr/bin/printf") (which "printf")))
+               (let ((path-ext
+                       (map (cute string-append <> "/bin")
+                            (cons (assoc-ref outputs "out")
+                                  (map (cute assoc-ref inputs <>)
+                                       '("awk" "coreutils" "grep" "inetutils"
+                                         "perl-file-mimeinfo" "sed" "xprop"
+                                         "xset"))))))
+                 (for-each
+                   (cute wrap-program <>
+                        `("PATH" ":" prefix ,path-ext))
+                   (find-files "."))))
              #t))
          (add-before 'build 'locate-catalog-files
            (lambda* (#:key inputs #:allow-other-keys)
-- 
2.24.1

[bug#39102] [PATCH v2 1/2] gnu: xdg-utils: Don't use propagated inputs.

[Marius Bakke]

[-- Attachment #1.1: Type: text/plain, Size: 965 bytes --]

Jakub Kądziołka <kuba@kadziolka.net> writes:

> Sending a revision to the first patch after feedback on IRC. Changes:
>  - Remove the `string-prefix? "PATH:"' hack, list the packages
>    explicitly.
>  - Use cute to make the code easier to read.
>
> ---
> * gnu/packages/freedesktop.scm (xdg-utils)
>   [propagated-inputs, inputs]: Add awk, coreutils, grep and sed, make
>   the dependencies not propagated.
>   [arguments](patch-hardcoded-paths): Move to after 'install to make
>   wrap-program function correctly. Wrap the installed programs.

FYI: The stray '---' here made git disregard the actual commit message,
and instead only added the four lines above.  The other way around would
be perfect.  :-)

I've applied this patch, with the changes below (leaving the original
patch-hardcoded-paths phase intact, renaming the new phase accordingly;
and added a let binding for "out").

I'll address the Qt patch in a separate message.


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #1.2: diff --]
[-- Type: text/x-patch, Size: 3135 bytes --]

diff --git a/gnu/packages/freedesktop.scm b/gnu/packages/freedesktop.scm
index 9e1bf730d2..ed221439b4 100644
--- a/gnu/packages/freedesktop.scm
+++ b/gnu/packages/freedesktop.scm
@@ -122,29 +122,15 @@
     (arguments
      `(#:tests? #f   ; no check target
        #:modules ((srfi srfi-26)
-                  (guix build utils)
-                  (guix build gnu-build-system))
+                  ,@%gnu-build-system-modules)
        #:phases
        (modify-phases %standard-phases
-         (add-after 'install 'patch-hardcoded-paths
-           (lambda* (#:key inputs outputs #:allow-other-keys)
-             (with-directory-excursion (string-append (assoc-ref outputs "out")
-                                                      "/bin")
-               (substitute* "xdg-mime"
-                 (("/usr/bin/file") (which "file")))
-               (substitute* "xdg-open"
-                 (("/usr/bin/printf") (which "printf")))
-               (let ((path-ext
-                       (map (cute string-append <> "/bin")
-                            (cons (assoc-ref outputs "out")
-                                  (map (cute assoc-ref inputs <>)
-                                       '("awk" "coreutils" "grep" "inetutils"
-                                         "perl-file-mimeinfo" "sed" "xprop"
-                                         "xset"))))))
-                 (for-each
-                   (cute wrap-program <>
-                        `("PATH" ":" prefix ,path-ext))
-                   (find-files "."))))
+         (add-after 'unpack 'patch-hardcoded-paths
+           (lambda _
+             (substitute* "scripts/xdg-mime.in"
+               (("/usr/bin/file") (which "file")))
+             (substitute* "scripts/xdg-open.in"
+               (("/usr/bin/printf") (which "printf")))
              #t))
          (add-before 'build 'locate-catalog-files
            (lambda* (#:key inputs #:allow-other-keys)
@@ -168,6 +154,21 @@
                                  "/manpages/docbook.xsl man")))
                (setenv "STYLESHEET"
                        (string-append xsldoc "/html/docbook.xsl"))
+               #t)))
+         (add-after 'install 'wrap-executables
+           (lambda* (#:key inputs outputs #:allow-other-keys)
+             (let ((out (assoc-ref outputs "out")))
+               (with-directory-excursion (string-append out "/bin")
+                 (let ((path-ext
+                        (map (cute string-append <> "/bin")
+                             (cons out
+                                   (map (cute assoc-ref inputs <>)
+                                        '("awk" "coreutils" "grep" "inetutils"
+                                          "perl-file-mimeinfo" "sed" "xprop"
+                                          "xset"))))))
+                   (for-each (cute wrap-program <>
+                                   `("PATH" ":" prefix ,path-ext))
+                             (find-files "."))))
                #t))))))
     (home-page "https://www.freedesktop.org/wiki/Software/xdg-utils/")
     (synopsis "Freedesktop.org scripts for desktop integration")

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 487 bytes --]

[bug#39102] [PATCH 2/2 staging] gnu: qtbase: Open links properly without xdg-utils in profile

[Marius Bakke]

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

Jakub Kądziołka <kuba@kadziolka.net> writes:

> * gnu/packages/patches/qtbase-use-xdg-open-in-store.patch: New file.
> * gnu/packages/qt.scm (qtbase)[source][patches]: Apply the patch.
>   [inputs]: Add a dependency on xdg-utils to get its store path.
>   [arguments]: Add a new phase to patch the path into the source code.

This patch does a lot.  :-)

With this patch, BROWSER and DEFAULT_BROWSER would no longer be
consulted, right?

Does checkExecutable work with absolute file names?  I.e. could we get
away by simply patching "xdg-open" with its store file name?  Probably
should change the default browsers while at it, though.  :-)

Wrt the easy substitution, I think we should try and avoid introducing
changes to source code that depend on magic from #:phases.  That way
people will still be (mostly) able to build Qt manually using the Guix
source.  In this case, maybe defaulting to just "xdg-open" is enough?

In short, I'm looking for an easier way to achieve the same goal,
without the rather intrusive patch.

Copying Efraim as our resident Qt expert.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 487 bytes --]

[bug#39102] [PATCH 2/2 staging] gnu: qtbase: Open links properly without xdg-utils in profile

[Efraim Flashner]

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

On Sun, Jan 12, 2020 at 08:44:43PM +0100, Marius Bakke wrote:
> Jakub Kądziołka <kuba@kadziolka.net> writes:
> 
> > * gnu/packages/patches/qtbase-use-xdg-open-in-store.patch: New file.
> > * gnu/packages/qt.scm (qtbase)[source][patches]: Apply the patch.
> >   [inputs]: Add a dependency on xdg-utils to get its store path.
> >   [arguments]: Add a new phase to patch the path into the source code.
> 
> This patch does a lot.  :-)
> 
> With this patch, BROWSER and DEFAULT_BROWSER would no longer be
> consulted, right?
> 
> Does checkExecutable work with absolute file names?  I.e. could we get
> away by simply patching "xdg-open" with its store file name?  Probably
> should change the default browsers while at it, though.  :-)
> 
> Wrt the easy substitution, I think we should try and avoid introducing
> changes to source code that depend on magic from #:phases.  That way
> people will still be (mostly) able to build Qt manually using the Guix
> source.  In this case, maybe defaulting to just "xdg-open" is enough?
> 
> In short, I'm looking for an easier way to achieve the same goal,
> without the rather intrusive patch.
> 
> Copying Efraim as our resident Qt expert.

I disagree about being a qt expert, I just like fixing packages :)

Looking at the patch, I'm not in love with how there's a default list of
browsers to look for. Looking at the code, it seems that if there's
xdg-open available then open browser from the pre-defined list.

What is the code around m_documentLauncher? Does that really need to be
removed?

I think our best bet would be to substitute xdg-open with the actual
xdg-open binary around line 130 and to change the list of *browsers[] to
ones we actually have in Guix. If we switched the list to {"icecat",
"next", "chromium", "netsurf"} and made the substitution on xdg-open
we'd be in a much better place than we are now.

As it currently stands I know I don't have BROWSER or DEFAULT_BROWSER
defined and we don't have any Debian-like package named
'default-browser' or similar that we could throw in.


-- 
Efraim Flashner   <efraim@flashner.co.il>   אפרים פלשנר
GPG key = A28B F40C 3E55 1372 662D  14F7 41AA E7DC CA3D 8351
Confidentiality cannot be guaranteed on emails sent or received unencrypted

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

[bug#39102] [PATCH v2 2/2 staging] gnu: qtbase: Open links properly without xdg-utils in profile

[Jakub Kądziołka]

* gnu/packages/qt.scm (qtbase)[inputs]: Add XDG-UTILS.
  [arguments](patch-xdg-open): New phase.
---

On Sun, Jan 12, 2020 at 08:44:43PM +0100, Marius Bakke wrote:
> Jakub Kądziołka <kuba@kadziolka.net> writes:
> 
> > * gnu/packages/patches/qtbase-use-xdg-open-in-store.patch: New file.
> > * gnu/packages/qt.scm (qtbase)[source][patches]: Apply the patch.
> >   [inputs]: Add a dependency on xdg-utils to get its store path.
> >   [arguments]: Add a new phase to patch the path into the source code.
> 
> This patch does a lot.  :-)

Yeah, for some reason I thought a patch like this might as well not
leave any dead code behind.

> With this patch, BROWSER and DEFAULT_BROWSER would no longer be
> consulted, right?

BROWSER is checked by xdg-open anyway. DEFAULT_BROWSER would indeed be
no longer consulted.

> Does checkExecutable work with absolute file names?  I.e. could we get
> away by simply patching "xdg-open" with its store file name?

That's what I considered doing at first, but when I drilled a few
methods into the code, I decided that it's very unlikely to work and I
don't want to check it empirically due to the painfully long
compile-times.

> Probably should change the default browsers while at it, though.  :-)

I don't think there's much point, as that code becomes dead when
xdg-open is always found.

> Wrt the easy substitution, I think we should try and avoid introducing
> changes to source code that depend on magic from #:phases.  That way
> people will still be (mostly) able to build Qt manually using the Guix
> source.  In this case, maybe defaulting to just "xdg-open" is enough?

Perhaps applying the patch in a phase instead of (source _) would help?

> In short, I'm looking for an easier way to achieve the same goal,
> without the rather intrusive patch.

See PATCHv2, which uses a much more minimal approach, but leaves some
dead stuff that isn't likely to be optimized by the compiler. Caring
about this might not be rational, now that I think about it...

On Mon, Jan 13, 2020 at 09:53:12AM +0200, Efraim Flashner wrote:
| Looking at the patch, I'm not in love with how there's a default list of
| browsers to look for. Looking at the code, it seems that if there's
| xdg-open available then open browser from the pre-defined list.

You seem to be misreading the code. If xdg-open is available, it is
used, the browser list is only used when xdg-open isn't found.

| I think our best bet would be to [...] change the list of *browsers[] to
| ones we actually have in Guix.

As mentioned above, the list would never be read, since xdg-open would
always be found.
---
 gnu/packages/qt.scm | 11 +++++++++++
 1 file changed, 11 insertions(+)

diff --git a/gnu/packages/qt.scm b/gnu/packages/qt.scm
index 514577678e..8dc771a5f8 100644
--- a/gnu/packages/qt.scm
+++ b/gnu/packages/qt.scm
@@ -14,6 +14,7 @@
 ;;; Copyright © 2019, 2020 Marius Bakke <mbakke@fastmail.com>
 ;;; Copyright © 2018 John Soo <jsoo1@asu.edu>
 ;;; Copyright © 2020 Mike Rosset <mike.rosset@gmail.com>
+;;; Copyright © 2020 Jakub Kądziołka <kuba@kadziolka.net>
 ;;;
 ;;; This file is part of GNU Guix.
 ;;;
@@ -363,6 +364,7 @@ developers using C++ or QML, a CSS & JavaScript like language.")
     (build-system gnu-build-system)
     (propagated-inputs
      `(("mesa" ,mesa)
+       ;; Use which the package, not the function
        ("which" ,(@ (gnu packages base) which))))
     (inputs
      `(("alsa-lib" ,alsa-lib)
@@ -407,6 +409,7 @@ developers using C++ or QML, a CSS & JavaScript like language.")
        ("xcb-util-keysyms" ,xcb-util-keysyms)
        ("xcb-util-renderutil" ,xcb-util-renderutil)
        ("xcb-util-wm" ,xcb-util-wm)
+       ("xdg-utils" ,xdg-utils)
        ("zlib" ,zlib)))
     (native-inputs
      `(("bison" ,bison)
@@ -428,6 +431,14 @@ developers using C++ or QML, a CSS & JavaScript like language.")
                             "qmake/library/qmakebuiltins.cpp")
                           (("/bin/sh") (which "sh")))
              #t))
+         (add-after 'configure 'patch-xdg-open
+           (lambda _
+             (substitute* '("src/platformsupport/services/genericunix/qgenericunixservices.cpp")
+                          (("^.*const char \\*browsers.*$" all)
+                           (string-append "*browser = QStringLiteral(\""
+                                          (which "xdg-open")
+                                          "\"); return true; \n" all)))
+             #t))
          (replace 'configure
            (lambda* (#:key outputs #:allow-other-keys)
              (let ((out (assoc-ref outputs "out")))
-- 
2.24.1

bug#39102: [PATCH v2 2/2 staging] gnu: qtbase: Open links properly without xdg-utils in profile

[Marius Bakke]

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

Jakub Kądziołka <kuba@kadziolka.net> writes:

> * gnu/packages/qt.scm (qtbase)[inputs]: Add XDG-UTILS.
>   [arguments](patch-xdg-open): New phase.
> ---
>
> On Sun, Jan 12, 2020 at 08:44:43PM +0100, Marius Bakke wrote:
>> Jakub Kądziołka <kuba@kadziolka.net> writes:
>> 
>> > * gnu/packages/patches/qtbase-use-xdg-open-in-store.patch: New file.
>> > * gnu/packages/qt.scm (qtbase)[source][patches]: Apply the patch.
>> >   [inputs]: Add a dependency on xdg-utils to get its store path.
>> >   [arguments]: Add a new phase to patch the path into the source code.
>> 
>> This patch does a lot.  :-)
>
> Yeah, for some reason I thought a patch like this might as well not
> leave any dead code behind.
>
>> With this patch, BROWSER and DEFAULT_BROWSER would no longer be
>> consulted, right?
>
> BROWSER is checked by xdg-open anyway. DEFAULT_BROWSER would indeed be
> no longer consulted.
>
>> Does checkExecutable work with absolute file names?  I.e. could we get
>> away by simply patching "xdg-open" with its store file name?
>
> That's what I considered doing at first, but when I drilled a few
> methods into the code, I decided that it's very unlikely to work and I
> don't want to check it empirically due to the painfully long
> compile-times.
>
>> Probably should change the default browsers while at it, though.  :-)
>
> I don't think there's much point, as that code becomes dead when
> xdg-open is always found.

Right, thanks for clarifying.

>> Wrt the easy substitution, I think we should try and avoid introducing
>> changes to source code that depend on magic from #:phases.  That way
>> people will still be (mostly) able to build Qt manually using the Guix
>> source.  In this case, maybe defaulting to just "xdg-open" is enough?
>
> Perhaps applying the patch in a phase instead of (source _) would help?
>
>> In short, I'm looking for an easier way to achieve the same goal,
>> without the rather intrusive patch.
>
> See PATCHv2, which uses a much more minimal approach, but leaves some
> dead stuff that isn't likely to be optimized by the compiler. Caring
> about this might not be rational, now that I think about it...

Heh.  Considering possible side effects is good, even if the effects are
not very worrying.  :-)

> On Mon, Jan 13, 2020 at 09:53:12AM +0200, Efraim Flashner wrote:
> | Looking at the patch, I'm not in love with how there's a default list of
> | browsers to look for. Looking at the code, it seems that if there's
> | xdg-open available then open browser from the pre-defined list.
>
> You seem to be misreading the code. If xdg-open is available, it is
> used, the browser list is only used when xdg-open isn't found.
>
> | I think our best bet would be to [...] change the list of *browsers[] to
> | ones we actually have in Guix.
>
> As mentioned above, the list would never be read, since xdg-open would
> always be found.

OK.  The new patch is much less intimidating, I will apply it shortly.

(by the way, please attach full patches instead of plain diffs, so we
can 'git am' straight from our MUA instead of having to mangle it)

Thank you!

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 487 bytes --]

[bug#39102] [PATCH v2 2/2 staging] gnu: qtbase: Open links properly without xdg-utils in profile

[Marius Bakke]

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

Jakub Kądziołka <kuba@kadziolka.net> writes:

> On Mon, Jan 13, 2020 at 10:43:01PM +0100, Marius Bakke wrote:
>> (by the way, please attach full patches instead of plain diffs, so we
>> can 'git am' straight from our MUA instead of having to mangle it)
>
> I did! This is how format-patch generated the email, with commentary
> added like you've suggested before:
>
>> FYI: The stray '---' here made git disregard the actual commit message,
>> and instead only added the four lines above.  The other way around would
>> be perfect.  :-)
>
> How should my email have looked?

Ha, you're right, my bad!  I had not even tried applying the patch, but
it did indeed work out of the box.  So I pushed it as-is in
6e332fd3706fbe81c67b50c9d6b27df18f363c34.

I need to adjust my mental patch parser ;-)

Thank you!

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 487 bytes --]