[bug#48849,core-updates] : Add #:sh argument to wrap-qt-program

Hi guix,

This patch series adds a #:sh keyword argument to wrap-qt-program
and adjusts callers to set this keyword argument appropriately.

Setting this argument appropriately is required for cross-compilation.
Otherwise, a bash for SYSTEM (as in --system=..) is used instead
of a bash for TARGET (as in --target=...).

I didn't test building some qt programs with this patch series yet,
as I'm currently waiting on the substitute servers to catch up
with recent changed to core-updates (to avoid building a tower
of rusts). I'll do that later.

Greetings,
Maxime

Hi Maxime,

Maxime Devos <maximedevos@telenet.be> writes:

> Hi guix,
>
> This patch series adds a #:sh keyword argument to wrap-qt-program
> and adjusts callers to set this keyword argument appropriately.
>
> Setting this argument appropriately is required for cross-compilation.
> Otherwise, a bash for SYSTEM (as in --system=..) is used instead
> of a bash for TARGET (as in --target=...).
>
> I didn't test building some qt programs with this patch series yet,
> as I'm currently waiting on the substitute servers to catch up
> with recent changed to core-updates (to avoid building a tower
> of rusts). I'll do that later.
>
> Greetings,
> Maxime
>
> From 27d42f25f54b16f382e18b9ef0fb202fb00da90d Mon Sep 17 00:00:00 2001
> From: Maxime Devos <maximedevos@telenet.be>
> Date: Sat, 5 Jun 2021 11:02:16 +0200
> Subject: [PATCH 1/8] qt-utils: Allow overriding the shell interpreter in
>  'wrap-qt-program'.
>
> * guix/build/qt-utils.scm (wrap-qt-program): Introduce a #:sh keyword
>   argument and pass it to 'wrap-program'.

If bash-minimal is added to inputs as we do for other packages making
use of wrap-program, we don't need to do more, no?  Why do we need to
explicit the argument here?

>> Subject: [PATCH 1/8] qt-utils: Allow overriding the shell interpreter in
>>  'wrap-qt-program'.
>>
>> * guix/build/qt-utils.scm (wrap-qt-program): Introduce a #:sh keyword
>>   argument and pass it to 'wrap-program'.
>
> If bash-minimal is added to inputs as we do for other packages making
> use of wrap-program, we don't need to do more, no?  Why do we need to
> explicit the argument here?

Post-hoc reason (for the first patch): wrap-program has #:sh argument, wrap-qt-program doesn’t, which is inconsistent.

For the rest (to be clear I think the remaining patches can be removed):

Right, technically we don’t. The reason is to make sure that it’s the bash from inputs instead of the bash native-inputs. Currently, at first it gets the (wrong) native bash, and later on this is fixed up by the patch-shebangs phase, IIRC.

However, (IIRC) that behaviour is a bug – patch-shebangs is for /usr/bin/… -> /gnu/store/… stuff – if the code “make install” or the like already set a proper /gnu/store/… shebang, why automatically change it to something else? Presumably it set it to the right interpreter, and now patch-shebangs might autocorrupt it.

Another problem: there might not even be a patch-shebangs phase, uses of wrap-program, wrap-qt-program and the phase of the qt-build-system that uses wrap-qt-program (IIRC there exists such a phase) should be usable in isolation. Also, there is a hidden assumption that the uses of wrap-program are _before_ the shebang patching, whereas it might be run afterwards as wll.

Instead, I think it’s better for the uses of ‘wrap-program’ to directly set it to the _right_ bash.
That’s what the #:sh argument is for, but #:sh is set to by default (which “bash”), which is incorrect. Hence, #:sh needs to be set explicitly, and hence wrap-qt-program needs a #:sh argument or the like to pass on to wrap-program.

That said, I don’t think all this explicit #:sh is appropriate either – it would need to be repeated for every single package definition refering to wrap-program, etc.. Instead, for the future, I’d propose to eliminate the argument list of phases, turning phase procedures in phase thunks and stuffing the old arguments in parameter objects instead.

Then, the #:sh of ‘wrap-program’ could default to (search-input-file (inputs) “bin/inputs”) – automatically correct (without needing patch-shebangs) both for native and cross-compilation, and when cross-compiling without “bash” in (implicit) inputs, it automatically errors out (instead of doing the wrong thing as done currently).

The phases would also be a bit less verbose to write – (lambda* (#:key this that #:allow-other-keys) (proc this) stuff …) could become (lambda () (proc) stuff …).

(The ‘procedure’ syntax (inputs) for parameter objects might not be the best here, but that’s nothing some bikeshedding over the precise syntax can’t fix.)

Bst regards,
Maxime Devos

(p.s. I received the mails for the other patches but I’m not responding at the time – not active with Guix currently, and borrowing another computer because of repairs.)

Hi Maxime,

M <maximedevos@telenet.be> writes:

>>> Subject: [PATCH 1/8] qt-utils: Allow overriding the shell interpreter in
>>>  'wrap-qt-program'.
>>>
>>> * guix/build/qt-utils.scm (wrap-qt-program): Introduce a #:sh keyword
>>>   argument and pass it to 'wrap-program'.
>>
>> If bash-minimal is added to inputs as we do for other packages making
>> use of wrap-program, we don't need to do more, no?  Why do we need to
>> explicit the argument here?
>
> Post-hoc reason (for the first patch): wrap-program has #:sh argument,
> wrap-qt-program doesn’t, which is inconsistent.

Good point.

> For the rest (to be clear I think the remaining patches can be removed):

OK, good.

> Right, technically we don’t. The reason is to make sure that it’s the
> bash from inputs instead of the bash native-inputs. Currently, at
> first it gets the (wrong) native bash, and later on this is fixed up
> by the patch-shebangs phase, IIRC.

I see.

> However, (IIRC) that behaviour is a bug – patch-shebangs is for
> /usr/bin/… -> /gnu/store/… stuff – if the code “make install” or the
> like already set a proper /gnu/store/… shebang, why automatically
> change it to something else? Presumably it set it to the right
> interpreter, and now patch-shebangs might autocorrupt it.

Ah!  Interesting.  I haven't seen any report for such bug.

> Another problem: there might not even be a patch-shebangs phase, uses
> of wrap-program, wrap-qt-program and the phase of the qt-build-system
> that uses wrap-qt-program (IIRC there exists such a phase) should be
> usable in isolation. Also, there is a hidden assumption that the uses
> of wrap-program are _before_ the shebang patching, whereas it might be
> run afterwards as wll.
>
> Instead, I think it’s better for the uses of ‘wrap-program’ to directly set it to the _right_ bash.
> That’s what the #:sh argument is for, but #:sh is set to by default
> (which “bash”), which is incorrect. Hence, #:sh needs to be set
> explicitly, and hence wrap-qt-program needs a #:sh argument or the
> like to pass on to wrap-program.
>
> That said, I don’t think all this explicit #:sh is appropriate either
> – it would need to be repeated for every single package definition
> refering to wrap-program, etc.. Instead, for the future, I’d propose
> to eliminate the argument list of phases, turning phase procedures in
> phase thunks and stuffing the old arguments in parameter objects
> instead.
>
> Then, the #:sh of ‘wrap-program’ could default to (search-input-file
> (inputs) “bin/inputs”) – automatically correct (without needing
> patch-shebangs) both for native and cross-compilation, and when
> cross-compiling without “bash” in (implicit) inputs, it automatically
> errors out (instead of doing the wrong thing as done currently).
>
> The phases would also be a bit less verbose to write – (lambda* (#:key
> this that #:allow-other-keys) (proc this) stuff …) could become
> (lambda () (proc) stuff …).
>
> (The ‘procedure’ syntax (inputs) for parameter objects might not be
> the best here, but that’s nothing some bikeshedding over the precise
> syntax can’t fix.)

Thanks for sharing your perspective on this.  It's an interesting idea
that you are proposing, but it'd entail a massive effort to port the
code base.

Cheers!  Enjoy whatever hack you are currently pursuing!

Hi again,

Maxime Devos <maximedevos@telenet.be> writes:

> Hi guix,
>
> This patch series adds a #:sh keyword argument to wrap-qt-program
> and adjusts callers to set this keyword argument appropriately.
>
> Setting this argument appropriately is required for cross-compilation.
> Otherwise, a bash for SYSTEM (as in --system=..) is used instead
> of a bash for TARGET (as in --target=...).
>
> I didn't test building some qt programs with this patch series yet,
> as I'm currently waiting on the substitute servers to catch up
> with recent changed to core-updates (to avoid building a tower
> of rusts). I'll do that later.
>
> Greetings,
> Maxime
>
> From 27d42f25f54b16f382e18b9ef0fb202fb00da90d Mon Sep 17 00:00:00 2001
> From: Maxime Devos <maximedevos@telenet.be>
> Date: Sat, 5 Jun 2021 11:02:16 +0200
> Subject: [PATCH 1/8] qt-utils: Allow overriding the shell interpreter in
>  'wrap-qt-program'.
>
> * guix/build/qt-utils.scm (wrap-qt-program): Introduce a #:sh keyword
>   argument and pass it to 'wrap-program'.

I see this patch was already merged by Mathieu in
77b98bf1874aac8ed447e9f0b0ee0865a1d652ba; thus, I'm closing this ticket.

Thank you!