Re: branch master updated: gnu: Add warsow-qfusion.

Re: branch master updated: gnu: Add warsow-qfusion.

[Marius Bakke]

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

Hi Pierre,

guix-commits@gnu.org writes:

> commit 39f1806ca1d04b9aee70e897e06466aadbbee152
> Author: Pierre Neidhardt <mail@ambrevar.xyz>
> AuthorDate: Thu Apr 9 15:56:42 2020 +0200
>
>     gnu: Add warsow-qfusion.
>     
>     * gnu/local.mk (warsow-qfusion): New variable.

This commit message is weird.

> ---
>  gnu/local.mk                      |  1 +
>  gnu/packages/game-development.scm | 78 +++++++++++++++++++++++++++++++++++++++
>  2 files changed, 79 insertions(+)

warsow-qfusion-fix-bool-return-type.patch is missing, breaking the
build of both Guix and this package.

> +(define-public warsow-qfusion
> +  ;; As of 2020-04-09, the latest stable version 2.1.0 is deprecated.
> +  ;; The 2.5 beta as published on the homepage is commit
> +  ;; c4de15df559410aff0ca6643724e24cddb0ecbbd
> +  (let ((commit "c4de15df559410aff0ca6643724e24cddb0ecbbd")
> +        (arch (match (or (%current-target-system)
> +                         (%current-system))
> +                ("x86_64-linux" "x86_64")
> +                ("i686-linux" "i386")
> +                (_ ""))))

Don't do this if you just need to determine architecture at build time.

> +           (add-after 'install 'really-install
> +             (lambda* (#:key outputs #:allow-other-keys)
> +               (let ((out (assoc-ref outputs "out")))
> +                 (install-file (string-append "../source/build/basewsw/libgame_"
> +                                              ,arch ".so")
> +                               (string-append out "/lib/"))
> +                 (install-file (string-append "../source/build/libui_" ,arch ".so")
> +                               (string-append out "/lib/"))

Add it here instead.  There is also no need for the fallback value
as the package apparently only supports i686 and x86_64 according to
supported-systems.

> +      (synopsis "Warsow's fork of qfusion, the id Tech 2 derived game engine")
> +      (supported-systems '("i686-linux" "x86_64-linux"))
> +      (description
> +       "This package contains Warsow's fork of qfusion, the id Tech 2 derived
> +game engine.  id Tech 2 is the engine originally behind Quake 2.")
> +      (license license:gpl2+))))

Nit-pick: could you put description immediately after synopsis?  Also,
please avoid restating the synopsis in the description, but try to
expand on it.  I.e. what is Warsow?  I thought it was a city!

Meanwhile I've reverted the commit so that 'make' works again.

Thanks!

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

Re: branch master updated: gnu: Add warsow-qfusion.

[Pierre Neidhardt]

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

Hi Marius,

Sorry, I rebased and edited a few things, some changes slipped through
and resulted in this terrible commit :(

> +  (let ((commit "c4de15df559410aff0ca6643724e24cddb0ecbbd")
> +        (arch (match (or (%current-target-system)
> +                         (%current-system))
> +                ("x86_64-linux" "x86_64")
> +                ("i686-linux" "i386")
> +                (_ ""))))
> Don't do this if you just need to determine architecture at build time.

Just to be sure, you suggest moving this inside the 'really-install
phase, right?

Will fix as soon as possible.

Sorry for the mess again!

Cheers!

-- 
Pierre Neidhardt
https://ambrevar.xyz/

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

Re: branch master updated: gnu: Add warsow-qfusion.

[Marius Bakke]

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

Pierre Neidhardt <mail@ambrevar.xyz> writes:

> Hi Marius,
>
> Sorry, I rebased and edited a few things, some changes slipped through
> and resulted in this terrible commit :(
>
>> +  (let ((commit "c4de15df559410aff0ca6643724e24cddb0ecbbd")
>> +        (arch (match (or (%current-target-system)
>> +                         (%current-system))
>> +                ("x86_64-linux" "x86_64")
>> +                ("i686-linux" "i386")
>> +                (_ ""))))
>> Don't do this if you just need to determine architecture at build time.
>
> Just to be sure, you suggest moving this inside the 'really-install
> phase, right?

Yes.  Otherwise it gets needlessly evaluated by the Guix "front-end"
every time one uses 'guix search' etc -- even on unsupported
architectures.  It's better to do it at build time, especially when it
is only used within a single phase.

On a related note, the 'ccl' package has a similar issue, and actually
produces an invalid derivation on aarch64 and armhf.  It can be seen by
running e.g. 'guix weather -s armhf-linux'.  I've been meaning to file a
bug about it forever, but typically just comment the package and forget
about it.  :-/

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

Re: branch master updated: gnu: Add warsow-qfusion.

[Pierre Neidhardt]

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

Marius Bakke <mbakke@fastmail.com> writes:

> On a related note, the 'ccl' package has a similar issue, and actually
> produces an invalid derivation on aarch64 and armhf.  It can be seen by
> running e.g. 'guix weather -s armhf-linux'.  I've been meaning to file a
> bug about it forever, but typically just comment the package and forget
> about it.  :-/

I don't understand why the `ccl' package would have an invalid
derivation on armhf-linux: All `match' seem to be correct, but I can't
test at the moment.  Are you sure this is a similar issue?

-- 
Pierre Neidhardt
https://ambrevar.xyz/

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

Re: branch master updated: gnu: Add warsow-qfusion.

[Pierre Neidhardt]

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

Marius Bakke <mbakke@fastmail.com> writes:

>>> +  (let ((commit "c4de15df559410aff0ca6643724e24cddb0ecbbd")
>>> +        (arch (match (or (%current-target-system)
>>> +                         (%current-system))
>>> +                ("x86_64-linux" "x86_64")
>>> +                ("i686-linux" "i386")
>>> +                (_ ""))))
>>> Don't do this if you just need to determine architecture at build time.
>>
>> Just to be sure, you suggest moving this inside the 'really-install
>> phase, right?
>
> Yes.  Otherwise it gets needlessly evaluated by the Guix "front-end"
> every time one uses 'guix search' etc -- even on unsupported
> architectures.  It's better to do it at build time, especially when it
> is only used within a single phase.

I don't understand the difference.

(match (or (%current-target-system)
           (%current-system))
   ("x86_64-linux" "x86_64")
   ("i686-linux" "i386"))

must be unquoted inside the phase, so it's evaluated when the package
is and not at build time, isn't it?
Else how do you match against the build architecture at build time?

-- 
Pierre Neidhardt
https://ambrevar.xyz/

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

Re: branch master updated: gnu: Add warsow-qfusion.

[Marius Bakke]

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

Pierre Neidhardt <mail@ambrevar.xyz> writes:

> Marius Bakke <mbakke@fastmail.com> writes:
>
>>>> +  (let ((commit "c4de15df559410aff0ca6643724e24cddb0ecbbd")
>>>> +        (arch (match (or (%current-target-system)
>>>> +                         (%current-system))
>>>> +                ("x86_64-linux" "x86_64")
>>>> +                ("i686-linux" "i386")
>>>> +                (_ ""))))
>>>> Don't do this if you just need to determine architecture at build time.
>>>
>>> Just to be sure, you suggest moving this inside the 'really-install
>>> phase, right?
>>
>> Yes.  Otherwise it gets needlessly evaluated by the Guix "front-end"
>> every time one uses 'guix search' etc -- even on unsupported
>> architectures.  It's better to do it at build time, especially when it
>> is only used within a single phase.
>
> I don't understand the difference.
>
> (match (or (%current-target-system)
>            (%current-system))
>    ("x86_64-linux" "x86_64")
>    ("i686-linux" "i386"))
>
> must be unquoted inside the phase, so it's evaluated when the package
> is and not at build time, isn't it?
>
> Else how do you match against the build architecture at build time?

You can use (lambda* (#:key target system #:allow-other-keys)) to access
those variables at build time.

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

Re: branch master updated: gnu: Add warsow-qfusion.

[Marius Bakke]

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

Pierre Neidhardt <mail@ambrevar.xyz> writes:

> Marius Bakke <mbakke@fastmail.com> writes:
>
>> On a related note, the 'ccl' package has a similar issue, and actually
>> produces an invalid derivation on aarch64 and armhf.  It can be seen by
>> running e.g. 'guix weather -s armhf-linux'.  I've been meaning to file a
>> bug about it forever, but typically just comment the package and forget
>> about it.  :-/
>
> I don't understand why the `ccl' package would have an invalid
> derivation on armhf-linux: All `match' seem to be correct, but I can't
> test at the moment.  Are you sure this is a similar issue?

Admittedly it's only tangentially related.  It is really easy to test by
running 'guix build -s armhf-linux --no-grafts -d ccl'.

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

Re: branch master updated: gnu: Add warsow-qfusion.

[Pierre Neidhardt]

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

Just learned something new!
Can't believe I never ran across a package using the "system" keyword
argument so far :)

I've fixed all your comments and pushed with 0b5bf61573c5491a52dfb70ea2b6e91ca61ef199.

Thanks!

-- 
Pierre Neidhardt
https://ambrevar.xyz/

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