Re: [PATCH -v3] Fix gtk-im-modules for gtk+3

[Huang\, Ying]

Hi, Wenwu,

Thanks for comments.

address@hidden (宋文武) writes:

> address@hidden writes:
>
>> From: "Huang, Ying" <address@hidden>
>>
>> Gtk+3 now have multiple outputs, so the gtk-query-immodules-3.0 should be 
>> find
>> in output "bin" instead of "out".
>>
>> * guix/profiles.scm (manifest-lookup-package): New argument output to select
>>   package or store path.
>>   (gtk-im-modules): Use "bin" output to find gtk-query-immodules-3.0
>>
>> The fix works, but appears hacky, because I haven't read much guix source
>> code, so I don't know the best solution.
>>
>
> Thank you!  The patch looks good to me, but:
>
> I think we can split this into 2 patches, one for adding output
> selecting to 'manifest-lookup-package' and another for fixing
> 'gtk-im-modules'.

OK for me.  If no others have objection, I will split the patch.

> And to fix 'gtk-im-modules', we can pass the path of current
> 'gtk-query-modules-x.x' instead of the one in profile to the build
> procedure, so users don't need to install gtk+:bin explicitly (but it
> always pull in the dependency of latest gtk+:bin).  This may or may not
> be the desired behavior...

Yes.  It is better not to force gtk+:bin to be installed into profile.

> Here is what I mean:
>
> [attachment]
>
> 0001-profiles-gtk-im-modules-Fix-for-gtk3.patch
>
> From 54a9b4de63f87084984a1632800a039b155592f0 Mon Sep 17 00:00:00 2001
> From: "address@hidden" <address@hidden>
> Date: Sun, 5 Feb 2017 13:41:47 +0800
> Subject: [PATCH] profiles: gtk-im-modules: Fix for gtk3.
>
> Gtk+3 now have multiple outputs, so the gtk-query-immodules-3.0 should be find
> in output "bin" instead of "out".
>
> * guix/profiles.scm (gtk-im-modules): Pass the path of gtk-query-immodules-x.x
> as 'query' argument to the 'build' procedure.
> ---
>  guix/profiles.scm | 20 ++++++++++++++------
>  1 file changed, 14 insertions(+), 6 deletions(-)
>
> diff --git a/guix/profiles.scm b/guix/profiles.scm
> index 495a9e2e7..cb93b8b67 100644
> --- a/guix/profiles.scm
> +++ b/guix/profiles.scm
> @@ -739,7 +739,7 @@ for both major versions of GTK+."
>    (mlet %store-monad ((gtk+   (manifest-lookup-package manifest "gtk+" "3"))
>                        (gtk+-2 (manifest-lookup-package manifest "gtk+" "2")))
>  
> -    (define (build gtk gtk-version)
> +    (define (build gtk gtk-version query)
>        (let ((major (string-take gtk-version 1)))
>          (with-imported-modules '((guix build utils)
>                                   (guix build union)
> @@ -756,8 +756,6 @@ for both major versions of GTK+."
>  
>                (let* ((prefix  (string-append "/lib/gtk-" #$major ".0/"
>                                               #$gtk-version))
> -                     (query   (string-append #$gtk 
> "/bin/gtk-query-immodules-"
> -                                             #$major ".0"))
>                       (destdir (string-append #$output prefix))
>                       (moddirs (cons (string-append #$gtk prefix "/immodules")
>                                      (filter file-exists?
> @@ -768,7 +766,7 @@ for both major versions of GTK+."
>  
>                  ;; Generate a new immodules cache file.
>                  (mkdir-p (string-append #$output prefix))
> -                (let ((pipe    (apply open-pipe* OPEN_READ query modules))
> +                (let ((pipe    (apply open-pipe* OPEN_READ #$query modules))
>                        (outfile (string-append #$output prefix
>                                                "/immodules-gtk" #$major 
> ".cache")))
>                    (dynamic-wind
> @@ -784,8 +782,18 @@ for both major versions of GTK+."
>  
>      ;; Don't run the hook when there's nothing to do.
>      (let ((gexp #~(begin
> -                    #$(if gtk+   (build gtk+   "3.0.0")  #t)
> -                    #$(if gtk+-2 (build gtk+-2 "2.10.0") #t))))
> +                    #$(if gtk+
> +                          (build
> +                           gtk+ "3.0.0"
> +                           #~(string-append
> +                              #$gtk+:bin "/bin/gtk-query-immodules-3.0"))

If "gtk+" is store path instead of package, this doesn't work.  In a
previous version, "gtk+" will be store path, it is package now.  If it
will always be package in the future.  We can pass the store path of
target output too.

Best Regards,
Huang, Ying

> +                          #t)
> +                    #$(if gtk+-2
> +                          (build
> +                           gtk+-2 "2.10.0"
> +                           #~(string-append
> +                              #$gtk+-2 "/bin/gtk-query-immodules-2.0"))
> +                          #t))))
>        (if (or gtk+ gtk+-2)
>            (gexp->derivation "gtk-im-modules" gexp
>                              #:local-build? #t