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

Switch swh to submodule #3931

Merged
merged 2 commits into from
Nov 17, 2017
Merged

Switch swh to submodule #3931

merged 2 commits into from
Nov 17, 2017

Conversation

tresf
Copy link
Member

@tresf tresf commented Nov 2, 2017

Moves the "swh" LADSPA plugins to pull from upstream swh/ladspa by building the source from a git submodule. This requires a few new build tools over what we use today, namely perl and perl-xml.

Before

plugins/LadspaEffect/swh/acconfig.h
plugins/LadspaEffect/swh/alias_1407.c
plugins/LadspaEffect/swh/allpass_1895.c
[... 109 total files ...]

After

plugins/LadspaEffect/swh/CMakeLists.txt
plugins/LadspaEffect/swh/ladspa@ 5edf95c -> Points to http://github.com/swh/ladspa
[...]

New Dependencies

perl libxml2-utils libxml-perl liblist-moreutils-perl

Once merged, the dependencies must be updated on the wiki. Supercedes #3489, related #296

@gi0e5b06
Copy link

gi0e5b06 commented Nov 2, 2017

swh-lv2
swh-plugins (ladspa)

SWH is a package on most GNU/Linux distros. No need to make it a submodule, no need to have it as part of LMMS.

@tresf
Copy link
Member Author

tresf commented Nov 2, 2017

SWH is a package on most GNU/Linux distros. No need to make it a submodule, no need to have it as part of LMMS.

Thanks. We know. But this is the exception and not the rule and the systems you mention have packagers that already take advantage of this.

So in summary, the statement is not true for any use-case we care about... OUR installers.

Our installers include: Windows, MacOS & Linux AppImages (our ONLY official distribution methods).

Bundled installers need this and we will continue to bundle in this fashion until a better solution is provided

@gi0e5b06
Copy link

gi0e5b06 commented Nov 2, 2017

Windows: https://www.fosshub.com/Audacity.html/LADSPA_plugins-win-0.4.15.exe
MacOS: https://www.fosshub.com/Audacity.html/swh-plugins-mac-0.4.15.zip
AppImage: no one is using that (and anyway there is the package)

It would be better to support DSSI, LV2, VST3 and Nyquist plugins instead.
And Audio Units on MacOS, and native VST on both MacOS and Linux.

@tresf
Copy link
Member Author

tresf commented Nov 2, 2017

It would be better to support DSSI, LV2, VST3 and Nyquist plugins instead.
And Audio Units on MacOS, and native VST on both MacOS and Linux.

You're going off topic.

MacOS: https://www.fosshub.com/Audacity.html/swh-plugins-mac-0.4.15.zip

I'm not sure the point. The fact that some unrelated 3rd party provides binaries doesn't negate the value this PR has. We may eventually decouple our packaging as well, but that's out of scope. This PR explains its purpose and it's something that's been discussed for several months. Perhaps you need to jump on Discord? https://lmms.io/chat

@tresf
Copy link
Member Author

tresf commented Nov 2, 2017

AppImage: no one is using that (and anyway there is the package)

