-
-
Notifications
You must be signed in to change notification settings - Fork 1k
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
Switch swh to submodule #3931
Conversation
swh-lv2 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 |
Windows: https://www.fosshub.com/Audacity.html/LADSPA_plugins-win-0.4.15.exe It would be better to support DSSI, LV2, VST3 and Nyquist plugins instead. |
You're going off topic.
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 |
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. |
I feel like this already is the third warning: (1) #3422 (comment) (2) #3876 (comment) |
This is successfully compiling on all platforms. I'd like to point a few things out...
|
Good idea 👍. We can even run
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
Use 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 |
eeb9720
to
38d0e37
Compare
Great 👍 I'll have another look at the CMake scripts soon. |
cmake/modules/CheckSubmodule.cmake
Outdated
IF(NOT EXISTS "${ABSOLUTE_FILE}") | ||
MESSAGE("-- Submodule '${SUBMODULE_NAME}' missing") | ||
MESSAGE("-- Cloning '${SUBMODULE_NAME}' using git") | ||
CHECK_COMMAND(git) |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
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. |
Merge now or wait for swh/ladspa#52 to be accepted? |
My instinct is to wait. |
Upstream has been merged. Squashed to two separate commits. This PR should be "rebased and merged" once Travis is looking OK. |
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 |
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, namelyperl
andperl-xml
.Before
After
New Dependencies
Once merged, the dependencies must be updated on the wiki. Supercedes #3489, related #296