Page 2 of 3

Re: Guitarix release 0.36.0

Posted: Sun Dec 10, 2017 10:34 am
by simonvanderveldt
I just tried building Guitarix 0.36.1 and creating a package for it for gentoo. During which I found some things in the wscript/waf configure options that don't seem to be working the way I expected them to.

I noticed there are two options "--no-ladspa" and "--no-new-ladspa", what's the difference between them?

The checks for some external dependencies don't fail when those external dependencies are not installed. For example if I don't use --includeresampler or --includeconvolver and I run ./waf configure it will not fail when I don't have zita-resampler or zita-convolver installed. @tramp Would you be open to patch that changes this?

How do I build LADPSA plugins only? It seems the options to target what to actually build are incomplete and/or too specific. There's an --lv2-only option, which is being used with a negation (not lv2-only) to force checks that don't relate to lv2 at all, i.e. if jack or gtk or ladspa is installed.
IMHO it would be much nicer to define during configure what to build instead of what not to build.
I.e. have a --standalone (that defaults to True) and a --lv2 and a --ladspa configure flag that enables building that specific version of guitarix and is used to check it's own relevant dependencies.
@tramp What do you think?

Re: Guitarix release 0.36.0

Posted: Mon Dec 11, 2017 4:26 am
by tramp
aaahaaap wrote:I noticed there are two options "--no-ladspa" and "--no-new-ladspa", what's the difference between them?
--no-ladspa exclude the very old guitarix ladspa plugins from the build, I'll exclude them on default within the next circle.
--no-new-ladspa exclude the newer ladspa plugin which provide the full guitarix engine as 2 ladspa plugins. This one is more a prov of concept and not been meant to be used in production. It may well be that we'll remove it completely as well.
aaahaaap wrote:The checks for some external dependencies don't fail when those external dependencies are not installed. For example if I don't use --includeresampler or --includeconvolver and I run ./waf configure it will not fail when I don't have zita-resampler or zita-convolver installed. @tramp Would you be open to patch that changes this?
This is intentional, as we provide zita-resampler and zita-convolver within the source. The --includeresampler and --includeconvolver build flags been meant to use the internal versions even when a usable version is found on the system.
When non are found, there is no reason to let the configure step fail, as we provide the needed sources. So, sorry, I ain't want to change this behave.
aaahaaap wrote:How do I build LADPSA plugins only?
You ain't. As I pointed out above, support for building the ladspa plugs will be drooped sooner or later. I know some people use the newer guitarix ladspa plug, that's why it is still there, but, . . .
aaahaaap wrote: It seems the options to target what to actually build are incomplete and/or too specific. There's an --lv2-only option, which is being used with a negation (not lv2-only) to force checks that don't relate to lv2 at all, i.e. if jack or gtk or ladspa is installed.
IMHO it would be much nicer to define during configure what to build instead of what not to build.
I.e. have a --standalone (that defaults to True) and a --lv2 and a --ladspa configure flag that enables building that specific version of guitarix and is used to check it's own relevant dependencies.
@tramp What do you think?
Nope, I think it will be good to have the flags set to the defaults we are expected to build the software. Anyone is free to set the flags to his own need, but I ain't want to disable, for example the LV2 plugin build by default. If you ain't want them, you've to disable them.

Most users been expected to just run ./waf configure && ./waf build && ./waf install
to get the main application and the plugs.

Other then that, the --lv2-only flag is needed to build the LV2 plugs within the MOD plugin builder environment.

Re: Guitarix release 0.36.0

