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

multiple deployers: Ignore .debug files #182

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

tdewey-rpi
Copy link

@tdewey-rpi tdewey-rpi commented Jul 29, 2024

This commit replicates a pattern from the TextToSpeechPluginsDeployer across a range of other deployer classes.

Essentially - it's extremely unlikely that composing an AppImage with debug symbols is what a user actually wants to do. I'm also not aware of a good story for connecting debug symbols to an AppImage at all.

As a result, let's make a sensible choice - and not package them. Trust that developers will use their underlying build system for Debug, and that release tracking will allow developers to tie an AppImage back to a source control revision.

Finally, note that this works around an interesting aarch64 bug - where the .debug files that are created are marked as x86_64 ELF binaries - even when you are building on an aarch64 host.

Fixes: #110, but not in the manner suggested by the first respondent as they have conflated a desire to remove the .debug objects with a desire to remove text objects.

@TheAssassin
Copy link
Member

After having merged #175, this needs to be rebased most likely but on the other hand, it'll greatly simplify this contribution.

@tdewey-rpi
Copy link
Author

Happy to rebase atop the new structure.

@TheAssassin
Copy link
Member

@tdewey-rpi please rebase.

This commit replicates a pattern from the TextToSpeechPluginsDeployer
into the BasicPluginsDeployer - to affect all standard deployers.

Essentially - it's extremely unlikely that composing an AppImage
with debug symbols is what a user actually wants to do. I'm also
not aware of a good story for connecting debug symbols to an AppImage
at all.

As a result, let's make a sensible choice - and not package them.
Trust that developers will use their underlying build system for Debug,
and that release tracking will allow developers to tie an AppImage
back to a source control revision.

Finally, note that this works around an interesting aarch64 bug -
where the .debug files that are created are marked as x86_64 ELF
binaries - even when you are _building on an aarch64 host_.
Copy link
Contributor

@bjorn bjorn left a comment

Choose a reason for hiding this comment

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

Approved as-is, however, since BasicPluginsDeployer::deployStandardQtPlugins now excludes .debug files, TextToSpeechPluginsDeployer::doDeploy could be cleaned up to just call deployStandardQtPlugins({"texttospeech"}), right?

@tdewey-rpi
Copy link
Author

Approved as-is, however, since BasicPluginsDeployer::deployStandardQtPlugins now excludes .debug files, TextToSpeechPluginsDeployer::doDeploy could be cleaned up to just call deployStandardQtPlugins({"texttospeech"}), right?

For the sake of a clean history, I'll run add this to the PR.

With the refactor of BasicPluginsDeployer, we can collapse the contents of the TextToSpeechPluginsDeployer to just a call into the base class, with the appropriate argument.
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.

Don't deploy .so.debug for Qt plugins
3 participants