-
Notifications
You must be signed in to change notification settings - Fork 38
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
base: master
Are you sure you want to change the base?
Conversation
After having merged #175, this needs to be rebased most likely but on the other hand, it'll greatly simplify this contribution. |
Happy to rebase atop the new structure. |
@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_.
29bcebd
to
1e1f146
Compare
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.
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.
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.