Posted: Mon Dec 11, 2017 11:03 am
by simonvanderveldt
Thanks for the response/info!
tramp wrote:
aaahaaap wrote:I noticed there are two options "--no-ladspa" and "--no-new-ladspa", what's the difference between them?
--no-ladspa exclude the very old guitarix ladspa plugins from the build, I'll exclude them on default within the next circle.
--no-new-ladspa exclude the newer ladspa plugin which provide the full guitarix engine as 2 ladspa plugins. This one is more a prov of concept and not been meant to be used in production. It may well be that we'll remove it completely as well.
aaahaaap wrote:How do I build LADPSA plugins only?
You ain't. As I pointed out above, support for building the ladspa plugs will be drooped sooner or later. I know some people use the newer guitarix ladspa plug, that's why it is still there, but, . . .
OK, it sounds like LADSPA is actually not really a supported format. I think I'll either just disable both of them by default or remove the option all together.
Removing them would be nice(r), since that would prevent some confusion and maybe users that use them assuming they are usable?
tramp wrote:
aaahaaap wrote:The checks for some external dependencies don't fail when those external dependencies are not installed. For example if I don't use --includeresampler or --includeconvolver and I run ./waf configure it will not fail when I don't have zita-resampler or zita-convolver installed. @tramp Would you be open to patch that changes this?
This is intentional, as we provide zita-resampler and zita-convolver within the source. The --includeresampler and --includeconvolver build flags been meant to use the internal versions even when a usable version is found on the system.
When non are found, there is no reason to let the configure step fail, as we provide the needed sources. So, sorry, I ain't want to change this behave.
Maybe I didn't express myself well enough, but I was looking for the same behavior for the external dependencies as for the internal ones. I.e. an explicit choice.
There can of course still be a default for one of these two be set in the wscript, but when a user asks to use the external ones and they are not detected imho that should fail the configure step and report that to the user.
tramp wrote:
aaahaaap wrote: It seems the options to target what to actually build are incomplete and/or too specific. There's an --lv2-only option, which is being used with a negation (not lv2-only) to force checks that don't relate to lv2 at all, i.e. if jack or gtk or ladspa is installed.
IMHO it would be much nicer to define during configure what to build instead of what not to build.
I.e. have a --standalone (that defaults to True) and a --lv2 and a --ladspa configure flag that enables building that specific version of guitarix and is used to check it's own relevant dependencies.
@tramp What do you think?
Nope, I think it will be good to have the flags set to the defaults we are expected to build the software. Anyone is free to set the flags to his own need, but I ain't want to disable, for example the LV2 plugin build by default. If you ain't want them, you've to disable them.

Most users been expected to just run ./waf configure && ./waf build && ./waf install
to get the main application and the plugs.

Other then that, the --lv2-only flag is needed to build the LV2 plugs within the MOD plugin builder environment.
This is kind of similar to the external/internal zita dependencies, so again I might've expressed myself poorly. I'm not suggesting to change the default config but merely how this config is defined/achieved.
What I meant was to have explicit options for what the user wants to build, be it the standalone application or LV2 plugins (or LADSPA plugins for that matter).
That way the necessary checks for dependencies can be done based on simple logic (standalone enabled? Needs jack, etc. LV2 enabled? Needs liblv2, etc) and they can fail and report so to the user in case any of these dependencies are not found.

With this config there can still be sane defaults, so for example by default the standalone application and LV2 are enabled, whilst LADSPA is disabled. And the user can choose to override this by enabling/disabled what he/she wants.
Hope this clarifies it a bit.

Re: Guitarix release 0.36.0

Posted: Tue Dec 12, 2017 3:55 am
by tramp
aaahaaap wrote:@tramp Would you be open to patch that changes this?
aaahaaap wrote:Maybe I didn't express myself well enough,
At least it sounds reasonable what you said now.
If you provide a patch here:
https://sourceforge.net/p/guitarix/patc ... rce=navbar
I'll look at it, and if it turns out to be more clear in the configure steps, I'll accept it.

regards
hermann

Re: Guitarix release 0.36.0

Posted: Tue Dec 12, 2017 8:34 am
by simonvanderveldt
tramp wrote: At least it sounds reasonable what you said now.
If you provide a patch here:
https://sourceforge.net/p/guitarix/patc ... rce=navbar
I'll look at it, and if it turns out to be more clear in the configure steps, I'll accept it.

regards
hermann
I'll give it a try! :)

