GNU bug report logs - #66870
[PATCH 0/6] Add yabridge.

Previous Next

Full log

View this message in rfc822 format

From: Gabriel Wicki <gabriel <at> erlikon.ch>
To: Sughosha <sughosha <at> disroot.org>
Cc: 66870 <at> debbugs.gnu.org
Subject: [bug#66870] Review
Date: Wed, 29 Nov 2023 15:48:31 +0100

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 <at> 0.1-1.cd3f3c8 cl-collider <at> 2018.7.15-0.a469088 ecl-cl-collider <at> 2018.7.15-0.a469088 gnome-arcade <at> 0.240 widelands <at> 1.1 musikcube <at> 3.0.1 jami <at> 20230323.0 restbed <at> 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 <at> 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 <at> 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 <at> 1.28.0:
source not archived on Software Heritage and missing from the Disarchive
database
~/src/guix/gnu/packages/cpp.scm:2311:13: function2 <at> 4.2.3:
can be upgraded to 4.2.4
~/src/guix/gnu/packages/audio.scm:2724:13: clap <at> 1.1.9: can
be upgraded to 1.1.10
~/src/guix/gnu/packages/audio.scm:6002:0: yabridge <at> 5.0.5:
line 6002 is way too long (95 characters)
~/src/guix/gnu/packages/audio.scm:6013:0: yabridge <at> 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

Previous Next

GNU bug tracking system
Copyright (C) 1999 Darren O. Benham, 1997,2003 nCipher Corporation Ltd, 1994-97 Ian Jackson.