[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
[bug#39102] [PATCH v2 2/2 staging] gnu: qtbase: Open links properly with
[bug#39102] [PATCH v2 2/2 staging] gnu: qtbase: Open links properly without xdg-utils in profile
Mon, 13 Jan 2020 12:39:45 +0100
* 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 <address@hidden> 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
> 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
@@ -14,6 +14,7 @@
;;; Copyright © 2019, 2020 Marius Bakke <address@hidden>
;;; Copyright © 2018 John Soo <address@hidden>
;;; Copyright © 2020 Mike Rosset <address@hidden>
+;;; Copyright © 2020 Jakub Kądziołka <address@hidden>
;;; This file is part of GNU Guix.
+ ;; Use which the package, not the function
("which" ,(@ (gnu packages base) which))))
+ ("xdg-utils" ,xdg-utils)
(("/bin/sh") (which "sh")))
+ (add-after 'configure 'patch-xdg-open
+ (lambda _
+ (("^.*const char \\*browsers.*$" all)
+ (string-append "*browser = QStringLiteral(\""
+ (which "xdg-open")
+ "\"); return true; \n" all)))
(lambda* (#:key outputs #:allow-other-keys)
(let ((out (assoc-ref outputs "out")))