* [bug#66870] Review
[not found] ` <2460289E-B408-41CD-B44B-8DCCB638A482@disroot.org>
@ 2023-11-29 14:48 ` Gabriel Wicki
2023-11-30 8:15 ` sughosha via Guix-patches via
2023-11-30 10:09 ` Sughosha via Guix-patches via
0 siblings, 2 replies; 3+ messages in thread
From: Gabriel Wicki @ 2023-11-29 14:48 UTC (permalink / raw)
To: Sughosha; +Cc: 66870
Hi
So i've de-frizzled the patches by renaming #66870 to "[PATCH 0/6] Add
yabridge."
[PATCH 1/6] looks good to me! Have you checked the packages depending
on asio still build and work?
You can check the dependents with this command:
$ guix refresh -l asio
Building the following 8 packages would ensure 16 dependent packages are rebuilt: emacs-ob-sclang@0.1-1.cd3f3c8 cl-collider@2018.7.15-0.a469088 ecl-cl-collider@2018.7.15-0.a469088 gnome-arcade@0.240 widelands@1.1 musikcube@3.0.1 jami@20230323.0 restbed@4.8
I checked and they all still build fine :)
[PATCH 2/6] LGTM.
[PATCH 3/6] Looks goot, although I am a bit confused WRT the comment for
the (recursive #t) clause in the (source ..) field (indicating we need
to recursively copy the repo for the tests to complete) and argument
(#:tests? #f) indicating that the tests wouldn't be run.
[PATCH 4/6] LGTM.
[PATCH 5/6] LGTM.
[PATCH 6/6] Wow, now this is a big one! Apart from the following
comments this looks good to me!
- I'm not sure whether your (let ((arch))) clause isn't an abuse; you
test whether (%current-system) is either x86_64-linux or aarch64-linux
and set it to x86_64-unix and i386-unix otherwise. Does this actually
work for aarch64-linux systems? You could make use of the
target-64-bit? function in (guix utils) and only allow the package to
build on x86 systems. Also, you inherit wine64's supported-systems
field - which only lists x86_64-linux. Building for i368 seems to be
unsupported by our wine packages (or wine in general?)
- I'm a bit confused about your patch series adding clap 1.1.9 -
although yabridge seems to need 1.1.7. Why not add clap@1.1.7 with this
series and with the next upgrade of yabridge also update clap?
- Similar with input vst3sdk: Why not add it as a separate package?
If I interpret the situation correctly, using default definitions of
these inputs could also let you simplify your native-inputs list to the
new style.
All packages built fine and running $(guix style) didn't change
anything.
$(guix lint) on the patched packages gave the following output:
~/src/guix/gnu/packages/networking.scm:3431:15: asio@1.28.0: permanent
redirect from https://think-async.com/Asio to
https://think-async.com/Asio/
fetching CVE database for 2023...
~/src/guix/gnu/packages/networking.scm:3417:5: asio@1.28.0:
source not archived on Software Heritage and missing from the Disarchive
database
~/src/guix/gnu/packages/cpp.scm:2311:13: function2@4.2.3:
can be upgraded to 4.2.4
~/src/guix/gnu/packages/audio.scm:2724:13: clap@1.1.9: can
be upgraded to 1.1.10
~/src/guix/gnu/packages/audio.scm:6002:0: yabridge@5.0.5:
line 6002 is way too long (95 characters)
~/src/guix/gnu/packages/audio.scm:6013:0: yabridge@5.0.5:
line 6013 is way too long (100 characters)
Please send in a updated patch-set.
Thanks for your time and effort!
g
^ permalink raw reply [flat|nested] 3+ messages in thread
* [bug#66870] Review
2023-11-29 14:48 ` [bug#66870] Review Gabriel Wicki
@ 2023-11-30 8:15 ` sughosha via Guix-patches via
2023-11-30 10:09 ` Sughosha via Guix-patches via
1 sibling, 0 replies; 3+ messages in thread
From: sughosha via Guix-patches via @ 2023-11-30 8:15 UTC (permalink / raw)
To: Gabriel Wicki; +Cc: 66870
Hi,
Thanks for the review.
> So i've de-frizzled the patches by renaming #66870 to "[PATCH 0/6] Add
> yabridge."
Thank you.
> [PATCH 1/6] looks good to me! Have you checked the packages depending
> on asio still build and work?
I checked only musikcube, jami and widelands and they worked fine.
Others
must work as well.
> [PATCH 3/6] Looks goot, although I am a bit confused WRT the comment
> for
> the (recursive #t) clause in the (source ..) field (indicating we need
> to recursively copy the repo for the tests to complete) and argument
> (#:tests? #f) indicating that the tests wouldn't be run.
Removed "(recursive #t)".
> - I'm not sure whether your (let ((arch))) clause isn't an abuse; you
> test whether (%current-system) is either x86_64-linux or aarch64-linux
> and set it to x86_64-unix and i386-unix otherwise. Does this actually
> work for aarch64-linux systems? You could make use of the
> target-64-bit? function in (guix utils) and only allow the package to
> build on x86 systems. Also, you inherit wine64's supported-systems
> field - which only lists x86_64-linux. Building for i368 seems to be
> unsupported by our wine packages (or wine in general?)
Removed the substitute as it is no more needed.
> - I'm a bit confused about your patch series adding clap 1.1.9 -
> although yabridge seems to need 1.1.7. Why not add clap@1.1.7 with this
> series and with the next upgrade of yabridge also update clap?
Moved clap@1.1.7 to a seperate definition.
> - Similar with input vst3sdk: Why not add it as a separate package?
> If I interpret the situation correctly, using default definitions of
> these inputs could also let you simplify your native-inputs list to the
> new style.
This vst3sdk is not the original one. It is found in a different
repository than the original one and is suppose to clone as subproject.
So
it does not make sense to package it in a seperate definition.
> $(guix lint) on the patched packages gave the following output:
>
> ~/src/guix/gnu/packages/networking.scm:3431:15: asio@1.28.0: permanent
> redirect from https://think-async.com/Asio to
> https://think-async.com/Asio/
> fetching CVE database for 2023...
> ~/src/guix/gnu/packages/networking.scm:3417:5: asio@1.28.0:
> source not archived on Swith "(let ((arch)))"oftware Heritage and
> missing from the Disarchive
> database
> ~/src/guix/gnu/packages/cpp.scm:2311:13: function2@4.2.3:
> can be upgraded to 4.2.4
> ~/src/guix/gnu/packages/audio.scm:2724:13: clap@1.1.9: can
> be upgraded to 1.1.10
> ~/src/guix/gnu/packages/audio.scm:6002:0: yabridge@5.0.5:
> line 6002 is way too long (95 characters)
> ~/src/guix/gnu/packages/audio.scm:6013:0: yabridge@5.0.5:
> line 6013 is way too long (100 characters)
Fixed and updated these packages.
I will submit the v2 patches with the above changes.
Regards,
Sughosha
^ permalink raw reply [flat|nested] 3+ messages in thread
* [bug#66870] Review
2023-11-29 14:48 ` [bug#66870] Review Gabriel Wicki
2023-11-30 8:15 ` sughosha via Guix-patches via
@ 2023-11-30 10:09 ` Sughosha via Guix-patches via
1 sibling, 0 replies; 3+ messages in thread
From: Sughosha via Guix-patches via @ 2023-11-30 10:09 UTC (permalink / raw)
To: Gabriel Wicki; +Cc: 66870
[-- Attachment #1: Type: text/plain, Size: 204 bytes --]
A small thing to remove in [PATCH v2 7/7]:
in yabridge definition, "(modules '((guix build utils)))" inside "(source)" is not needed. If you can remove it before merging, it would be great.
Thank you.
[-- Attachment #2: Type: text/html, Size: 273 bytes --]
^ permalink raw reply [flat|nested] 3+ messages in thread
end of thread, other threads:[~2023-11-30 10:11 UTC | newest]
Thread overview: 3+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
[not found] <emsvoocbxrkac7u37ljc2g73icjg4pzfffjy7asikd22mltau6@fiag3vi7r4ln>
[not found] ` <2460289E-B408-41CD-B44B-8DCCB638A482@disroot.org>
2023-11-29 14:48 ` [bug#66870] Review Gabriel Wicki
2023-11-30 8:15 ` sughosha via Guix-patches via
2023-11-30 10:09 ` Sughosha via Guix-patches via
Code repositories for project(s) associated with this external index
https://git.savannah.gnu.org/cgit/guix.git
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.