Skip to content

Fix GTK Namespace Not Found Error in Linux#756

Merged
freakboy3742 merged 74 commits into
beeware:mainfrom
danyeaw:linuxdeploy-gtk-plugin
Jul 21, 2022
Merged

Fix GTK Namespace Not Found Error in Linux#756
freakboy3742 merged 74 commits into
beeware:mainfrom
danyeaw:linuxdeploy-gtk-plugin

Conversation

@danyeaw

@danyeaw danyeaw commented Jun 5, 2022

Copy link
Copy Markdown
Member

Summary

This PR fixes a portion of #718 and goes along with linuxdeploy/linuxdeploy-plugin-gtk#33.

The issue is caused by missing typelib libraries for GTK which are not copied by linuxdeploy. The solution is to use the linuxdeploy-plugin-gtk to get the proper libraries installed by GTK. Although packaging GTK apps in older distributions worked, I think that was actually a case of AppImage bleed through to the host system libraries like in #662, to be confirmed.

PR Checklist:

  • All new features have been tested
  • All new features have been documented
  • I have read the CONTRIBUTING.md file
  • I will abide by the code of conduct

Triage of the Issue

When using the helloworld tutorial using a newer version of Linux, executing briefcase run results in the following error:

$ ~/P/b/helloworld briefcase run

[helloworld] Starting app...