Re: Guitarix release 0.36.0

Posted: Mon Dec 18, 2017 10:18 am
by simonvanderveldt
aaahaaap wrote:
tramp wrote: At least it sounds reasonable what you said now.
If you provide a patch here:
https://sourceforge.net/p/guitarix/patc ... rce=navbar
I'll look at it, and if it turns out to be more clear in the configure steps, I'll accept it.

regards
hermann
I'll give it a try! :)
I'm still working on this, will take a bit longer unfortunately, WAF has got to be one the worst build tools there is and the useless docs are not really helping either :( I have some time off the next two weeks so I should be able to figure it out during that time :)

Re: Guitarix release 0.36.0

Posted: Mon Jan 01, 2018 4:04 pm
by simonvanderveldt
tramp wrote:
aaahaaap wrote:@tramp Would you be open to patch that changes this?
aaahaaap wrote:Maybe I didn't express myself well enough,
At least it sounds reasonable what you said now.
If you provide a patch here:
https://sourceforge.net/p/guitarix/patc ... rce=navbar
I'll look at it, and if it turns out to be more clear in the configure steps, I'll accept it.

regards
hermann
Hi Hermann,

First of all, happy new year! :)

It took a bit longer, but can you have a look at the changes I have so far? I've tried to make the commits as isolated as possible. https://github.com/simonvanderveldt/gui ... nk/wscript

The following is now in place:
- Fail when user requests to use external zita-resampler/zita-convolver and they are not installed
- Most of the no-<option> options have been replaced by regular <option> options. This enables a lot clearer logic in the wscript.
For example no more "if not lv2_only" but a clear "if standalone" to conditionally check the standalone application's dependencies.
- This also means it's now possible for the user to clearly choose which formats he/she wants to be built: "standalone", "lv2", "ladspa" and "new-ladspa"
- LV2 sub-options (GUI and MOD GUI) are disabled when LV2 is disabled

Based on your comments I've only made small changes to the current defaults:
- Building of LADSPA plugins is disabled by default (but can be enabled by the user)
- Building of new LADSPA plugins is disabled by default (but can be enabled by the user)
- Building of MOD LV2 plugin UI's is disabled by default (but can be enabled by the user)

I'd be very interested in what you think of it.
IMHO it's already a lot easier to read/understand the logic and it's a lot easier for the user to choose what to build + that choice is now properly honored as well :)

Also I ran into some things I'm not sure are correct/have some questions about:
- lilv seems to be needed for the standalone application and for the LADSPA plugins. Is that correct?
https://github.com/simonvanderveldt/gui ... cript#L684
- When is the Roboto font needed? I guess only for the standalone application and for LV2 GUI's?
- In src/gx_head's wscript "ladspaplugin.cpp" and "ladspaback.cpp" are always included which means ladspa and rdf need to be installed even when not building LADSPA plugins. Is this correct? I image it might be for using LADSPA plugins in the standalone application?
- The no-sse option is honored only when LV2 plugins are being built, I don't think that's necessary, right? Could just always be honored.
https://github.com/simonvanderveldt/gui ... lify-logic
- There's one plugin (jcm800pre) in src/plugins that has LV2 stuff in it. All other LV2 plugins live in src/LV2 so I think something went wrong there.
- In some places having the build work depends on order of the code in the wscript instead of dependencies being defined. For example the new LADSPA build needs to happen after one or more of the dirs in the lines above it are built otherwise it fails. It would be cleaner if these dependencies are properly defined.
- I haven't added a check that at least 1 of "standalone", "lv2", "ladspa" and "new-ladspa" should be chosen. Do you think that's necessary?

[edit] I have checked the majority of build combinations:
- Building everything works
- Building just the standalone application works
- Building just LV2 plugins works
- Building just LADSPA or new LADSPA plugins doesn't work. They seem to require several other items to be built/copied first (src/faust, src/plugins and others). These dependencies are currently not defined (and I don't really know them). Is this something you'd like to be fixed?