Again, off-topic. If you have issues with AppImages or the popularity thereof, please place them in the appropriate channels (#devtalk on Discord or any of the AppImage issues on this tracker).

On a side note, you've been asked several times to improve the quality of your contributions on GitHub.
This is your first warning. After three, you will be removed from our tracker. Let's keep comments on-topic and progressive, please. If you have a problem with this, we can discuss privately or publicly on Discord.

@lukas-w
Copy link
Member

lukas-w commented Nov 2, 2017

This is your first warning.

I feel like this already is the third warning: (1) #3422 (comment) (2) #3876 (comment)

@tresf
Copy link
Member Author

tresf commented Nov 2, 2017

This is successfully compiling on all platforms. I'd like to point a few things out...

  1. I put a submodule guard in here to avoid any strangeness with non-empty directories. We can likely standardize this into a macro (e.g. SUBMODULE_CHECK(swh/configure.ac)) and use it as a standard moving forward. It's a very basic check but it can help us move contributors away from confusing error messages. This will become more important as we add more submodules. So far, we have malloc, zynaddsubfx and swh . This list is going to grow.

  2. This PR contains patches that don't belong in LMMS source code but are required for compilation. Upstream PRs have been opened and will supercede those patches. I'm going to give Steve Harris (swh) a few days to merge, which he's very good about. Then I'll bump the submodule to the HEAD of upstream.

  3. If anyone has feedback on the "rube-goldberg" approach to building this project, speak now. I've considered asking upstream to switch to cmake, but the author no longer maintains this, so this would have to be done by a community member instead. The perl/xml template dependency makes this project a bad candidate for cmake I feel, but thoughts are welcome.

  4. Speaking of ^rube goldberg, there's an inefficiency added to compilation where the .xml files are coerced into .c files every time cmake is run. These should be cached and only updated as the .xml files are changed, but I don't know the best way to do this. Advice is appreciated. If I can't figure it out, I'll leave the FIXME in there and attack it at a later time.

@lukas-w
Copy link
Member

lukas-w commented Nov 3, 2017

  1. We can likely standardize this [submodule guard] into a macro (e.g. SUBMODULE_CHECK(swh/configure.ac)) and use it as a standard moving forward. It's a very basic check but it can help us move contributors away from confusing error messages.

Good idea 👍. We can even run git to fetch the submodule automatically if it's missing.

  1. If anyone has feedback on the "rube-goldberg" approach to building this project, speak now.

I'm slightly worried about what this means for building on Windows, especially regarding the perl dependency. It may be more portable to generate the .c files once, upload them on e.g. Github Releases, and fetch them at build time using ExternalProject_Add. But then again, the .c files will likely need fixing for MSVC anyway.

  1. […] there's an inefficiency added to compilation where the .xml files are coerced into .c files every time cmake is run. These should be cached and only updated as the .xml files are changed, but I don't know the best way to do this.

Use add_custom_target or add_custom_command instead of execute_process. They have a DEPENDS option where you can specify a list of files to watch for changes.

Something like this could work:

FOREACH(_item ${XML_SOURCES})
	SET(out_file "${_plugin}.c")
	GET_FILENAME_COMPONENT(_plugin "${_item}" NAME_WE)
	ADD_CUSTOM_COMMAND(
		OUTPUT ${out_file}
		COMMAND ./makestub.pl ${_item} > ${out_file}
		DEPENDS ${_item}
		WORKING_DIRECTORY ${CMAKE_CURRENT_SOURCE_DIR}/swh
		VERBATIM
	)
	LIST(APPEND PLUGIN_SOURCES ${out_file})
ENDFOREACH()

Then use the resulting PLUGIN_SOURCES list instead of globbing.

@tresf tresf force-pushed the swhsubmodule branch 2 times, most recently from eeb9720 to 38d0e37 Compare November 3, 2017 19:04
@tresf
Copy link
Member Author

tresf commented Nov 3, 2017

@lukas-w done via 38d0e37 (rebased, ea69da7) as well as some other cleanup and the first draft of the submodule/git script.

@lukas-w
Copy link
Member

lukas-w commented Nov 3, 2017

Great 👍 I'll have another look at the CMake scripts soon.

IF(NOT EXISTS "${ABSOLUTE_FILE}")
MESSAGE("-- Submodule '${SUBMODULE_NAME}' missing")
MESSAGE("-- Cloning '${SUBMODULE_NAME}' using git")
CHECK_COMMAND(git)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it's better to just use FIND_PACKAGE(Git) here instead. No need for an extra CMake module.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'll switch to FIND_PACKAGE(Git) and use GIT_EXECUTABLE variable, thanks. This module was originally added to look for some of the automake/autoconf/autopoint tools that weren't trivial to locate, but I've since realized we don't need them if we build with cmake. Thanks. I'll retire the module for now.

@tresf
Copy link
Member Author

tresf commented Nov 4, 2017

The following upstream patches have been merged:

The following upstream patches are still open:

.. once merged all patches and plugin blacklisting can safely be removed from this PR and we can simply fast-forward the submodule to be in sync with swh/ladspa upstream master.

@lukas-w
Copy link
Member

lukas-w commented Nov 5, 2017

Merge now or wait for swh/ladspa#52 to be accepted?

@tresf
Copy link
Member Author

tresf commented Nov 5, 2017

My instinct is to wait.

@tresf
Copy link
Member Author

tresf commented Nov 16, 2017

Upstream has been merged. Squashed to two separate commits. This PR should be "rebased and merged" once Travis is looking OK.

@tresf
Copy link
Member Author

tresf commented Nov 16, 2017

Dependencies for Ubuntu have been updated. Note, I have not updated Fedora, Debian or the others at this time. I also do not recommend updating Qt4 listing as we will drop Qt4 support with the master branch as soon as someone has the time to do so.

Another disclaimer for Windows coders... (e.g. @DomClark), this will also break master builds on Windows desktop with WANT_SWH enabled until we have the tools installed and documented properly. Our Windows build tutorial is grossly outdated since switching to Qt5, so this is a separate task and any help is appreciated.

@tresf tresf merged commit d393bdc into LMMS:master Nov 17, 2017
@tresf tresf deleted the swhsubmodule branch November 17, 2017 02:12
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

3 participants