Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Update ZynAddSubFX #315

Closed
diizy opened this issue Feb 14, 2014 · 26 comments
Closed

Update ZynAddSubFX #315

diizy opened this issue Feb 14, 2014 · 26 comments

Comments

@diizy
Copy link
Contributor

diizy commented Feb 14, 2014

Our version of Zyn is really outdated, there's a much newer version with more functionality available.

So as discussed on the mailing list, we need somebody to port the latest version of Zyn to LMMS, and get it working. And as always, do it in a way that doesn't break anything...

Any takers?

@eagles051387
Copy link

Is this really release critical for 1.0.0

On Fri, Feb 14, 2014 at 6:59 PM, Vesa V notifications@github.com wrote:

Our version of Zyn is really outdated, there's a much newer version with
more functionality available.

So as discussed on the mailing list, we need somebody to port the latest
version of Zyn to LMMS, and get it working. And as always, do it in a way
that doesn't break anything...

Any takers?

Reply to this email directly or view it on GitHubhttps://github.com//issues/315
.

Jonathan Aquilina

@diizy
Copy link
Contributor Author

diizy commented Feb 15, 2014

On 02/15/2014 10:16 AM, eagles051387 wrote:

Is this really release critical for 1.0.0

Probably not, no. But it's something that should be started work on.

@eagles051387
Copy link

I agree there. If you are looking at working on this I would create a fork
and ask toby to create a branch for it.

On Sat, Feb 15, 2014 at 9:45 AM, Vesa V notifications@github.com wrote:

On 02/15/2014 10:16 AM, eagles051387 wrote:

Is this really release critical for 1.0.0

Probably not, no. But it's something that should be started work on.

Reply to this email directly or view it on GitHubhttps://github.com//issues/315#issuecomment-35150517
.

Jonathan Aquilina

@diizy
Copy link
Contributor Author

diizy commented Feb 15, 2014

On 02/15/2014 10:48 AM, eagles051387 wrote:

I agree there. If you are looking at working on this I would create a fork
and ask toby to create a branch for it.

I'm not, not right now anyway. I have other stuff on my todo list, so
I'd prefer for someone else to tackle this one.

@tobydox
Copy link
Member

tobydox commented Feb 15, 2014

I can work on it though I would not target it for 1.0.0.

@tobydox
Copy link
Member

tobydox commented Feb 16, 2014

About 50% done - stay tuned!

@mikobuntu
Copy link
Contributor

Toby will this update include the NTK graphics iirc they are an extension to the FLTK?

thanks Mikobuntu ;)

Date: Sun, 16 Feb 2014 07:14:27 -0800
From: notifications@github.com
To: lmms@noreply.github.com
Subject: Re: [lmms] Update ZynAddSubFX (#315)

About 50% done - stay tuned!


Reply to this email directly or view it on GitHub.

@tobydox
Copy link
Member

tobydox commented Feb 16, 2014

I don't think so. Does it provide any beneficials? I guess that would require additional external dependencies?

@mikobuntu
Copy link
Contributor

no benefits other than just graphical changes, yeah which require the NTK toolkit to be installed i think along with the FLTK , libntk0.0 is only 1260kb and the dev files are 1331kb, whichever is needed, so they are not large, and iirc a switch in the build or configure is all that is needed to make the change.

thanks Mikobuntu ;)