Regards,
Simon

P.S I have never used sourceforge before, so don't know (yet) how to create a patch. Will try to do so when I get the OK from you :)

Re: Guitarix release 0.36.0

Posted: Tue Jan 02, 2018 2:47 am
by tramp
Hi

Thanks for that.
I've a short look and it looks good so far.
Unfortunately I'm away for 14 day's now, and can't check it out for real.
I'll comment on your github page when I'm back, so that we could clear it for future use.

regards
hermann

Re: Guitarix release 0.36.0

Posted: Tue Jan 02, 2018 11:24 am
by simonvanderveldt
tramp wrote:Hi

Thanks for that.
I've a short look and it looks good so far.
Unfortunately I'm away for 14 day's now, and can't check it out for real.
I'll comment on your github page when I'm back, so that we could clear it for future use.

regards
hermann
Thanks for the quick look. And no problem, looking forward to the feedback in a couple of weeks :)
In the meantime I'll continue to try different build config combinations.

Re: Guitarix release 0.36.0

Posted: Sat Jan 20, 2018 7:01 pm
by simonvanderveldt
tramp wrote:Hi

Thanks for that.
I've a short look and it looks good so far.
Unfortunately I'm away for 14 day's now, and can't check it out for real.
I'll comment on your github page when I'm back, so that we could clear it for future use.

regards
hermann
Hi Hermann,

Have you had the chance to take a quick look? If you have any feedback, just let me know.

Re: Guitarix release 0.36.0

Posted: Sun Jan 21, 2018 4:43 am
by tramp
Hi Simon

First, many thanks for your contribution.
Configure options become indeed more clear now.
Even if I used to use it like it was, I could see the benefit.
I've pushed your changes to our git repository.
https://sourceforge.net/p/guitarix/git/ci/master/tree/

For the record, I've created a patch from your branch with:

Code: Select all

git format-patch master --stdout > wscript-simplify-logic.patch
to get a patch with all history information
and apply it to our master with:

Code: Select all

git am --signoff < wscript-simplify-logic.patch
so in the git history you are the author of this commits as it should to be,
and I've signed them up.

If you've more changes to do, let me know and I'll check it out, . .

regards
hermann

Re: Guitarix release 0.36.0

Posted: Sun Jan 21, 2018 12:41 pm
by simonvanderveldt
tramp wrote:Hi Simon

First, many thanks for your contribution.
Configure options become indeed more clear now.
Even if I used to use it like it was, I could see the benefit.
I've pushed your changes to our git repository.
https://sourceforge.net/p/guitarix/git/ci/master/tree/
...
so in the git history you are the author of this commits as it should to be,
and I've signed them up.

If you've more changes to do, let me know and I'll check it out, . .

regards
hermann
Hi Hermann,

That's great! And thanks for being so careful about the attribution. Appreciated! I would've been totally OK without is as well,I'm already happy I can help :)

If you have time, can you have a look at the questions I posted? Just trying to clear some stuff up and make sure everything is defined and working as it should. See below for the list:
aaahaaap wrote:Also I ran into some things I'm not sure are correct/have some questions about:
- lilv seems to be needed for the standalone application and for the LADSPA plugins. Is that correct?
https://github.com/simonvanderveldt/gui ... cript#L684
- When is the Roboto font needed? I guess only for the standalone application and for LV2 GUI's?
- In src/gx_head's wscript "ladspaplugin.cpp" and "ladspaback.cpp" are always included which means ladspa and rdf need to be installed even when not building LADSPA plugins. Is this correct? I image it might be for using LADSPA plugins in the standalone application?
- The no-sse option is honored only when LV2 plugins are being built, I don't think that's necessary, right? Could just always be honored?
https://github.com/simonvanderveldt/gui ... lify-logic
- There's one plugin (jcm800pre) in src/plugins that has LV2 stuff in it. All other LV2 plugins live in src/LV2 so I think something went wrong there?
- I haven't added a check that at least 1 of "standalone", "lv2", "ladspa" and "new-ladspa" should be chosen. Do you think that's necessary?
- Building just LADSPA or new LADSPA plugins without building the standalone application doesn't work. They seem to require several other items to be built/copied first (src/faust, src/plugins and others). These dependencies are currently not defined (and I don't really know them). Is this something you'd like to be fixed? Or would it be better to just start removing the LADSPA stuff alltogether?