Traceback (most recent call last):
  File "/tmp/.mount_Hello_um2Tym/usr/lib/python3.10/runpy.py", line 196, in _run_module_as_main
    return _run_code(code, main_globals, None,
  File "/tmp/.mount_Hello_um2Tym/usr/lib/python3.10/runpy.py", line 86, in _run_code
    exec(code, run_globals)
  File "/tmp/.mount_Hello_um2Tym/usr/app/helloworld/__main__.py", line 4, in <module>
    main().main_loop()
  File "/tmp/.mount_Hello_um2Tym/usr/app/helloworld/app.py", line 27, in main
    return HelloWorld()
  File "/tmp/.mount_Hello_um2Tym/usr/app_packages/toga/app.py", line 272, in __init__
    self.factory = get_platform_factory(factory)
  File "/tmp/.mount_Hello_um2Tym/usr/app_packages/toga/platform.py", line 41, in get_platform_factory
    from toga_gtk import factory
  File "/tmp/.mount_Hello_um2Tym/usr/app_packages/toga_gtk/factory.py", line 1, in <module>
    from .app import App, DocumentApp, MainWindow
  File "/tmp/.mount_Hello_um2Tym/usr/app_packages/toga_gtk/app.py", line 13, in <module>
    from .keys import gtk_accel
  File "/tmp/.mount_Hello_um2Tym/usr/app_packages/toga_gtk/keys.py", line 3, in <module>
    from .libs import Gdk
  File "/tmp/.mount_Hello_um2Tym/usr/app_packages/toga_gtk/libs/__init__.py", line 1, in <module>
    from .gtk import *  # noqa: F401, F403
  File "/tmp/.mount_Hello_um2Tym/usr/app_packages/toga_gtk/libs/gtk.py", line 3, in <module>
    gi.require_version('Gtk', '3.0')
  File "/tmp/.mount_Hello_um2Tym/usr/app_packages/gi/__init__.py", line 126, in require_version
    raise ValueError('Namespace %s not available' % namespace)
ValueError: Namespace Gtk not available

Research in to Linuxdeploy

As part of developing this PR, I read the docs for linuxdeploy and added an overview of what it is to the appimage docs. After I pointed out the existence of the linuxdeploy gtk plugin a couple of weeks ago, I investigated how the main bash script works.

@nickzoic confirmed that he got the plugin to work. Although I was successful at getting the GTK plugin loaded, I was still getting the same Namespace Not Available message. I was somewhat expecting this result because the Namespace unavailable message indicates that Python is not able to load PyGObject introspection itself, not that libraries are missing.

Troubleshooting PyGObject

$ cd linux/appimage/Hello\ World/Hello\ World.AppDir/bin
$ ./python3 -c "import gi"
Traceback (most recent call last):
  File "<string>", line 1, in <module>
ModuleNotFoundError: No module named 'gi'

That's not good, gi can't be imported at all. I referenced the environmental variables here: https://docs.python.org/3/using/cmdline.html#environment-variables.
Let's try to add it manually by setting the PYTHONPATH:

$ PYTHONPATH=../app_packages ./python3 -c "import gi; from gi.repository import Gtk"
Traceback (most recent call last):
  File "<string>", line 1, in <module>
  File "/home/dan/Projects/beeware-practice/helloworld2/linux/appimage/Hello World/Hello World.AppDir/usr/app_packages/gi/importer.py", line 136, in load_module
    raise ImportError('cannot import name %s, '
ImportError: cannot import name Gtk, introspection  not found

This gives a hint that we are missing the typelib because the introspection is not found. Typelibs provide the namespace for GIRepository, so it makes sense that without them that we would be getting a namespace error. This error was being somewhat masked when we use gi.require_version('Gtk', '3.0') when the app normally loads.

The briefcase-linux-appimage-template and the Linux support package that goes along with provide Python and the AppDir structure only, and this is set up when you run briefcase create. They don't include any of the other system libraries that are added in when you run briefcase build. So missing typelibs is caused by linuxdeploy not bundling these extra dependencies. The only supported way to add in extra dependencies with linuxdeploy
is by creating a plugin.

In Linux, pkg-config is used to determine the library location without having to know where each distro installs each library. To find the location of the GTK typelibs for introspection we run:

$ pkg-config --variable typelibdir gobject-introspection-1.0
/usr/lib64/girepository-1.0

In Ubuntu, it would be in a different location: /usr/lib/x86_64-linux-gnu/girepository-1.0/
The linuxdeploy gtk plugin already finds other libraries, so to add the typelibs I added a new tree copy of the typelibs that installs them to the right location in the APPDIR.

Other Solutions Considered

  1. Implement the scripts to copy the typelibs directly in Briefcase. Although this would be possible, I think the implementation of how to import libraries to build an AppImage should stay with linuxdeploy.
  2. Switch the default package format for Linux to Flatpak. Longer term this might be a good strategy, and @nickzoic already started working on it Fixes #359 - Implement Linux/Flatpak output for Briefcase #754, which is great!
  3. Use PyInstaller to freeze the libraries and then package the app in to an AppImage. This might another possible solution for the future since it has a great set of import hooks which solve finding libraries for a wide set of dependencies. We would have to consider if we want to add another dependency to Briefcase, and it would be a large change to our AppImage implementation.

Final Solution

You can now add a new linuxdeploy_plugins configuration in the linux portion of the config in the pyproject.toml, in order to have linuxdeploy load extra resources in to the AppImage bundle. This is needed for applications that depend on libraries that cannot be automatically discovered by linuxdeploy like GTK. For example,

linuxdeploy_plugins = ['gtk']

Briefcase can take plugins in three different formats:

  1. A plugin provided by Briefcase. Currently gtk is supported.
  2. A URL to a plugin, for example 'https://github.com/linuxdeploy/linuxdeploy-gtk-plugin/master/linuxdeploy-gtk-plugin.sh'
  3. A path to a plugin relative to the pyproject.toml file, like 'packaging/linuxdeploy-gtk-plugin.sh'

Some plugins need an environmental variable passed to configure it. For example, the gtk plugin needs the DEPLOY_GTK_VERSION environmental variable set to 3 or 4. If you use the gtk plugin provided by Briefcase,
it is set to 3 by default, but you can override it or set it for other plugins by setting the DEPLOY_GTK_VERSION environmental variable. For example, when using a relative path, you can set it like this:

linuxdeploy_plugins = ['DEPLOY_GTK_VERSION=3 packaging/linuxdeploy-gtk-plugin.sh']

@danyeaw danyeaw marked this pull request as ready for review June 6, 2022 00:21
@danyeaw danyeaw requested a review from freakboy3742 June 6, 2022 01:51
@nickzoic

nickzoic commented Jun 6, 2022

Copy link
Copy Markdown

I gave this a quick go with the default 'Hello World' app and once I added 'libgtk-3-dev', 'librsvg2-dev', to the system_requires it worked first time!

@danyeaw

danyeaw commented Jun 6, 2022

Copy link
Copy Markdown
Member Author

@nickzoic Thanks for giving it a test run and for all the groundwork you did on this issue!

Are you getting an error when you don't update the system_requires? It shouldn't need the extras now because I updated the gtk plugin to use default Ubuntu paths if those development libraries aren't available.

@nickzoic

nickzoic commented Jun 6, 2022

Copy link
Copy Markdown

No worries ... yeah, on e15d783 I was getting errors
(missing libgtk+-3.0.pc, missing librsvg-2.0.pc,see below for details) at briefcase build time. The plugin knows it wants them but they're not in the docker image.

The host is Ubuntu 20.04.4 but the libs were missing from the docker container so not sure if that matters or why it'd work differently since the container is always 18.04.

I had to add those -dev packages and redo briefcase create to get them into the docker container, after that the build worked.

$ briefcase build

[helloworld] Building AppImage...
Using linuxdeploy GTK plugin

[helloworld] Entering Docker context...
linuxdeploy version 1-alpha (git commit ID 097212a), GitHub actions build 80 built on 2022-05-03 17:10:24 UTC

-- Creating basic AppDir structure -- 

... Creating, Deploying, etc ...

-- Running input plugin: gtk -- 
[gtk/stdout] Installing AppRun hook
[gtk/stdout] Installing GLib schemas
[gtk/stdout] Installing GTK 3.0 modules
[gtk/stderr] Package gtk+-3.0 was not found in the pkg-config search path.
[gtk/stderr] Perhaps you should add the directory containing `gtk+-3.0.pc'
[gtk/stderr] to the PKG_CONFIG_PATH environment variable
[gtk/stderr] No package 'gtk+-3.0' found
[gtk/stderr] /home/brutus/.briefcase/tools/linuxdeploy-plugin-gtk.sh: there is no 'exec_prefix' variable for 'gtk+-3.0' library.
[gtk/stderr] Please check the 'gtk+-3.0.pc' file is present in $PKG_CONFIG_PATH (you may need to install the appropriate -dev/-devel package).
ERROR: Failed to run plugin: gtk (exit code: 1) 

Error while building app helloworld.

The pyproject.toml as created by briefcase new has:

system_requires = [
    'libgirepository1.0-dev',
    'libcairo2-dev',
    'libpango1.0-dev',
    'libwebkitgtk-3.0-0',
    'gir1.2-webkit-3.0',
]

@danyeaw

danyeaw commented Jun 6, 2022

Copy link
Copy Markdown
Member Author

@nickzoic Did you try to delete ~/.briefcase/tools/linuxdeploy-plugin-gtk.sh to make sure a new one is pulled down?

@nickzoic

nickzoic commented Jun 6, 2022

Copy link
Copy Markdown

I hadn't thought of that! I'd just been deleting the build directories, I'll give it a go ... oh no, it's not downloading correctly for me now, I end up with a linuxdeploy-plugin-gtk.sh full of HTML! Looks like it failed in a way that wasn't an exception, I'll try and work out why.

I do think the idea of having state outside the build directory is fraught, though ... it's bound to come back and bite us at a later date.

@danyeaw

danyeaw commented Jun 6, 2022

Copy link
Copy Markdown
Member Author

@nickzoic Whoops, thanks for helping to test and for all your contributions providing the basis of this PR in the first place!

I updated the download URL to fix the issue 👍

@danyeaw danyeaw force-pushed the linuxdeploy-gtk-plugin branch 2 times, most recently from 13e7562 to 19fda7e Compare June 7, 2022 01:30

@freakboy3742 freakboy3742 left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Looks like a good start; I've got a couple of questions inline.

The other "bigger picture" questions I have are:

  1. What the plugin is getting us that our existing library discovery isn't getting us? I think it's mostly the typelibdir stubs... is there anything else? What happens if someone uses a library that uses typelib stubs that isn't in the "blessed list" that the Gtk plugin captures?
  2. What's the impact on the Qt story? Or for any other toolkit that we support?

Comment thread docs/reference/platforms/linux/appimage.rst Outdated
Comment thread docs/reference/platforms/linux/appimage.rst Outdated
plugins = []
if app.requires:
for dependency in app.requires:
if "toga-gtk" in dependency or "PyGObject" in dependency:

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Will this catch:

  • toga-gtk>=0.3.0.dev33? (i.e., a version string on the dependency)?
  • pygobject (i.e., case incompatibility?)
  • An app that uses a different toolkit that wraps GTK, but doesn't directly import pygobject? (not sure I have any specific examples in mind, but I'm sure someone has built one)
    ?

On the last example - is there a reason you opted for dependency inspection rather than an explicit list of linuxdeploy plugins needed by the app?

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

toga-gtk>=0.3.0.dev33? (i.e., a version string on the dependency)?

Yes, I think so, that is what I tested it with a toga-gtk is in that string.

pygobject (i.e., case incompatibility?)

Updated the implementation to support this, good idea!

An app that uses a different toolkit that wraps GTK, but doesn't directly import pygobject? (not sure I have any specific examples in mind, but I'm sure someone has built one)

No, but we could support this in the future if we had something in mind.

On the last example - is there a reason you opted for dependency inspection rather than an explicit list of linuxdeploy plugins needed by the app?

I think the general use case this was implementing for was users either have a Toga or PyGObject app and those will be listed already in the requires. We could add another option for explicit plugins needed, but I am not sure that this is needed at this time unless we implement a much larger plugin system, and if we do that we could add it at that time.

for dependency in app.requires:
if "toga-gtk" in dependency or "PyGObject" in dependency:
self.logger.info("Using linuxdeploy GTK plugin")
env["DEPLOY_GTK_VERSION"] = "3"

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Is this envvar something we can reliably set in code? What happens if someone uses GTK4?

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Right now it is hard coded to support our Toga use case. The plugin does do some automatic detection, but our apps are not successfully being detected. This could be a future PR to extend support for GTK4.

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Yeah, it's meant to auto-detect but I found it wasn't ... the version would have to be the correct one for the toga-gtk version I suppose (see previous conversation about mapping libs to requirements)

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

The autodetect doesn't work because it uses ldd to look for executables that are linked against the GTK libraries. Our only executable is Python itself which is not linked against GTK.

raise CorruptToolError("linuxdeploy")


class LinuxDeployGtkPlugin:

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Is there any potential for a generic plugin interface here? I noticed there is a Qt plugin - it seems likely we may need to use that for PySide2/6 deployments

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

I think there definitely is a potential for something more general in the future if we add additional plugins. 👍

Comment thread src/briefcase/integrations/linuxdeploy.py
@danyeaw

danyeaw commented Jun 7, 2022

Copy link
Copy Markdown
Member Author

Thanks for the review and feedback, I really appreciate it!

Overall the scope of this contribution was to solve the immediate issue that many users in Linux are not able to package a Toga app using the tutorial. I agree that we should also discuss our longer term vision of our strategy for Linux packaging going forward.

  1. What the plugin is getting us that our existing library discovery isn't getting us? I think it's mostly the typelibdir stubs... is there anything else?

If an AppImage should be completely self-contained with no guarantee that a user will have certain libraries (besides a working X Windows or Wayland system) installed, then I think our original implementation was missing other things beyond typelibs including:

  1. GDK Pixbuf cache and module loaders. The cache has the module loaders paths hardcoded and is generated by the gdk-pixbuf-query-loaders binary. The module loaders are also dynamic libraries (*.so), but they weren't being pulled in by linuxdeploy without the plugin. I do not think loading images will work without this (without relying on GDK being installed on the host system).
  2. Input Method support: immodules in GTK supports input characters that cannot be written directly by keyboard, for example to support languages like Chinese, Korean, and Japanese. This is similar to the one above, where there is a cache file that is generated using the gtk-query-immodules-3.0 executable.
  3. Printbackends if an app wants to include print functionality. This may include things like file, cups, and lpr.

What happens if someone uses a library that uses typelib stubs that isn't in the "blessed list" that the Gtk plugin captures?

This solution only supports typelibs for GTK, if there are other libraries that need typelibs, we would have to add plugins for those or use a different option.

  1. What's the impact on the Qt story? Or for any other toolkit that we support?

There is a plugin we could add for Qt as well with a similar implementation. I think these two are by far the most complex to get working.

Why I discussed other solutions I considered is because I think we need to decide:

  1. Do we want to support AppImage?
  2. If we do, is our goal to reliably package most Toga and Qt apps? Or reliably package any app using any imported python module?

If our focus is on Toga and Qt, I think we can extend linuxdeploy. If our focus is on any app, then we shouldn't reinvent the wheel and we should make Briefcase a user friendly wrapper over PyInstaller. For Linux, Windows, and macOS, they have solved the analyzing python code and pulling in all required dependencies so that it can then be packaged problem.

@freakboy3742

Copy link
Copy Markdown
Member
  1. What the plugin is getting us that our existing library discovery isn't getting us? I think it's mostly the typelibdir stubs... is there anything else?

If an AppImage should be completely self-contained with no guarantee that a user will have certain libraries (besides a working X Windows or Wayland system) installed, then I think our original implementation was missing other things beyond typelibs including:

  1. GDK Pixbuf cache and module loaders. The cache has the module loaders paths hardcoded and is generated by the gdk-pixbuf-query-loaders binary. The module loaders are also dynamic libraries (*.so), but they weren't being pulled in by linuxdeploy without the plugin. I do not think loading images will work without this (without relying on GDK being installed on the host system).
  2. Input Method support: immodules in GTK supports input characters that cannot be written directly by keyboard, for example to support languages like Chinese, Korean, and Japanese. This is similar to the one above, where there is a cache file that is generated using the gtk-query-immodules-3.0 executable.
  3. Printbackends if an app wants to include print functionality. This may include things like file, cups, and lpr.

That certainly explains some of the bug reports we've had - in particular the errors around loading images/icons.

I guess the follow up question is why are those pieces not being loaded? Is there any way that we could systematically ensure that pieces like this are found? What is the GTK plugin doing that is "special" in this regard?

My understanding was that Linuxdeploy essentially followed the linking chain from every .so that it encountered. This fails as an automated technique as soon as dynamic loading occurs - but as long as you manually specify one of the entry points that is dynamically loaded, the link-following approach can continue.

It seems like you're suggesting that even that approach has it's limits, because there are additional files and resources that won't be picked up by this link-following process. This does make some sense - if there's a resource file that is required at runtime, it's going to be very difficult/almost impossible to automatically detect that a .so references that resource file.

The problem that we face is that we can include a plugin for GTK, and maybe even Qt, which will work for tutorial cases and simple apps - but if a user writes an app that uses some obscure library, they might end up in a situation where they can't package the app simply because there isn't a linuxdeploy plugin that works for their particular library.

Are we in a position (however theoretical) where we could reproduce what the GTK plugin is doing as a matter of configuration? Linuxdeploy doesn't have any real way to define a "configuration" - but we're not constrained in that way. We're in a position to provide almost arbitrarily complex lists of files/directories that need to be passed to the linuxdeploy invocation. If necessary, we could define our own "generic" plugin that uses configuration items in the pyproject.toml file - or we could provide a plugin definition as part of the project template, which would give end-users an easy way to customise what is/isn't included in their own apps. Based on what you know, does this approach sound even remotely viable?

What happens if someone uses a library that uses typelib stubs that isn't in the "blessed list" that the Gtk plugin captures?

This solution only supports typelibs for GTK, if there are other libraries that need typelibs, we would have to add plugins for those or use a different option.

  1. What's the impact on the Qt story? Or for any other toolkit that we support?

There is a plugin we could add for Qt as well with a similar implementation. I think these two are by far the most complex to get working.

Agreed. A quick look at the qt plugin shows that it's a compiled binary, not just a shell script, which makes me question whether the 'custom plugin in the template' approach I described will work...

Why I discussed other solutions I considered is because I think we need to decide:

  1. Do we want to support AppImage?
  2. If we do, is our goal to reliably package most Toga and Qt apps? Or reliably package any app using any imported python module?

Addressing point (2) first - The end goal should be "any app, using any library"; however, I'm also comfortable with a 90% solution that only manages Toga, GTK and Qt apps with no "exotic" libraries as an interim step. Ideally, we'd be able to identify when an app hits this "exotic" boundary so we can report the problem to the user at time of build, rather than have the problem be discovered at runtime by end users.

As for point (1) - my enthusiasm for AppImage is driven primarily by the fact that it was the one I was able to get to work... for a definition of "work" that appears to be at least partially accidental. The current implementation has clearly missed some of edge cases, and as distributions diverge from what was present on Ubuntu 18.04, these edge cases are becoming more apparent. The biggest downside to AppImage appears to be that it is a passion project of a very small number of developers who... well, let's just say they don't consistently adhere to the standards of conduct that the BeeWare community expects of its members.

For me, the question is two-fold:
a. Can AppImage ever be made to work reliably - even in the 90% case? Will accomodating just the GTK and Qt plugins be enough to get us to the 90% case, or will we be perpetually chasing our tail?
b. Are there viable alternatives? From an industry standpoint, Flatpak seems to have a lot more traction than AppImage and Snap; will it provide a more robust packaging solution?

Supporting both AppImage and Flatpak is also an option, but we'd need to make a call as to which one is the default.

If our focus is on Toga and Qt, I think we can extend linuxdeploy. If our focus is on any app, then we shouldn't reinvent the wheel and we should make Briefcase a user friendly wrapper over PyInstaller. For Linux, Windows, and macOS, they have solved the analyzing python code and pulling in all required dependencies so that it can then be packaged problem.

I'm not especially enamoured by the idea of using pyinstaller - that would seem to be a bit of an existential question for Briefcase. Why does Briefcase exist if all it is doing is wrapping another python packaging tool? Working out how to package what is needed for a standalone app is kind of the point of Briefcase; if we can't do it, then we should pack up shop and move on :-)

There is undeniably some overlap in what pyinstaller does and what Briefcase does/needs to do - to the extent that I'm sure we can learn from each other - but there's also enough different in the core approaches that I don't think we should give up.

@danyeaw

danyeaw commented Jun 8, 2022

Copy link
Copy Markdown
Member Author

It seems like you're suggesting that even that approach has it's limits, because there are additional files and resources that won't be picked up by this link-following process. This does make some sense - if there's a resource file that is required at runtime, it's going to be very difficult/almost impossible to automatically detect that a .so references that resource file.

Yes, that's my understanding as well.

The problem that we face is that we can include a plugin for GTK, and maybe even Qt, which will work for tutorial cases and simple apps - but if a user writes an app that uses some obscure library, they might end up in a situation where they can't package the app simply because there isn't a linuxdeploy plugin that works for their particular library.

Yup, I agree that is the current limitation of our approach.

Are we in a position (however theoretical) where we could reproduce what the GTK plugin is doing as a matter of configuration? Linuxdeploy doesn't have any real way to define a "configuration" - but we're not constrained in that way. We're in a position to provide almost arbitrarily complex lists of files/directories that need to be passed to the linuxdeploy invocation. If necessary, we could define our own "generic" plugin that uses configuration items in the pyproject.toml file - or we could provide a plugin definition as part of the project template, which would give end-users an easy way to customise what is/isn't included in their own apps. Based on what you know, does this approach sound even remotely viable?

I think it sounds viable, and I think a more generalized form of the first alternative I considered. However, we probably need to consider the user experience of this approach. It is very difficult figuring out all the things to include to get a certain dependency to work. PyInstaller did this by allowing for users to create "hooks". The hooks are then shared as part of PyInstaller itself and a contrib library, so once a hook for a certain troublesome library is created, then everyone who uses that library gets that solution from then on.

For me, the question is two-fold: a. Can AppImage ever be made to work reliably - even in the 90% case? Will accomodating just the GTK and Qt plugins be enough to get us to the 90% case, or will we be perpetually chasing our tail?

Yes, I think we can get to a 90% solution, and I think this plus a 2nd PR to add Qt support would get us most of the way there.

b. Are there viable alternatives? From an industry standpoint, Flatpak seems to have a lot more traction than AppImage and Snap; will it provide a more robust packaging solution?

Supporting both AppImage and Flatpak is also an option, but we'd need to make a call as to which one is the default.

I agree that Flatpak is the primary alternative, and I would vote that it is even the default.

I'm not especially enamoured by the idea of using pyinstaller - that would seem to be a bit of an existential question for Briefcase. Why does Briefcase exist if all it is doing is wrapping another python packaging tool? Working out how to package what is needed for a standalone app is kind of the point of Briefcase; if we can't do it, then we should pack up shop and move on :-)

One of the reasons that Python has had so much success in the data science field is that the community built on top of what already existed. Seaborn is built on top of matplotlib for example.

I think of packaging in a few phases:

  1. Install dependencies for your app on the platform you are targeting
  2. Collect all of those dependencies and other files needed to run the app
  3. Package the app in a format that can be installed by someone else, including signing and notarizing

Briefcase might not have to solve all the problems for all 3 of those, but it could take the best of those things that exist and glue them together and make them easier for the user.

There is undeniably some overlap in what pyinstaller does and what Briefcase does/needs to do - to the extent that I'm sure we can learn from each other - but there's also enough different in the core approaches that I don't think we should give up.

PyInstaller really only solves the 2nd one above, and only on the current platform, no cross-compilation to mobile for example. It also uses a spec format for configuration that isn't particularly user-friendly.

@nickzoic

nickzoic commented Jun 8, 2022

Copy link
Copy Markdown

I don't know if there's any way to get from "requires toga-gtk" to "system_requires libgtk-3-dev and librsvg2-dev" automatically ... maybe python libraries should have such a thing, it'd be super handy instead of the traditional README.md declaration of "first, apt install all this crap, then ..."

(It seems like this is the kind of metadata that python packages should already have but ... no? Seems weird there's no PEP for this at least ...)

We could maybe get from the 90% solution to the 95% solution for briefcase just by having a little table of "if you require this python library we know you'll need those system libraries". The build process could then implicitly require that, or explicitly add it to the pyproject.toml file, or warn the user to add them, or something.

@freakboy3742

Copy link
Copy Markdown
Member

I don't know if there's any way to get from "requires toga-gtk" to "system_requires libgtk-3-dev and librsvg2-dev" automatically ... maybe python libraries should have such a thing, it'd be super handy instead of the traditional README.md declaration of "first, apt install all this crap, then ..."

We are already able to do this using the briefcase project template - if you specify that you want a Toga app, you get a bunch of libraries included as system defaults. If those two libraries are required, it's easy to add to the template; but from what I'm reading in the discussion, @danyeaw didn't need those libraries to get the app working.

(It seems like this is the kind of metadata that python packages should already have but ... no? Seems weird there's no PEP for this at least ...)

We could maybe get from the 90% solution to the 95% solution for briefcase just by having a little table of "if you require this python library we know you'll need those system libraries". The build process could then implicitly require that, or explicitly add it to the pyproject.toml file, or warn the user to add them, or something.

Binary requirements for "the whole of PyPI" is probably out of scope. There might be a possibility of defining system-requires requirements in packaging metadata as a Briefcase extension that packages could opt into - or, we could turn to Conda as a better source of defined binary dependencies.

@nickzoic

nickzoic commented Jun 8, 2022

Copy link
Copy Markdown

Oh, well, I do love a good Astronomically Out Of Scope :-) If there's really no PEP maybe I should write one.

What I mean was a bit of a middle ground though: we currently know toga-gtk's requirements, no problem, but are there other common python packages we also should check for?

@freakboy3742

Copy link
Copy Markdown
Member

Oh, well, I do love a good Astronomically Out Of Scope :-) If there's really no PEP maybe I should write one.

What I mean was a bit of a middle ground though: we currently know toga-gtk's requirements, no problem, but are there other common python packages we also should check for?

I guess it wouldn't be hard to generate a list of the "top N PyPI binary packages", build a map of those packages to their ubuntu 18.04 system package requirements, and then put those requirements in a data file somewhere (say, in the app template).

However, I'm also acutely aware that that statement is also a good chunk of what Conda does out of the box, so... maybe it would better to spend that effort working out how to leverage what Conda can provide, rather than trying to reinvent the wheel (pun 100% intended).

@nickzoic

nickzoic commented Jun 8, 2022

Copy link
Copy Markdown

PS: It also seems to be what PyInstaller hooks are for ...

Comment thread src/briefcase/platforms/linux/appimage.py Outdated

@freakboy3742 freakboy3742 left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

The testing that I've done around #757 has convinced me that adopting the GTK plugin is definitely worth adding. However, it's highlighted to me that plugins are a key part of the linuxdeploy ecosystem. I wonder if aiming generic might be the better approach here.

This PR makes a special case of the GTK plugin, and autodetects the need for that plugin. This has 2 downsides:

  • The autodetection can fail.
  • It only allows for the use of the "official" plugin.

The upside is that there's no need for any additional configuration items.

As a counterproposal: We could add an linuxdeploy_plugins configuration item for the AppImage backend:

linuxdeploy_plugins = ['gtk', 'https://example.com/linuxdeploy-plugin-magic.sh']

This would allow end users to specify whatever combination of plugins that are necessary. We can include a "simple" list of shortcodes(like gtk and qt) that includes the officially supported plugins that expands to the official download URLs; but allow users to provide their own URLs (with a user-provided explicit plugin overriding the builtin). This would allow users to provide their own plugins; or their own bug fixed versions of plugins (e.g., your bugfix version of the GTK plugin).

This wouldn't allow us to handle the DEPLOY_GTK_VERSION=3 environment variable; although we could possible handle this by:

  1. Setting that environment variable for the user as part of the gtk 'shortcode"
  2. Ensure that variables in the environment used to invoke briefcase are passed to linuxdeploy (i.e., you'd need to invoke DEPLOY_GTK_VERSION=3 briefcase build linux)
  3. Allow for DEPLOY_GTK_VERSION=3 gtk as a syntax for declaring plugins - i.e., use the shell convention of setting environment variables in the definition.

(3) would be my preferred of those options, but is obviously more complex to implement.

The downside to this approach is that there is more manual configuration. We can slightly mitigate this by auto-adding the plugins in the base project template, but there's no escaping the fact that end-users will need to know a lot more about Linux packaging to be able to drive the tool effectively. Unfortunately, I'm not sure if this is completely avoidable. Linux packaging is... complex... and trying to completely avoid that is a

Another option would be to split the difference - have autodetection of 'known, supported' plugins, but also allow for manual specification. This might also fit into the solution for #757 - if we know that certain packages must be deployed as source, we can provide that assistance as an automation; but we can also open the door to allow manual configuration when things go wrong.

Any thoughts about this proposal?

@danyeaw danyeaw force-pushed the linuxdeploy-gtk-plugin branch from 6ea55e2 to cf90ca2 Compare June 12, 2022 02:39
@danyeaw

danyeaw commented Jun 12, 2022

Copy link
Copy Markdown
Member Author

As a counterproposal: We could add an linuxdeploy_plugins configuration item for the AppImage backend:

linuxdeploy_plugins = ['gtk', 'https://example.com/linuxdeploy-plugin-magic.sh']

I like your counterproposal! 👍 For the configuration in the pyproject.toml, maybe it would look something like this?

[tool.briefcase.app.helloworld.linux]
requires = [
    'toga-gtk>=0.3.0.dev34',
]
system_requires = [
    'libgirepository1.0-dev',
    'libcairo2-dev',
    'libpango1.0-dev',
    'libwebkitgtk-3.0-0',
    'gir1.2-webkit-3.0',
]
linuxdeploy_plugins = [
    'gtk',
]

Where we could enter 3 things:

  1. A known plugin like 'gtk' or 'qt'
  2. A URL to a plugin like 'https://github.com/linuxdeploy/linuxdeploy-gtk-plugin/master/linuxdeploy-gtk-plugin.sh'
  3. A path to a plugin that is part of the project, like 'packaging/linuxdeploy-gtk-plugin.sh'

I thought of doing both name and URL, but it seems like it would just make things more complex for the user.

This wouldn't allow us to handle the DEPLOY_GTK_VERSION=3 environment variable

I like your idea to support shell variables, but it may not be needed at this time. Ubuntu didn't have GTK4 in the repos until 21.04, and 22.04 is the first LTS version with it. I am assuming we would want to support 18.04 until 2023 and 20.04 until 2025 for AppImage compatibility. So we wouldn't support GTK4 until 2025, unless someone wants to use Docker to build it from source using JHBuild - I have done this, but it is not trivial. Not to pile on AppImage limitations, but GTK4 was released in December 2020, and it will then take 5 years to support the new toolkit. 😦

I would propose will hold off on this piece until we are a little more sure of our packaging strategy for Linux, and we see a need for passing the environmental variables. YAGNI 😄

@freakboy3742

Copy link
Copy Markdown
Member

I like your counterproposal! 👍 For the configuration in the pyproject.toml, maybe it would look something like this?
...
Yup - that's what I had in mind.

Where we could enter 3 things:

  1. A known plugin like 'gtk' or 'qt'
  2. A URL to a plugin like 'https://github.com/linuxdeploy/linuxdeploy-gtk-plugin/master/linuxdeploy-gtk-plugin.sh'
  3. A path to a plugin that is part of the project, like 'packaging/linuxdeploy-gtk-plugin.sh'

I hadn't thought of "local path", but that's a good idea.

I thought of doing both name and URL, but it seems like it would just make things more complex for the user.

Yeah - I had similar a thought. However, AFAICT, linuxdeploy plugin naming has a required scheme, so you can derive the name of the plugin from the name of the file to be executed; that makes name = value redundant (completely aside from what to use in the "name only " case).

The one additional detail that occurred to me - if the plugin is user-provided, we won't be able to cache it in ~/.briefcase/tools. If I have one project using the "default" GTK plugin, and a second project using a custom GTK plugin, we would end up with an ambiguity over which version is being cached, based on which project was generated first.

The docs for linuxdeploy say that one of the locations that is inspected for plugins is the location of linuxdeploy binary, or the location of the AppDir, which we can use to our advantage - depending on the order of resolution.

If linuxdeploy looks in the AppDir first, then a URL-specified plugin can be downloaded/copied and stored next to the AppDir as an artefact of the creation process. If the plugin is a system default, it can be downloaded and cached as you've implemented here.

If linuxdeploy looks at the same directory as linuxdeploy first, then we'll need to cache the system plugins in ~/.briefcase/tools/linuxdeploy-plugins, and copy default plugins into the project. That way, the plugin will always be read from the AppDir location, we'll just have some redundant copies of the plugin on the system. We might be able to use a symlink to avoid the copy?

This wouldn't allow us to handle the DEPLOY_GTK_VERSION=3 environment variable

I like your idea to support shell variables, but it may not be needed at this time. Ubuntu didn't have GTK4 in the repos until 21.04, and 22.04 is the first LTS version with it. I am assuming we would want to support 18.04 until 2023 and 20.04 until 2025 for AppImage compatibility. So we wouldn't support GTK4 until 2025, unless someone wants to use Docker to build it from source using JHBuild - I have done this, but it is not trivial. Not to pile on AppImage limitations, but GTK4 was released in December 2020, and it will then take 5 years to support the new toolkit. 😦

I would propose will hold off on this piece until we are a little more sure of our packaging strategy for Linux, and we see a need for passing the environmental variables. YAGNI 😄

Totally onboard with YAGNI as an analysis; but what will insert the GTK environment variable in the case of a user-customized GTK plugin? From my testing, the plugin crashes on Briefcase projects if the variable isn't set.

@danyeaw

danyeaw commented Jun 16, 2022

Copy link
Copy Markdown
Member Author

The one additional detail that occurred to me - if the plugin is user-provided, we won't be able to cache it in ~/.briefcase/tools.

Great point on caching. I'll need to think about this more and to make sure that it works with the 3 different cases of a known plugin, URL, and path. I think for a path we don't need to cache at all because it will already be a local file.

If linuxdeploy looks at the same directory as linuxdeploy first, then we'll need to cache the system plugins in ~/.briefcase/tools/linuxdeploy-plugins, and copy default plugins into the project. That way, the plugin will always be read from the AppDir location, we'll just have some redundant copies of the plugin on the system. We might be able to use a symlink to avoid the copy?

Another option might be to make the cache logic more advanced, while still avoiding downloading the file if unless there is a cache miss. Maybe we could use the pyproject.toml file's checksum as a cache key and redownload the used plugins if the configuration changes.

what will insert the GTK environment variable in the case of a user-customized GTK plugin? From my testing, the plugin crashes on Briefcase projects if the variable isn't set.

Ahh yes! I was focused on the known plugin, and if we need environmental variables for URLs and paths, then I guess we support it for all 3.

Thanks for the additional great discussion. I should have time to update the PR this weekend.

@freakboy3742

Copy link
Copy Markdown
Member

For anyone looking at this PR - you can start a new project using the template from beeware/briefcase-template#30 or add:

linuxdeploy_plugins = ['DEPLOY_GTK_VERSION=3 gtk']

to the linux configuration section of an existing project. No changes are needed to the briefcase-linux-AppImage.

@rmartin16

rmartin16 commented Jul 21, 2022

Copy link
Copy Markdown
Member

Worked out of the box on pop_os 22.04 🦾

A few remarks. These may come down to expectations but but given the whole goal is to avoid target system dependencies, looking for clarification.

Below is the listed of loaded libraries for hello world with toga created within docker that did not come from the AppImage:

python3 158594 russell  mem       REG                8,1  1306280   26742794 /usr/lib/x86_64-linux-gnu/libX11.so.6.4.0
python3 158594 russell  mem       REG                8,1   844624   26743407 /usr/lib/x86_64-linux-gnu/libharfbuzz.so.0.20704.0
python3 158594 russell  mem       REG                8,1   133224   26743300 /usr/lib/x86_64-linux-gnu/girepository-1.0/WebKit2-4.0.typelib
python3 158594 russell  mem       REG                8,1   149760   26743530 /usr/lib/x86_64-linux-gnu/libgpg-error.so.0.32.1
python3 158594 russell  mem       REG                8,1   153904   26743577 /usr/lib/x86_64-linux-gnu/libgraphite2.so.3.2.1
python3 158594 russell  mem       REG                8,1    34888   26743220 /usr/lib/x86_64-linux-gnu/libdatrie.so.1.4.0
python3 158594 russell  mem       REG                8,1    41152   26744454 /usr/lib/x86_64-linux-gnu/libthai.so.0.3.1
python3 158594 russell  mem       REG                8,1 15752576   26218205 /usr/lib/locale/locale-archive
python3 158594 russell  mem       REG                8,1   813128   26743966 /usr/lib/x86_64-linux-gnu/libfreetype.so.6.18.1
python3 158594 russell  mem       REG                8,1  2216304   26743105 /usr/lib/x86_64-linux-gnu/libc.so.6
python3 158594 russell  mem       REG                8,1    16336   26740571 /usr/lib/x86_64-linux-gnu/girepository-1.0/JavaScriptCore-4.0.typelib
python3 158594 russell  mem       REG                8,1    47472   26743898 /usr/lib/x86_64-linux-gnu/libmd.so.0.0.5
python3 158594 russell  mem       REG                8,1   137560   26743091 /usr/lib/x86_64-linux-gnu/libbrotlicommon.so.1.0.9
python3 158594 russell  mem       REG                8,1   194872   26743343 /usr/lib/x86_64-linux-gnu/libexpat.so.1.8.7
python3 158594 russell  mem       REG                8,1   166504   26744683 /usr/lib/x86_64-linux-gnu/libxcb.so.1.1.0
python3 158594 russell  mem       REG                8,1   298120   26743386 /usr/lib/x86_64-linux-gnu/libfontconfig.so.1.12.0
python3 158594 russell  mem       REG                8,1   940560   26743884 /usr/lib/x86_64-linux-gnu/libm.so.6
python3 158594 russell  mem       REG                8,1    89096   26743097 /usr/lib/x86_64-linux-gnu/libbsd.so.0.11.5
python3 158594 russell  mem       REG                8,1    26800   26742811 /usr/lib/x86_64-linux-gnu/libXdmcp.so.6.0.0
python3 158594 russell  mem       REG                8,1    18720   26742800 /usr/lib/x86_64-linux-gnu/libXau.so.6.0.0
python3 158594 russell  mem       REG                8,1   108936   26744727 /usr/lib/x86_64-linux-gnu/libz.so.1.2.11
python3 158594 russell  mem       REG                8,1    51512   26743093 /usr/lib/x86_64-linux-gnu/libbrotlidec.so.1.0.9
python3 158594 russell  mem       REG                8,1    68552   26744315 /usr/lib/x86_64-linux-gnu/libresolv.so.2
python3 158594 russell  mem       REG                8,1    14432   26744542 /usr/lib/x86_64-linux-gnu/libutil.so.1
python3 158594 russell  mem       REG                8,1    14432   26743261 /usr/lib/x86_64-linux-gnu/libdl.so.2
python3 158594 russell  mem       REG                8,1    21448   26744278 /usr/lib/x86_64-linux-gnu/libpthread.so.0
python3 158594 russell  mem       REG                8,1    30920   26744546 /usr/lib/x86_64-linux-gnu/libuuid.so.1.3.0
python3 158594 russell  mem       REG                8,1    14664   26744329 /usr/lib/x86_64-linux-gnu/librt.so.1
python3 158594 russell  mem       REG                8,1    27002   26742057 /usr/lib/x86_64-linux-gnu/gconv/gconv-modules.cache
python3 158594 russell  mem       REG                8,1   240936   26742580 /usr/lib/x86_64-linux-gnu/ld-linux-x86-64.so.2

Most of these appear to be fine except for girepository-1.0/WebKit2-4.0.typelib, girepository-1.0/JavaScriptCore-4.0.typelib, libthai.so.0.3.1, libgraphite2.so.3.2.1, and libdatrie.so.1.4.0.

libgraphite2 is being pulled in from libharfbuzz which blocklisted by AppImage's packaging. Probably fine.

libthai is required by libpango but AppImage's packaging explicitly excludes it....apparently for audicity. I don't know how to override that blocklist (or if we'd want to)....but I'm also not sure if most linux distros include this by default.

libdatrie is required by libthai.

As for those typelibs coming from girepository, I cannot track down what's loading them. linuxdeploy doesn't detect them when packaging the AppImage and LD_DEBUG=files doesn't say they are being loaded when opening the AppImage... If i build the AppImage outside of docker, i get the same behavior with linuxdeploy and LD_DEBUG but now they are being loaded from the girepository from within the AppImage.....idk

As a sidenote, briefcase doesn't have much resiliency against the Docker image disappearing. This PR makes that situation worse, though, since it's passing a CalledProcessError exception back to the interpreter when the image disappears (instead of just erroring); I'll add a comment to at least catch that. I can also create a new issue for better handling this in general (see #796).

# environment *inside* the Docker container.
if self.use_docker:
echo_cmd = ["/bin/bash", "-c", "echo $PATH"]
base_path = self.Docker(self, app).check_output(echo_cmd).strip()

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

This can bomb out if the Docker image disappears.

[helloworld] Configuring Linuxdeploy plugins...

>>> Running Command:
>>>     docker run --volume /home/russell/tmp/beeware/helloworld/linux:/app:z --volume /home/russell/.cache/briefcase:/home/brutus/.cache/briefcase:z --rm briefcase/com.example.helloworld:py3.10 /bin/bash
-c 'echo $PATH'
Unable to find image 'briefcase/com.example.helloworld:py3.10' locally
docker: Error response from daemon: pull access denied for briefcase/com.example.helloworld, repository does not exist or may require 'docker login': denied: requested access to the resource is denied.
See 'docker run --help'.
>>> Return code: 125

Log saved to /home/russell/tmp/beeware/helloworld/briefcase.2022_07_21-14_19_42.build.log

Traceback (most recent call last):
  File "/home/russell/github/danyeaw/briefcase/venv-3.10/bin/briefcase", line 33, in <module>
    sys.exit(load_entry_point('briefcase', 'console_scripts', 'briefcase')())
  File "/home/russell/github/danyeaw/briefcase/src/briefcase/__main__.py", line 15, in main
    command(**options)
  File "/home/russell/github/danyeaw/briefcase/src/briefcase/commands/build.py", line 58, in __call__
    state = self._build_app(
  File "/home/russell/github/danyeaw/briefcase/src/briefcase/commands/build.py", line 39, in _build_app
    state = self.build_app(app, **full_options(state, options))
  File "/home/russell/github/danyeaw/briefcase/src/briefcase/platforms/linux/appimage.py", line 173, in build_app
    base_path = self.Docker(self, app).check_output(echo_cmd).strip()
  File "/home/russell/github/danyeaw/briefcase/src/briefcase/integrations/docker.py", line 324, in check_output
    return self._subprocess.check_output(
  File "/home/russell/github/danyeaw/briefcase/src/briefcase/integrations/subprocess.py", line 297, in check_output
    cmd_output = self._subprocess.check_output(
  File "/home/russell/.pyenv/versions/3.10.4/lib/python3.10/subprocess.py", line 420, in check_output
    return run(*popenargs, stdout=PIPE, timeout=timeout, check=True,
  File "/home/russell/.pyenv/versions/3.10.4/lib/python3.10/subprocess.py", line 524, in run
    raise CalledProcessError(retcode, process.args,
subprocess.CalledProcessError: Command '['docker', 'run', '--volume', '/home/russell/tmp/beeware/helloworld/linux:/app:z', '--volume', '/home/russell/.cache/briefcase:/home/brutus/.cache/briefcase:z', '--rm', 'briefcase/com.example.helloworld:py3.10', '/bin/bash', '-c', 'echo $PATH']' returned non-zero exit status 125.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I hit this error a couple of times, but I put it down to the fact that I was doing some "off script" rebuilds and cache refreshes. Definitely worth addressing, but I don't think it's a blocker for this PR specifically.

@mhsmith mhsmith left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

When used with beeware/briefcase-template#30, it also worked out of the box for me on Debian 10 (Buster), which was released in 2019. I looked through the linuxdeploy GTK plugin output and saw no concerning messages.

Comment thread docs/reference/platforms/linux/appimage.rst Outdated
Comment on lines +244 to +246
self.hash = hashlib.sha256()
for part in url_parts:
self.hash.update(part.encode("utf-8"))

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Simply hashing the URL directly would be simpler, and would also have the advantage of allowing the hash to be checked using the command-line sha256sum tool. If there's some benefit to doing it by parts, there should be a comment explaining what it is.

I think it's also possible that doing it by parts could cause different URLs to have the same hash, e.g. http://example.com/filename and http://example.com/file?name.

@freakboy3742 freakboy3742 Jul 21, 2022

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Having 2 different URLs produce different hashes was deliberate. The following 3 URLs could all produce different versions of a plugin:

  • http://example.com/release/linuxdeploy-plugin-foobar.sh
  • http://example.com/dev/linuxdeploy-plugin-foobar.sh
  • http://example.com/archive/linuxdeploy-plugin-foobar.sh?version=1

Hashing on just the domain/filename would make the three appear to be the same plugin.

Admittedly, this is an edge case that is unlikely to be an issue in practice, but if it ever comes up, it would be very difficult to diagnose, so I figured we should be proactive. Documenting the why is definitely a good idea, though.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Oh wait - I see what you're saying now - just hash the URL literal, rather than using HMAC. Yeah - that's a good idea - we're not depending on any cryptographic properties of the HMAC. I was overthinking things because I had the parsed URL parts handy.

Comment thread src/briefcase/platforms/linux/appimage.py Outdated
Comment on lines +152 to +154
@property
def plugin_id(self):
return self.file_name.split(".")[0].split("-")[2]

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

plugin_id is defined sometimes as a property and sometimes as a simple class variable. This makes LinuxDeploy.plugins a bit confusing, because it looks up plugin_id on the class, but when you go looking for its definition you might find the base class property, which can only be looked up on an instance, not a class.

I think it would be clearer to remove the plugin_id overrides in the GTK and Qt classes, and instead make LinuxDeploy.plugins set the known plugin names directly in a dict literal.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Yeah - I went back and forth on this. The inconsistency is potentially confusing; using a literal in LinuxDeploy.plugins means duplication of information. You're probably right that being confusing is a bigger risk than the duplication, though.

@codecov

codecov Bot commented Jul 21, 2022

Copy link
Copy Markdown

Codecov Report

Merging #756 (b519d04) into main (e07558a) will increase coverage by 0.21%.
The diff coverage is 100.00%.

Impacted Files Coverage Δ
src/briefcase/integrations/git.py 55.55% <ø> (ø)
src/briefcase/commands/upgrade.py 100.00% <100.00%> (+5.00%) ⬆️
src/briefcase/integrations/docker.py 92.77% <100.00%> (+0.36%) ⬆️
src/briefcase/integrations/linuxdeploy.py 100.00% <100.00%> (ø)
src/briefcase/platforms/linux/appimage.py 97.43% <100.00%> (+0.40%) ⬆️
src/briefcase/console.py 100.00% <0.00%> (ø)

@freakboy3742 freakboy3742 left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Following reviews from @mhsmith and @rmartin16, I'm going to call his done. Thanks to @danyeaw for the original work on this!

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