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

* 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(+)

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!

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!