Re: Guitarix release 0.36.0

Posted: Sun Jan 21, 2018 2:32 pm
by tramp
Hi
Yes sure, forgotten about this.
aaahaaap wrote:Also I ran into some things I'm not sure are correct/have some questions about:
- lilv seems to be needed for the standalone application and for the LADSPA plugins. Is that correct?
https://github.com/simonvanderveldt/gui ... cript#L684
Yes, lilv is needed for the standalone and the new ladspa plug, not for the older ones.
aaahaaap wrote: - When is the Roboto font needed? I guess only for the standalone application and for LV2 GUI's?
Roboto font is used only in the stand alone app, the LV2 UI's use what ever you've installed.
For the standalone app the Roboto font is chosen because it best fit in the layout of the interface.
aaahaaap wrote: - In src/gx_head's wscript "ladspaplugin.cpp" and "ladspaback.cpp" are always included which means ladspa and rdf need to be installed even when not building LADSPA plugins. Is this correct? I image it might be for using LADSPA plugins in the standalone application?
Yes, ladspa and rdf is needed by both, indeed the standalone need it to support ladspa plugins.
aaahaaap wrote: - The no-sse option is honored only when LV2 plugins are being built, I don't think that's necessary, right? Could just always be honored?
https://github.com/simonvanderveldt/gui ... lify-logic
The no-sse is special for the LV2 plugs, to simplify the build of the LV2 plugs for the MOD. Otherwise, you could just overwrite the --cxxflags-release flag, when you would disable sse support, but honestly, that is strongly not recommended. So better keep it just for the LV2 plugs.
aaahaaap wrote: - There's one plugin (jcm800pre) in src/plugins that has LV2 stuff in it. All other LV2 plugins live in src/LV2 so I think something went wrong there?
No, nothing wrong with that. The jcm800pre is generated by the ampsim toolkit, it could produce guitarix plugins, LV2 plugins, or plugins which support both API's. The jcm800pre support both, that's all.
aaahaaap wrote: - I haven't added a check that at least 1 of "standalone", "lv2", "ladspa" and "new-ladspa" should be chosen. Do you think that's necessary?
Maybe we just add a info output, if the standalone app will be build.
aaahaaap wrote: - Building just LADSPA or new LADSPA plugins without building the standalone application doesn't work. They seem to require several other items to be built/copied first (src/faust, src/plugins and others). These dependencies are currently not defined (and I don't really know them). Is this something you'd like to be fixed? Or would it be better to just start removing the LADSPA stuff alltogether?
Well, the new ladspa requires indeed the build of the standalone first. But I guess it's time to let it go slowly out. So no special addition for it to the waf script any more.

Hope that answer so far your questions,

regards
hermann

Re: Guitarix release 0.36.0

Posted: Sun Jan 21, 2018 3:08 pm
by simonvanderveldt
tramp wrote:Hi
...
Hope that answer so far your questions,

regards
hermann
This definitely answers all my questions :) Thanks again!

Re: Guitarix release 0.36.0

Posted: Tue Jan 23, 2018 7:54 am
by gimmeapill
Hi Guys,

Thanks for streamlining those build flags.
I have just updated the AUR pkgbuild and it does seems to reduce the size of the package by half (80MB -> 40MB).
According to the build log, everything seems to be there (LV2+GUI, new LADSPA), but I didn't have a chance to fully test yet.

So just to double check, here's what I went with:

Code: Select all

  python2 waf configure --prefix=/usr \
						--cxxflags-release \
						--optimize \
						--new-ladspa \
						--install-roboto-font
Did I miss something?