[PATCH] gnu: Update zynaddsubfx to 3.0.6

Empty Message

Sorry, one small typos in the commit message.

Hello,

Sughosha <Sughosha@proton.me> writes:

Recursive git clones are typically synonymous with bundled libraries or

other undesired, hence it needs to be explained in a comment. If there

are bundled libraries, they should be packaged separately ideally, or if

that's too much work to do, at least a TODO should be added mentioning

which bundled libraries are currently used.

Please use gexps for new packages, e.g. (arguments (list #:make-flags

#~(list "CC=gcc" ...) ...))

Disabling the tests warrants a comment, which can be as simple as the ";no

test suite" inline comment.

Please use complete sentences for standalone comments, e.g. "There is no

configure script."

Ditto.

After this is turned into a gexp, you can use #$output to refer to the

main output directly.

Can't it use the fonts provided by the system? Packages shouldn't

include any font, ideally, as they're heavy and should be

user-configurable.

Why is this empty file needed? A comment would be useful.

Please order dependencies lexicographically.

Trailing #t are no longer needed; remove them.

Please mind the ordering.

Could you send a v2 with the requested changes? You can do so with 'git

send-email -v2 --to=57984@debbugs.gnu.org -1' from the checkout which

has this v2 as last commit.

Thank you!

Maxim

Hi,

Sughosha <Sughosha@proton.me> writes:

Also, please separate your commits; each package should go to their

individual commit.

Lastly, I see there exists a previous effort to package mruby-zest, with

similar outstanding comments as I made here; see: #40440

(https://issues.guix.gnu.org/40440). These comments should also be

taken into consideration.

Thanks,

Maxim

See comments by Maxim, and ...

> "CC=gcc"

When cross-compilation, you need to TARGET-gcc such that it is compiled

for the right architecture. This is done automatically by

(string-append "CC=" #$(cc-for-target)), which should have plenty of

examples in the source code.

Greetings,

Maxime.

Thank you Maxim and Maxime for your detailed review. I considered all of the changes you pointed out, and here are the attatchments of the new patch files. Regarding recursive clone, I packages the runtime dependencies seperately but I left the mruby gems, since they seem to be needed just as source (without needing to be compiled) as they are to be configured by mruby for mruby-zest. I am also not familier with mruby or ruby.

Thanks,

Sughosha

Oops, sorry for messing with the patches. I was switching branches and messed somewhere. Here are the proper patches.

Thank you.

Sughosha

Empty Message

On 15-10-2022 16:14, Sughosha wrote:

/me looks at revised patches now

On 24-10-2022 19:02, Maxime Devos wrote:

Now at the third patch

Now at patch 4.

> seperately but I left the mruby gems, since they seem to be needed

> just as source (without needing to be compiled) as they are to be

> configured by mruby for mruby-zest. [...]

Bundling things that aren't compiled is still bundling. Also, now both

mruby-zest and zynaddsubfx bundle rtosc, which is worse than a package

bundling a dependent that isn't used anywhere else.

I've looked at the latest patches and did some changes (see attached

patches), though the bundling still remains.

In nanovg, I changed the 'revision' to 0 -- its just a monotonically

increasing number for each package update, it's not a 'how many commits

have there been', see (guix)Version Numbers.

I also changed "ar" to #$(ar-for-target) -- using the 'wrong' ar

sometimes works, but not always, e.g. IIRC it failed when

cross-compiling from some 64-bit arch to a 32-bit ach?

Upstream appears to use 'premake4' as a build system, so I adjusted the

package definition accordingly.

I set the 'file-name' field for to be more informative.

---

I corrected the revision in font-entypo as with nanovg.

I expanded the description a little, and added a TODO

on building the font from source (it seems to have been

neglected for font packages because of node dependencies).

The license combination seemed a bit odd to me so I added

some more text there.

---

mruby-zest:

I changed 'TODO: package mruby gems separately' to 'TODO: unbundle mruby

gems' as that seems easier to search for. Some things were actually

packaged, so I removed those from 'deps' in a snippet. It turned out

that the Makefile tried to build mruby, so I patched that out.

As mruby isn't included anymore, you could give removing some of the

bundled mruby gems a try, especially given that they aren't actually

installed.

I removed '#:tests? #false' because a test suite exists -- the makefile

has a 'test' and 'rtest' target. I choose the 'rtest' target (because

'test' is for testing mruby, not mruby-zest). It turned out the test

suite requires 'ruby-ruby-prof' so I added that as a native-input.

You have done #$some-input in the 'install' phase -- package

transformation don't know to adjust those, so I replaced it with a

search-input-file equivalent.

Putting the fonts in the output of mruby-zest doesn't lookk quite right

to me, so I instead moved things to a post-unpack phase and used

'substitute*' instead of 'symlink'.

I noticed that pcre was bundled (deps/mruby-regexp-pcre/pcre'), so I

removed that.

By looking at

https://github.com/archlinux/svntogit-community/tree/packages/zynaddsubfx/trunk,

I noticed it looks for libzest.so in /opt/zyn-fusion/libzest.so, which

is incorrect in Guix (and other distros too).

I removed the wrapping because:

* no reason was listed for wrapping

* presumably the incorrect reference was the thing that

LD_LIBRARY_PATH was a work-around for, but that reference has now

been corrected.

* LD_LIBRARY_PATH is leaky and hence to be avoided

(if zest spawns a subprocess, then that subprocess would get

LD_LIBRARY_PATH too)

As a bonus, this allowed removing 'bash-minimal' from the inputs.

From

https://github.com/archlinux/svntogit-community/blob/packages/zynaddsubfx/trunk/zynaddsubfx-mruby-zest-build-3.0.6-system_wide_location.patch,

I noticed that apparently some references to schema/test.json and

MainWindow.qml were incorrect. These are now patched.

I simplified the unbundling substitutions a little.

For example, it turned out that build_config.rb did not need any

substitutions somehow.

The makefile uses pkg-config instead of TARGET-pkg-config, which is

incorrect when cross-compiling, so I patched that.

On the fonts: I noticed that the fonts don't become part of the closure

(with "guix gc --references"). As such, maybe the source files that

use the fonts are actually unused. I propose to give installing the

examples that use the fonts a try, or alternatively explicitly choose

to not install the (font-using) examples.

---

I changed the version of zynaddsubfx from (package-version mruby-zest)

to "3.0.6", otherwise the package would break after a mruby-zest update.

I re-added the comment of 'remove-sse-flags-from-generic-target' that

was removed.

I changed #$mruby-zest to a search-input-file equivalent, for the same

reason as with previous patches.

You forgot to mention the home page, so I adjusted the commit message.

You are fixing a file name, not a path, so I adjusted the new phase name.

By removing the 'ntk' input, zynaddsubfx now has one less interface,

so I re-added it, with a comment.

It appeared that doxygen documentation wasn't actually build

(there is no .html in the output, and the configuraiton script

complained about some missing component), so I removed the new doxygen

(native-)input.

bash completions weren't installed in the new version even though they

were in the previous version, so I made some changes to support that.

I adjusted the substitution of libzest.so to substitute all cases,

in case somehow the first dlopen fails.

I compared the diff between old and new source code, there doesn't

appear to be anything 'suspicious' though its just a cursory look

and it would be easy to hide things:

guix shell diffoscope -- diffoscope

/gnu/store/9j09zj472211nrrs5jmyxdsq2lpfd36q-zynaddsubfx-3.0.5.tar.bz2

/gnu/store/xyjiq3nmk372ap4vq7sl7n7f9rc5xshs-zynaddsubfx-3.0.6.tar.bz2

Hi Maxime,

Impressive work. Are there outstanding things worthy of being fixed in

your opinion, or do you think this version is ready to be merged and we

can iterate improvements on top in time? To my cursory look, I'd think

it looks in a good enough shape, but I thought I'd ask before merging.

--

Thanks,

Maxim

On 27-10-2022 15:18, Maxim Cournoyer wrote:

My primary concern is that while it builds, I haven't tested whether

zynaddsubfx still works after my modifications. There are two secondary

concerns:

(*) The bundling. Both zynaddsubfx and the new mruby-zest bundle rtosc.

Having multiple versions of the same library in the same program is

known to potentially cause trouble, see e.g.

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

The other bundling could be considered as 'can iterate improvements on

top over time' I suppose.

(*) The fonts don't appear to be actually used (at least in the current

configuration), so as you appear to prefer no font inputs, that seems

feasible to me:

(Alternatively, maybe I messed up the substitutions)

Greetings,

Maxime.

I opened a new issue #63254 with the new patches to add mruby-zest,

unbundle nanovg and rtosc and then patch the source files:

Sughosha

merge 57984 63254

quit