bug#45193: Wrapper of Qt programs doesn't extend existing environment variable

bug#45193: Wrapper of Qt programs doesn't extend existing environment variable

[Zhu Zihao]

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

Reproduce steps:

   guix environment --ad-hoc qbittorrent && cat $GUIX_ENVIRONMENT/bin/qbittorrent


We can see the wrapper generated in qt-build-system doesn't extend
existing environment variable. Instead, it overrides them.

It was discussed in
https://lists.gnu.org/archive/html/guix-devel/2019-12/msg00117.html one
year ago. This's not a trivial issue because using input method in Qt
program requires an qt plugin(XIM doesn't work here) which is managed by
QT_PLUGIN_PATH.

We should change following functions:

1. guix/build/qt-build-system.scm(wrap-all-programs)
2. guix/build/qt-utils.scm(wrap-qt-program)

It's ideal to make wrap-all-programs use wrap-qt-program internally and
we don't need to maintain two copy of wrap code.



-- 
Retrieve my PGP public key: https://meta.sr.ht/~citreu.pgp

Zihao

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

bug#45193: Acknowledgement (Wrapper of Qt programs doesn't extend existing environment variable)

[Zhu Zihao]

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


In guix/build/qt-utils.scm:24:11

(define (wrap-qt-program out program)
  (define (suffix env-var path)
           ^^^^^
           I can't understand this, if you want to do a suffix wrap, you
           should do it in "wrap-program"(e.g. `("XDG_DATA_DIRS" suffix
           (,vars))), it will generate bash codes to do the job. If you
           use Guile code here, it'll capture build time environment
           variable values.

-- 
Retrieve my PGP public key: https://meta.sr.ht/~citreu.pgp

Zihao

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

bug#45193: Wrapper of Qt programs doesn't extend existing environment variable

[Mark H Weaver]

Hi,

Zhu Zihao <all_but_last@163.com> writes:

> Reproduce steps:
>
>    guix environment --ad-hoc qbittorrent && cat $GUIX_ENVIRONMENT/bin/qbittorrent
>
>
> We can see the wrapper generated in qt-build-system doesn't extend
> existing environment variable. Instead, it overrides them.
>
> It was discussed in
> https://lists.gnu.org/archive/html/guix-devel/2019-12/msg00117.html one
> year ago. This's not a trivial issue because using input method in Qt
> program requires an qt plugin(XIM doesn't work here) which is managed by
> QT_PLUGIN_PATH.
>
> We should change following functions:
>
> 1. guix/build/qt-build-system.scm(wrap-all-programs)
> 2. guix/build/qt-utils.scm(wrap-qt-program)
>
> It's ideal to make wrap-all-programs use wrap-qt-program internally and
> we don't need to maintain two copy of wrap code.

I agree with your analysis.  Would you like to propose a patch and test
it as thoroughly as you can?

     Regards,
       Mark

bug#45193: Wrapper of Qt programs doesn't extend existing environment variable

[Zhu Zihao]

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


Mark H Weaver writes:

> I agree with your analysis.  Would you like to propose a patch and test
> it as thoroughly as you can?

I just saw a patch posted by somebody on debbugs.

https://issues.guix.gnu.org/45221

Maybe we can go there to improve his patch and we don't have to write it
from scratch.

-- 
Retrieve my PGP public key:

  gpg --recv-keys D47A9C8B2AE3905B563D9135BE42B352A9F6821F

Zihao

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

bug#45193: Wrapper of Qt programs doesn't extend existing environment variable

[Zhu Zihao]

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


I try to read and understand how wrap-qt-program in qt-utils.scm works.
When building QT program, Guix builder populates qt related environment
variable, and wrap-qt-program just record it into wrapper.

However, the wrap behaviour in qt-build-system is quite different, it
search all inputs and mark them should be included in envvar definition
if correspond directory exists.

Another difference is, wrap-qt-program will include the directory of
output in envvar but qt-build-system won't do.

I'm not sure whether we need to include output, and don't know recording
build time environment follows reproducible build rule or not. Maybe we
need an expert on Qt programming/packaging to give us some hints? :(

-- 
Retrieve my PGP public key:

  gpg --recv-keys D47A9C8B2AE3905B563D9135BE42B352A9F6821F

Zihao

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

bug#45193: Wrapper of Qt programs doesn't extend existing environment variable

[Hartmut Goebel]

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

Zhu Zihao wrote

> When building QT program, Guix builder populates qt related 
> environmentvariable, and wrap-qt-program just record it into wrapper.
>
> However, the wrap behaviour in qt-build-system is quite different, 
> itsearch all inputs and mark them should be included in envvar 
> definitionif correspond directory exists.

This will have the same result in must cases:

The environment variables used in qt-utils are defined as 
"native-search-paths" by some package. So these variables will be set 
when creating the build environment, based in the inputs. So if the 
package does not touch these variables, the output should be the same 
(beside perhaps the order).

When using the environment-variables, this would allow the package 
definition to remove unwanted parts. Nevertheless this is cumbersome 
(fetching the input, string-append, manipulating the variable value). 
And AFAIS none of the pacakges using wrap-qt-program does this.

I agree that leaking the environments variables from the build 
environment to the package is not a good idea. Also we might want to add 
some filters to avoid all imports (including cmake) are going into the 
wrapping variables - which is much easier when dealing with inputs nor 
strings.


> Another difference is, wrap-qt-program will include the directory 
> ofoutput in envvar but qt-build-system won't do.

If I understand the code correctly,  line 103 of qt-build-system also 
handle the output directories

                              (append (list directory)
                                      input-directories))))

and the qt-build-system should even handle different outputs (while 
qt-tuils does not):

   (for-each handle-output outputs)

(I may be wrong on this, please double check.

-- 
Regards
Hartmut Goebel

| Hartmut Goebel          |h.goebel@crazy-compilers.com                |
|www.crazy-compilers.com  | compilers which you thought are impossible |


[-- Attachment #2: Type: text/html, Size: 2986 bytes --]

bug#45193: Wrapper of Qt programs doesn't extend existing environment variable

[Zhu Zihao]

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


Hartmut Goebel writes:

> I agree that leaking the environments variables from the build environment to
> the package is not a good idea. Also we might want to add some filters to avoid
> all imports (including cmake) are going into the wrapping variables - which is
> much easier when dealing with inputs nor strings.

I just check how Nixpkgs do Qt wrapping, it use same strategy like wrap-qt-program.
Since our environment variable only contains the path to inputs, capture
the build-time environment can be forgiven (compare with patch-shebang).

I think the main problem is include unwanted directory accidentally and
increase the closure size. But it looks like an impossible job to do it
automatically. My idea is provide a keyword argument
#:qt-wrap-exclude-inputs to prevent qt-build-system to search unwanted inputs.

BTW, would you like to use prefix wrap for wrap-qt-program in qt-utils.scm?

> If I understand the code correctly,  line 103 of qt-build-system also handle the
> output directories
>
>                              (append (list directory)
>                                      input-directories))))
>
> and the qt-build-system should even handle different outputs (while qt-tuils
> does not):
>
>   (for-each handle-output outputs)
>
> (I may be wrong on this, please double check.

Yes you're right, output was handled. I misunderstood the code before. 

-- 
Retrieve my PGP public key:

  gpg --recv-keys D47A9C8B2AE3905B563D9135BE42B352A9F6821F

Zihao

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

bug#45193: Wrapper of Qt programs doesn't extend existing environment variable

[Hartmut Goebel]

Am 19.12.20 um 19:20 schrieb Zhu Zihao:
> BTW, would you like to use prefix wrap for wrap-qt-program in qt-utils.scm?

I would support your proposal of unifying both wrappers. (Which way 
round is a matter of closure-size. I assume moving the code to 
qt-util.scm is the smaller solution.)

-- 
Regards
Hartmut Goebel

| Hartmut Goebel          | h.goebel@crazy-compilers.com               |
| www.crazy-compilers.com | compilers which you thought are impossible |

bug#45193: Wrapper of Qt programs doesn't extend existing environment variable

[Zhu Zihao]

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


Any progress in this patch? It's painful for CJK users that can't use
input method for Qt programs :(

-- 
Retrieve my PGP public key:

  gpg --recv-keys D47A9C8B2AE3905B563D9135BE42B352A9F6821F

Zihao

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

bug#45193: Wrapper of Qt programs doesn't extend existing environment variable

[Hartmut Goebel]

Patches are almost done. Expect the within thee next few days.

-- 
Regards
Hartmut Goebel

| Hartmut Goebel          | h.goebel@crazy-compilers.com               |
| www.crazy-compilers.com | compilers which you thought are impossible |