Re: [PATCH] gnu: kdbusaddons: Embed path to kdeinit5, avoid dependency cycles.

[ludo@gnu.org (Ludovic Courtès)]

Hi Thomas,

Thomas Danckaert <thomas.danckaert@gmail.com> skribis:

> From: ludo@gnu.org (Ludovic Courtès)
> Subject: Re: [PATCH] gnu: kdbusaddons: Embed path to kdeinit5, avoid
> dependency cycles.
> Date: Thu, 15 Dec 2016 17:06:19 +0100
>
>> Other options that come to mind: (1) make ‘QT_PLUGIN_PATH’ and
>> ‘QML2_IMPORT_PATH’ search paths of ‘kinit’; or (2) add a “profile
>> hook”
>> that creates a file containing the search path, and patch kinit to
>> honor
>> that file somehow.
>>
>> Option 1 sounds better, but ‘QT_PLUGIN_PATH’ really belongs to Qt,
>> not
>> to kinit.
>
> I think adding QT_PLUGIN_PATH to kinit (and a number of other KDE
> packages) could be justified. KDE applications heavily rely on
> QT_PLUGIN_PATH (and use a different path than the default path used by
> Qt: ${PREFIX}/lib/plugins instead of ${PREFIX}/plugins).  See for
> example the section on environment variables at
> https://community.kde.org/Guidelines_and_HOWTOs/Build_from_source
>
> However, I'm not sure it's enough: as far as I understand,
> search-paths will only work for packages directly installed in a
> user's profile, but KDE applications often need plugins provided by
> their dependencies (e.g. for kdevelop, plugins from kdevplatform and
> kinit must be found on the QT_PLUGIN_PATH, but a user who installs
> kdevelop shouldn't be forced to install kinit and kdevplatform in
> their profile?). (Perhaps I misunderstand how search-paths works)

I think you’re right.  Search paths only work for direct dependencies.

> So, provided option (2) can somehow find the paths of dependencies as
> well, maybe that's better? (Are there examples of such a “profile
> hook” somewhere?  I didn't find anything in the manual).

It’s not documented yet, but see (guix profiles) for examples.

The difficulty in writing these hooks is that you don’t want the hook to
pull in KDE (or GNOME, or GHC, etc.) if the user doesn’t have any KDE
application installed.  So that needs extra care.

>>> +;; This version of kdbusaddons does not use kinit as an input, and
>>> is used to
>>> +;; build kinit-bootstrap, as well as bootstrap versions of all
>>> kinit
>>> +;; dependencies which also rely on kdbusaddons.
>>> +(define kdbusaddons-bootstrap
>>> +  (package
>>> +    (inherit kdbusaddons)
>>> +    (source (origin
>>> +              (inherit (package-source kdbusaddons))
>>> +              (patches '())))
>>
>> Since ‘kdbusaddons’ doesn’t have any patches, you can omit this
>> ‘source’
>> field.
>
> This commit adds a patch to kdbusaddons (the one that adds the kinit
> store directory), so I think does become necessary?

Oh, OK.

>> [...]
>>
>> Isn’t it enough to do:
>>
>>   (define kinit-bootstrap
>>     ((package-input-rewriting `((,kdbusaddons
>> . ,kdbusaddons-bootstrap)))
>>      kinit))
>>
>> ?  Remember that ‘package-input-rewriting’ replaces inputs
>> recursively.
>
> Yes, ..., yes it is :-) I had this nagging feeling that tracking the
> dependency chain like that could be automated, and therefore probably
> already _had_ been automated in guix :-) I didn't pay attention to the
> word “recursive”... would have saved me a lot of work (you should have
> seen the first versions of this patch O_o).

Well, glad you like it!  ;-)

>> You can check with ‘guix graph -e '(@ (gnu packages kde)
>> kdeinit-bootstrap)'’
>> whether you’re really getting what you want.
>
> tangentially: this seems to work only if I make the kinit-bootstrap
> package a public variable?

Right.  If it’s private, the you need to use two at signs: @@.

>>> diff --git a/gnu/packages/patches/kdbusaddons-kinit-path.patch
>>> b/gnu/packages/patches/kdbusaddons-kinit-path.patch
>>> new file mode 100644
>>> index 0000000..97c7319
>>> --- /dev/null
>>> +++ b/gnu/packages/patches/kdbusaddons-kinit-path.patch
>>> @@ -0,0 +1,15 @@
>>> +Add placeholder for kinit's store path.
>>
>> s/path/file name/ please.  :-)
>>
>> In GNU “path” traditionally means “search path.”
>
> I'm happy to comply, but note that the info manual does talk about
> store paths.  Should this be changed?

It should.  Someday!  :-)

> (Also, I chose “store directory” instead of “store file name”)

I often use “store item”.

> From e6c66e9d1daafee8907fa03db2f4c11104aab2b5 Mon Sep 17 00:00:00 2001
> From: Thomas Danckaert <thomas.danckaert@gmail.com>
> Date: Tue, 6 Dec 2016 14:55:39 +0100
> Subject: [PATCH] gnu: kdbusaddons: Embed kinit store dir, avoid dependency
>  cycles.
>
> kdbusaddons needs to know the location of the kdeinit5 executable,
> provided by kinit. kinit depends on kdbusaddons, so we add bootstrap
> versions of all packages in the dependency chain from kinit to
> kdbusaddons to avoid cyclic dependencies.
>
> * gnu/packages/kde-frameworks.scm (kinit-bootstrap,
>   kdbusaddons-bootstrap): New variables.
>   (kdbusaddons)[inputs]: Add kinit-bootstrap.
>   [source,arguments]: Add patch and substitution to embed
>   kinit-bootstrap's store directory in the code.
> * gnu/packages/patches/kdbusaddons-kinit-file-name.patch: New file.
> * gnu/local.mk (dist_patch_DATA): Add it.

Go for it!

Thanks!

Ludo’.