Date: Sun, 16 Feb 2014 09:51:09 -0800
From: notifications@github.com
To: lmms@noreply.github.com
CC: chrissy.mc.1@hotmail.co.uk
Subject: Re: [lmms] Update ZynAddSubFX (#315)

I don't think so. Does it provide any beneficials? I guess that would require additional external dependencies?


Reply to this email directly or view it on GitHub.

@lukas-w lukas-w removed the bug label Feb 16, 2014
@tobydox
Copy link
Member

tobydox commented Feb 18, 2014

Ok, for the time being I'm finished. The updated ZynAddSubFX can be found in the "stable-0.4-zynaddsubfx-update" branch. The customized version of ZASF source code itself is integrated via a git submodule. After cloning the LMMS repository or checking out the new branch, you have to run

git submodule init
git submodule update

Later you only have to issue the update command every time it is touched.

@tobydox tobydox closed this as completed Feb 18, 2014
@lukas-w
Copy link
Member

lukas-w commented Feb 18, 2014

Please do not use git submodule. With submodules, you can't just clone the repo any more, instead every user is forced to run those extra commands, downloading the entire source from GitHub won't work any more etc.
On top of that, quoting @dobey:

Please do not use git submodules at all, ever. It's probably worse than the current solution. The bzr-git import module does not support submodules, and this would certainly mean I would have to disable any daily builds in my PPA for Ubuntu.

Alternatives are e.g. git subtrees (though not enabled in every distribution) or, which I have stumbled over a few days ago, this method by Felix Geisendörfer, which I find most elegant.
Both would still include the sources in the tree, but at least they make any extra steps for cloning unnecessary. Especially the latter one is a lot easier to use.

@tobydox
Copy link
Member

tobydox commented Feb 18, 2014

Yes, I'm not satisfied with submodules approach either but subtree is not an option as it seems unavailable for many people (including me) per default. The fake submodules method is nice for end users but sucks for development the same way - you always have to commit twice. Difference to submodules: you commit the actual content twice instead of just updating a reference. This is redundant and can lead to more confusion in the end. I think that everyone who compiles LMMS from Git is also capable of running the submodules update command.

What we can improve is a to add a check in plugins/zynaddsubfx/CMakeLists.txt which looks for the real zynaddsubfx source directory. If it does not exist, it will print a message informing you about the missing external component.

@eagles051387
Copy link

Can i make a suggestion.

Can we have it during the build if it needs to pull from another repository
such as submodules it can do that automatically (aka hands free) I have
seen libre office do this pull external things it needs to build in terms
of libraries etc from other locations.

On Tue, Feb 18, 2014 at 10:17 PM, Tobias Doerffel
notifications@git.luolix.topwrote:

Yes, I'm not satisfied with submodules approach either but subtree is not
an option as it seems unavailable for many people (including me) per
default. The fake submodules method is nice for end users but sucks for
development the same way - you always have to commit twice. Difference to
submodules: you commit the actual content twice instead of just updating a
reference. This is redundant and can lead to more confusion in the end. I
think that everyone who compiles LMMS from Git is also capable of running
the submodules update command.

What we can improve is a to add a check in
plugins/zynaddsubfx/CMakeLists.txt which looks for the real zynaddsubfx
source directory. If it does not exist, it will print a message informing
you about the missing external component.

Reply to this email directly or view it on GitHubhttps://github.com//issues/315#issuecomment-35435029
.

Jonathan Aquilina

@dobey
Copy link

dobey commented Feb 18, 2014

@eagles051387 No, pulling arbitrary things from the network during build is worse idea.

@tobydox What is the purpose of any changes that are made to Zynaddsubfx to have it work in the context of an lmms plug-in? Is it to enable IPC control or something similar? Why are the changes not being pushed upstream?

@lukas-w
Copy link
Member

lukas-w commented Feb 18, 2014

@tobydox
Hm, those are some valid points. I don't think the "commit twice" thing is that bad though.
I imagine the workflow with fake submodules like this: Active development could take place on LMMS/ZynAddSubFx. Every now and then, when there are great changes in the zasf repo, we make a single commit in LMMS/lmms to update to a newer version and pull in new changes.
It'd be a real shame if the PPA didn't work any more because of submodules.

@eagles051387
While technically possible with CMake (and I have to admit I find this kind of sexy), it would always require a working internet connection when building, which is not always given. I'm not even sure if Travis has this.

@dobey
Copy link

dobey commented Feb 18, 2014

@lukas-w The Launchpad builders block network access when building packages, for security reasons. I would expect Travis to do the same, but it might not (pulling junk from network is common practice with python/ruby things, and there are a lot of such projects using Travis).

@eagles051387
Copy link

Not true lukas, libreoffice they dont use git but if you pull from lets say
an ftp server its on your pc in a particular location where the source code
would be

On Tue, Feb 18, 2014 at 10:31 PM, Lukas W notifications@github.com wrote:

@tobydox https://github.com/tobydox
Hm, those are some valid points. I don't think the "commit twice" thing is
that bad though.
I imagine the workflow with fake submodules like this: Active development
could take place on LMMS/ZynAddSubFx. Every now and then, when there are
great changes in the zasf repo, we make a single commit in LMMS/lmms to
update to a newer version and pull in new changes.
It'd be a real shame if the PPA didn't work any more because of submodules.

@eagles051387 https://github.com/eagles051387
While technically possible with CMake (and I have to admit I find this
kind of sexy), it would always require a working internet connection when
building, which is not always given. I'm not even sure if Travis has this.

Reply to this email directly or view it on GitHubhttps://github.com//issues/315#issuecomment-35436574
.

Jonathan Aquilina

@lukas-w
Copy link
Member

lukas-w commented Feb 18, 2014

@dobey
Yep, most disable it for security reasons. I think Travis does as well, as far as I know, they mirror all the Apt and PyPi repos etc.

@eagles051387
Well that contradicts what you just said in your previous comment.

@eagles051387
Copy link

Lukas I think there are multiple ways we can go about this to get to the
same goal.

On Tue, Feb 18, 2014 at 10:36 PM, Lukas W notifications@github.com wrote:

@dobey https://github.com/dobey
Yep, most disable it for security reasons. I think Travis does as well, as
far as I know, they mirror all the Apt and PyPi repos etc.

@eagles051387 https://github.com/eagles051387
Well that contradicts what you just said in your previous comment.

Reply to this email directly or view it on GitHubhttps://github.com//issues/315#issuecomment-35437155
.

Jonathan Aquilina

@tobydox
Copy link
Member

tobydox commented Feb 18, 2014

@dobey yes, some of them (https://github.com/LMMS/zynaddsubfx/commits/master) should go upstream while others are LMMS-specific modifications (pitch wheel, QtXmlWrapper, alternate working directory, ...)

@lukas-w It's not about updating but about the workflow when making changes to ZASF. Sure, I'll commit and push to the ZASF repository. But what then? At toplevel, Git will always report modified files. If you revert them, you (as developer) have old files and Git inside ZASF will report modified files (reversed).

In order to easily stay up to date with ZASF upstream, the external repository is a good thing and should be kept. Of course we need to find a way to allow Travis/PPA builds easily. Maybe the source tarballs can be fetched from a different location where on a server a cronjob or similiar does a complete clone/submodule-update and updates the tarballs?

@dobey
Copy link

dobey commented Feb 18, 2014

@tobydox I don't see any good reason why those changes are lmms specific and couldn't go upstream.

@eagles051387
Copy link

@toby could we use your ftp server for that to keep the tarballs up to date?

On Tue, Feb 18, 2014 at 10:52 PM, Tobias Doerffel
notifications@git.luolix.topwrote:

@dobey https://github.com/dobey yes, some of them (
https://github.com/LMMS/zynaddsubfx/commits/master) should go upstream
while others are LMMS-specific modifications (pitch wheel, QtXmlWrapper,
alternate working directory, ...)

@lukas-w https://github.com/Lukas-W It's not about updating but about
the workflow when making changes to ZASF. Sure, I'll commit and push to the
ZASF repository. But what then? At toplevel, Git will always report
modified files. If you revert them, you (as developer) have old files and
Git inside ZASF will report modified files (reversed).

In order to easily stay up to date with ZASF upstream, the external
repository is a good thing and should be kept. Of course we need to find a
way to allow Travis/PPA builds easily. Maybe the source tarballs can be
fetched from a different location where on a server a cronjob or similiar
does a complete clone/submodule-update and updates the tarballs?

Reply to this email directly or view it on GitHubhttps://github.com//issues/315#issuecomment-35438862
.

Jonathan Aquilina

@diizy
Copy link
Contributor Author

diizy commented Feb 18, 2014

On 02/18/2014 11:57 PM, dobey wrote:

@tobydox https://github.com/tobydox I don't see any good reason why
those changes are lmms specific and couldn't go upstream.


Reply to this email directly or view it on GitHub
#315 (comment).

That's not really up to us though. If upstream accepts them, that's great.

@dobey
Copy link

dobey commented Feb 18, 2014

@eagles051387 No, that does not fix any of the problems, nor is it secure.

@dobey
Copy link

dobey commented Feb 18, 2014

@diizy Sure. But they don't all seem to be exactly necessary, and I don't see any evidence that upstream hasn't accepted them (or that they've been submitted there).

@eagles051387
Copy link

I think and im sure we coudl all agree any enhancements shoudl be submitted
upstream. These woudl be beneficial not only for us but other projects that
use upstream sources as well.

On Tue, Feb 18, 2014 at 11:06 PM, dobey notifications@github.com wrote:

@diizy https://github.com/diizy Sure. But they don't all seem to be
exactly necessary, and I don't see any evidence that upstream hasn't
accepted them (or that they've been submitted there).

Reply to this email directly or view it on GitHubhttps://github.com//issues/315#issuecomment-35440473
.

Jonathan Aquilina

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

6 participants