Skip to content

Conversation

tresf
Copy link
Member

@tresf tresf commented Sep 4, 2025

Remove some workaround code that was added as part of #7252 due to a misunderstanding of how linuxdeploy works.

This workaround is best explained here:

# Workaround libraries being incorrectly placed in usr/lib (e.g. instead of usr/lib/lmms, etc)
# FIXME: Remove when linuxdeploy supports subfolders https://github.com/linuxdeploy/linuxdeploy/issues/305
DIR="$( cd "$( dirname "${BASH_SOURCE[0]}" )/.." && pwd )"
export LMMS_PLUGIN_DIR="$DIR/usr/lib/"
export LADSPA_PATH="$DIR/usr/lib/"
export SUIL_MODULE_DIR="$DIR/usr/lib/"

Related upstream (wontfix) bug report:

Linux testing TODO:

  • Test ARM64 AppImage
    • Test LADSPA plugins
    • Test ZynAddSubFx
    • Test STK/Mallets
    • Test Samples/Waveforms
  • Test Intel Appimage
    • Test LADSPA plugins
    • Test Wine 32-bit VSTs
    • Test Wine 64-bit VSTs
    • Test Linux Native VSTs
    • Test ZynAddSubFx
    • Test STK/Mallets
    • Test Samples/Waveforms

I'm not sure the state of Suil plugins, so they may be testable too? Pinging @JohannesLorenz for any info on the status of that on master branch.

@tresf tresf changed the title Initial refactor to un-pollute $APP/usr/lib AppImage: Un-pollute $APP/usr/lib Sep 4, 2025
@tresf tresf requested review from messmerd and bratpeki September 4, 2025 19:31
@tresf tresf added this to the 1.3-alpha.2 milestone Sep 4, 2025
@tresf tresf changed the title AppImage: Un-pollute $APP/usr/lib AppImage: Un-pollute /usr/lib Sep 4, 2025
@tresf
Copy link
Member Author

tresf commented Sep 4, 2025

Surprisingly, all tests pass. Would still like a second set of eyes, but no regressions found with the plugin system.

@TheAssassin
Copy link

The amount of lines removed vs. lines added is really great to see.

@messmerd
Copy link
Member

messmerd commented Sep 4, 2025

I plan to test this, but it might not be until Sunday when I have the time

@tresf
Copy link
Member Author

tresf commented Sep 5, 2025

I half expected this but linuxdeploy doesn't like non-existent directories as parameters to --deploy-deps-only; this PR requires some slight refactoring to know what we're bundling.

@JohannesLorenz
Copy link
Contributor

JohannesLorenz commented Sep 5, 2025

I'm not sure the state of Suil plugins, so they may be testable too?

Suil was added back then with #4899 , just as a preparation for #7201 . Testing Suil properly would mean rebasing a copy of #7201 on this PR. I could try this on a weekend, though my time is very limited in September. If you need a fast test, someone else could try it out, too. Anyways, let me know when you think this PR is ready for testing.

@tresf
Copy link
Member Author

tresf commented Sep 5, 2025

I'm not sure the state of Suil plugins, so they may be testable too?

Suil was added back then with #4899 , just as a preparation for #7201 . Testing Suil properly would mean rebasing a copy of #7201 on this PR. I could try this on a weekend, though my time is very limited in September. If you need a fast test, someone else could try it out, too. Anyways, let me know when you think this PR is ready for testing.

I had a feeling it wasn't merged yet. No worries then @JohannesLorenz we can tackle that when #7201 is closer to being merged. From what I recall, the path to suil-0 needs to be specified in an environment variable, which I've modified as part of this PR, but shouldn't cause any impact to nightly since it's currently unused.

@tresf
Copy link
Member Author

tresf commented Sep 5, 2025

Mostly unrelated to this PR, but I'm adding it anyways... after revisiting copy_excluded(...) macro that's was part of LinuxDeploy.cmake, I've decided to give it a better name CopyDependency.cmake/copy_dependency(...) and move it to a dedicated cmake module via 2d93cf8.

Details below for those that are interested in the TL;DR.

Click to expand details

Originally, the intent of this macro was to make ldd foo.so |grep some_dep.so.0 a bit easier from cmake so that we could isolate (e.g. move) "excluded" libraries, such as libjack.so.0 into a safe location for systems that ship with it, but also provide it dynamically at startup for systems without (e.g. #7689) that would otherwise break the .AppImage from loading. ldd is used to get the EXACT name and location of the dependency and then we resolve any symlinks to ensure we're copying the physical file.

if ldd "$DIR/usr/bin/lmms" |grep "libjack.so" |grep "not found" > /dev/null 2>&1; then
echo "[$ME] Jack does not appear to be installed. That's OK, we'll use a dummy version instead." >&2
export LD_LIBRARY_PATH="$DIR/usr/lib/jack:$LD_LIBRARY_PATH"
else
echo "[$ME] Jack appears to be installed on this system, so we'll use it." >&2
fi

Where we ran into trouble (hence #7689) was the missing transient dependency, which required us to recursively call this function again against libjack.so.0 to resolve libdb-5.3.so, which we create a symlinks for... But this won't scale if other dependencies are needed...

However, with the advent of --deploy-deps-only, we can do this proactively by stashing libjack.so.0 into a dedicated directory and asking linuxdeploy to handle our dependency checks (e.g. libdb-5.3.so) while also fixing the rpath entries for this one-off lib.

In addition, I've also written the macro in a way that it should be reusable on other OSs for the same purpose if we need it into the future:

  • Tested on macOS (uses otool -L instead of ldd)
  • Tested on FreeBSD (file and ldd behave identically)
  • Tested on OpenBSD (file and ldd behave identically)
  • Tested on Haiku (for posterity) which uses application/x-sharedlib but there's no ldd equivalent, ignoring.
  • Tested on Solaris 11, but it doesn't support file -b --mime-type, ignoring.

@bratpeki bratpeki self-assigned this Sep 6, 2025
@bratpeki
Copy link
Member

bratpeki commented Sep 6, 2025

Debloating is always great. How do I test this?

@tresf
Copy link
Member Author

tresf commented Sep 6, 2025

Debloating is always great. How do I test this?

Most of the testing is bulleted here: #8053 (comment)

@messmerd usually does a file count/tree comparison (e.g. BEFORE vs. AFTER) which is nice too as a sanity check. I would expect everything to work, just need a second set of eyes.

@messmerd
Copy link
Member

@tresf
I compared the squashfs-root/ contents of this PR's AppImages to nightly's and everything looks fine.

I also tested it briefly and didn't notice any issues.

In #7252, you added what I believe is a workaround with the LMMS_PLUGIN_DIR environment variable to src/core/RemotePlugin.cpp - maybe that can be removed now? And I seem to remember some other workarounds too. Maybe in #7722 or #7691.

@tresf
Copy link
Member Author

tresf commented Sep 10, 2025

In #7252, you added what I believe is a workaround with the LMMS_PLUGIN_DIR environment variable to src/core/RemotePlugin.cpp - maybe that can be removed now?

Good catch, agreed.

And I seem to remember some other workarounds too. Maybe in #7722 or #7691.

Hmm... I don't see any compelling reason to revert any of this code. I don't see any disadvantage in fine-grained control at runtime for our plugin scanning. Notice that there are no TODO or FIXME entries in any of that code because it provides a useful feature for problematic or missing plugins.... meanwhile I can see several cases into the future where a user may need to leverage these.

…arent folder with LADSPA plugins)

This reverts commit 8b160c4.
@tresf
Copy link
Member Author

tresf commented Sep 10, 2025

In #7252, you added what I believe is a workaround with the LMMS_PLUGIN_DIR environment variable to src/core/RemotePlugin.cpp

Clarification:

  • LMMS_EXCLUDE_LADSPA: Yes.
  • LMMS_EXCLUDE_PLUGINS: No.

Reasoning: Our own libcarla plugin still needs to be excluded to silence unnecessary warnings from startup; This is a quality-of-life improvement unrelated to the workaround code.

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

Successfully merging this pull request may close these issues.

5 participants