GNOME Bugzilla – Bug 773764
Improve flatpak support
Last modified: 2016-11-16 07:05:33 UTC
This gets flatpak apps to run (simple ones at least). Hopefully soon I can make another patch to make it work without a root git commit.
Created attachment 338868 [details] [review] flatpak: Discover flatpak manifests and add them as runtimes When a repo includes a flatpak manifest, GNOME Builder should know how to use it with flatpak-builder to build the project's dependencies so the build will be more likely to work. This commit teaches GbpFlatpakRuntimeProvider how to identify manifests (by file name and doing a sanity check on the contents) and add them to the list of runtimes.
Created attachment 338869 [details] [review] configuration: Add flatpak-related properties to IdeConfiguration As we improve our flatpak support it will be helpful to have build configuration properties for the flatpak manifest and repository being used.
Created attachment 338870 [details] [review] runner: Don't use the "flatpak build ..." launcher The subprocess launcher created by the flatpak runtime runs commands within the application's container, but to run an installed app we want to run "flatpak run APP_ID" from the host, so this commit adds code to make that happen. In the future we may want to change the interfaces to make this work more smoothly (such as having two separate launchers).
Created attachment 338871 [details] [review] autotools: Don't find build targets in flatpak runtimes To run a flatpak application we don't need the autotools build targets, just the application ID, so this commit adds code to prevent ide_autotools_build_system_get_build_targets_async() from trying to find build targets in a flatpak build (which fails).
Created attachment 338872 [details] [review] flatpak: Properly build, install, and run flatpak apps This commit teaches the flatpak runtime how to use a number of flatpak commands to build, install, and run applications. 1. In the prebuild worker, "flatpak remote-add" is used to ensure a repo exists where we can export the build, and either "flatpak-builder" or "flatpak build-init" is used to build dependencies and initialize the build directory (depending on whether a manifest was found in the source tree). 2. In the create_launcher function, code was added to use the options and environment variables specified in the manifest to form a "flatpak build ..." command. 3. In the postinstall worker, multiple subprocesses are created to run "flatpak build-finish" to finish the build directory, "flatpak build-export" to export the build to the repo created earlier, "flatpak uninstall" in case the app was previously installed, and "flatpak install" to install it on the host system. 4. Finally, a create_runner function was added that builds a "flatpak run ..." command to run the app with access to the display even if that wasn't granted in the build-finish step. In this way we can exert more control over the build process than if we let flatpak-builder do all the work for us. NOTE: Currently this process only works if a manifest is present (so we know the app id) and at least one git commit has occurred (so flatpak-builder doesn't complain).
Created attachment 338924 [details] [review] flatpak: Bump required flatpak version to 0.6.9 flatpak 0.6.9 added support for the --stop-at option for flatpak-builder, which we need.
Review of attachment 338868 [details] [review]: This will need just a bit of rebase done for master. Also some cleanup in the JSON parsing stuff. ::: plugins/flatpak/Makefile.am @@ +20,3 @@ $(NULL) +libflatpak_plugin_la_CFLAGS = $(PLUGIN_CFLAGS) $(FLATPAK_CFLAGS) $(JSON_GLIB_CFLAGS) Just add json-glib-1.0 >= json_glib_required_version to PKG_CHECK_MODULES(FLATPAK, ...) ::: plugins/flatpak/gbp-flatpak-runtime-provider.c @@ -183,0 +208,22 @@ +static gchar * +guess_primary_module (JsonObject *manifest_object, + GFile *directory) ... 19 more ... You need to check the node type before calling get_object_element(). @@ -183,0 +208,24 @@ +static gchar * +guess_primary_module (JsonObject *manifest_object, + GFile *directory) ... 21 more ... strcmp functions are 0 for success (as they are designed for qsort()) @@ -183,0 +208,68 @@ +static gchar * +guess_primary_module (JsonObject *manifest_object, + GFile *directory) ... 65 more ... Use `g_autoptr(GFile) file = NULL;` and then do g_steal_pointer(&file) when stealing the pointer for the structure. @@ -183,0 +208,93 @@ +static gchar * +guess_primary_module (JsonObject *manifest_object, + GFile *directory) ... 90 more ... parser is leaked upon failure. Probably just use `g_autoptr(JsonParser) parser = NULL;` @@ -183,0 +208,95 @@ +static gchar * +guess_primary_module (JsonObject *manifest_object, + GFile *directory) ... 92 more ... local_error != NULL @@ -183,0 +208,102 @@ +static gchar * +guess_primary_module (JsonObject *manifest_object, + GFile *directory) ... 99 more ... Is root guaranteed to be a JSON_NODE_OBJECT? I would think that at least JSON_NODE_ARRAY is also possible. @@ -183,0 +208,117 @@ +static gchar * +guess_primary_module (JsonObject *manifest_object, + GFile *directory) ... 114 more ... If you know the node types are correct (which doesn't seem to be checked currently, that should be fixed), you can use json_object_get_string_member().
Review of attachment 338869 [details] [review]: I think it is probably okay to add the IdeConfiguration:app-id property, but I'd like to avoid the other flatpak specific properties. Maybe we can add an internal data helper that allows us to have a grab bag of values that are used by various subsystems? For example, I think IdeConfiguration properties should be limited to things that the user would configure in .buildconfig files. Maybe something like: ide_configuration_get_internal_value() ide_configuration_get_internal_string() ide_configuration_get_internal_boolean() etc...
Review of attachment 338870 [details] [review]: IdeRunner can be subclassed for this purpose. The goal is that we would have a GbpFlatpakRunner that can manage the launching different than the abstract base class (using a launcher that does "flatpak run $app_id" instead). We might also need some work to be able to setup --command= if not the same as the program itself (thinking about tests when we get to that).
Review of attachment 338871 [details] [review]: I don't understand why discover of binaries doesn't work for flatpak? I think it should be okay if ended up running a command like "flatpak run --command=foo org.gnome.Foo". That way we can use the same plumbing to run unit tests in the runtime. Let's drop this commit if possible and try to get discovery working.
Created attachment 338929 [details] [review] flatpak: Don't use flatpak-builder if there aren't any dependencies flatpak-builder currently downloads sources for all modules even if they're ignored by the --stop-at option, so it fails for the template app when no git commits have been created. This commit makes the runtime just use flatpak-build-init in the case where the manifest only has one module (meaning there are no dependencies to worry about).
Re: comment #7: g_autoptr(JsonParser) caused issues when I tried it. I think json-glib isn't smart enough to deal with g_autoptr
(In reply to Matthew Leeds from comment #12) > Re: comment #7: > > g_autoptr(JsonParser) caused issues when I tried it. I think json-glib isn't > smart enough to deal with g_autoptr We use it contrib/jsonrpc-glib now with g_autoptr(). I think the main thing is that you have to json_node_copy() the root structure. (I think that was the issue I had, vs say json_node_ref()).
Review of attachment 338924 [details] [review]: cool
Review of attachment 338929 [details] [review]: ::: plugins/flatpak/gbp-flatpak-runtime.c @@ +212,3 @@ + ide_subprocess_launcher_push_argv (launcher3, "--ccache"); + ide_subprocess_launcher_push_argv (launcher3, "--force-clean"); + ide_subprocess_launcher_push_argv (launcher3, g_strdup_printf ("--stop-at=%s", self->primary_module)); This is leaking the allocated string.
Review of attachment 338872 [details] [review]: Nice work, really glad you pushed through on this stuff. Some leaks to fix to make things mergeable, and then we can do some refactoring after that based on a few things this shows me we need to change in our plumbing. ::: plugins/flatpak/gbp-flatpak-runtime.c @@ +230,3 @@ + + /* If the directory is already initialized, don't fail */ + ide_subprocess_wait_check (process2, cancellable, NULL); No need to set &error above if we aren't using it here. @@ -195,0 +266,52 @@ +static void +gbp_flatpak_runtime_postinstall_worker (GTask *task, + gpointer source_object, ... 49 more ... leaks error @@ +338,3 @@ + ide_subprocess_launcher_push_argv (launcher, "build-finish"); + if (!ide_str_empty0 (command)) + ide_subprocess_launcher_push_argv (launcher, g_strdup_printf ("--command=%s", command)); leaks @@ -195,0 +266,88 @@ +static void +gbp_flatpak_runtime_postinstall_worker (GTask *task, + gpointer source_object, ... 85 more ... g_clear_object (&parser); @@ +489,3 @@ + parser = json_parser_new (); + json_parser_load_from_file (parser, manifest_path, &json_error); + if (json_error) json_error is leaked. use g_autoptr(GError) json_error = NULL. @@ -208,1 +469,47 @@ - g_autofree gchar *build_path = get_build_directory (self); + g_autofree gchar *build_path = NULL; + g_autofree gchar *manifest_path = NULL; + gchar *project_path; ... 44 more ... if (g_file_query_file_type (project_file, 0, NULL) == G_FILE_TYPE_DIRECTORY) @@ -208,1 +469,53 @@ - g_autofree gchar *build_path = get_build_directory (self); + g_autofree gchar *build_path = NULL; + g_autofree gchar *manifest_path = NULL; + gchar *project_path; ... 50 more ... project_path and project_name are both leaked. Use g_autofree gchar *foo = NULL;. @@ -212,0 +529,17 @@ + if (!ide_str_empty0 (project_path)) + { + ide_subprocess_launcher_push_argv (ret, "--nofilesystem=host"); ... 14 more ... If for some reason the member isn't a string, this will cause NULL to be used and the g_strdup_printf(). It's safe for us on GLib, because we have our own printf implementation that will handle NULL. But do note on other libc's that NULL for %s is not guaranteed to be safe from crashing. Also, the resulting string is leaked. @@ +548,3 @@ + } + if (!ide_str_empty0 (cflags)) + ide_subprocess_launcher_push_argv (ret, g_strdup_printf ("--env=CFLAGS=%s", cflags)); Leaks @@ +550,3 @@ + ide_subprocess_launcher_push_argv (ret, g_strdup_printf ("--env=CFLAGS=%s", cflags)); + if (!ide_str_empty0 (cxxflags)) + ide_subprocess_launcher_push_argv (ret, g_strdup_printf ("--env=CXXFLAGS=%s", cxxflags)); Leaks @@ -214,1 +559,3 @@ ide_subprocess_launcher_set_run_on_host (ret, TRUE); + + g_object_unref (parser); parser is created from a different scope. So either g_autoptr(JsonParser) or use g_clear_object (&parser). @@ -227,1 +612,2 @@ ide_configuration_set_prefix (configuration, "/app"); + ide_configuration_set_flatpak_repo_name (configuration, "gnome-builder-builds"); If we add the internal API for IdeConfiguration, we should use that here too. @@ -281,2 +671,4 @@ break; + case PROP_PRIMARY_MODULE: + self->primary_module = g_value_dup_string (value); This leaks unless "primary-module" is set G_PARAM_CONSTRUCT_ONLY. (It was set to CONSTRUCT). @@ +750,3 @@ + NULL, + (G_PARAM_READWRITE | + G_PARAM_CONSTRUCT | Probably want CONSTRUCT_ONLY.
(In reply to Christian Hergert from comment #10) > Review of attachment 338871 [details] [review] [review]: > > I don't understand why discover of binaries doesn't work for flatpak? I > think it should be okay if ended up running a command like "flatpak run > --command=foo org.gnome.Foo". That way we can use the same plumbing to run > unit tests in the runtime. > > Let's drop this commit if possible and try to get discovery working. I figured out why getting the build targets doesn't work for flatpak runtimes: https://git.gnome.org/browse/gnome-builder/tree/plugins/autotools/ide-makecache.c#n1898. It tries to use the "flatpak build ..." launcher rather than just running the command in the project directory. It's unclear to me if there's a clean way to fix that. But I've implemented all the rest of your feedback so other than this issue these patches should be ready to commit.
To clarify, the reason I think it doesn't work for the flatpak launcher is that the command ("make -f - ...") expects the Makefile on stdin but it goes to flatpak-build's stdin instead.
Created attachment 339179 [details] [review] flatpak: Discover flatpak manifests and add them as runtimes When a repo includes a flatpak manifest, GNOME Builder should know how to use it with flatpak-builder to build the project's dependencies so the build will be more likely to work. This commit teaches GbpFlatpakRuntimeProvider how to identify manifests (by file name and doing a sanity check on the contents) and add them to the list of runtimes.
Created attachment 339180 [details] [review] configuration: Add app-id property to IdeConfiguration This property is especially useful for the flatpak runtime.
Created attachment 339181 [details] [review] flatpak: Add GbpFlatpakRunner subclass of IdeRunner GbpFlatpakRunner allows flatpak runtimes to use a separate launcher for running apps ("flatpak run ...") than the ones that's used for building them ("flatpak build ...").
Created attachment 339182 [details] [review] flatpak: Properly build, install, and run flatpak apps This commit teaches the flatpak runtime how to use a number of flatpak commands to build, install, and run applications. 1. In the prebuild worker, "flatpak remote-add" is used to ensure a repo exists where we can export the build, and either "flatpak-builder" or "flatpak build-init" is used to build dependencies and initialize the build directory (depending on whether a manifest was found in the source tree). 2. In the create_launcher function, code was added to use the options and environment variables specified in the manifest to form a "flatpak build ..." command. 3. In the postinstall worker, multiple subprocesses are created to run "flatpak build-finish" to finish the build directory, "flatpak build-export" to export the build to the repo created earlier, "flatpak uninstall" in case the app was previously installed, and "flatpak install" to install it on the host system. 4. Finally, a create_runner function was added that builds a "flatpak run ..." command to run the app with access to the display even if that wasn't granted in the build-finish step. In this way we can exert more control over the build process than if we let flatpak-builder do all the work for us. NOTE: Currently this process only works if a manifest is present (so we know the app id) and at least one git commit has occurred (so flatpak-builder doesn't complain).
Created attachment 339183 [details] [review] flatpak: Bump required flatpak version to 0.6.9 flatpak 0.6.9 added support for the --stop-at option for flatpak-builder, which we need.
Created attachment 339184 [details] [review] autotools: Fix build target discovery for flatpak runtimes ide_makecache_get_build_targets_worker() tries to find build targets by spawning make processes in each directory with a Makefile, but since the flatpak-build command specifies the directory it is unaffected by the process's cwd. So finding build targets in subdirectories fails for flatpak runtimes. This commit fixes that by using the -C option on make to switch to the appropriate subdirectory.
Those patches should address all the things you pointed out. The main thing I'm not sure about in them is if it's okay for GbpFlatpakRunner to emit IdeRunner's signals. There are a few things left to do, like redirecting the prebuild output to the Build Output tab in Builder, but I'm not sure if you want to wait on that stuff before releasing 3.22.3.
Created attachment 339272 [details] [review] autotools: Fix build target discovery for flatpak runtimes ide_makecache_get_build_targets_worker() tries to find build targets by spawning make processes in each directory with a Makefile, but since the flatpak-build command specifies the directory it is unaffected by the process's cwd. So finding build targets in subdirectories fails for flatpak runtimes. This commit fixes that by using the -C option on make to switch to the appropriate subdirectory.
Created attachment 339279 [details] [review] configuration: Add app-id property to IdeConfiguration This property is especially useful for the flatpak runtime.
Created attachment 339288 [details] [review] flatpak: Discover flatpak manifests and add them as runtimes When a repo includes a flatpak manifest, GNOME Builder should know how to use it with flatpak-builder to build the project's dependencies so the build will be more likely to work. This commit teaches GbpFlatpakRuntimeProvider how to identify manifests (by file name and doing a sanity check on the contents) and add them to the list of runtimes.
Created attachment 339300 [details] [review] flatpak: Properly build, install, and run flatpak apps This commit teaches the flatpak runtime how to use a number of flatpak commands to build, install, and run applications. 1. In the prebuild worker, "flatpak remote-add" is used to ensure a repo exists where we can export the build, and either "flatpak-builder" or "flatpak build-init" is used to build dependencies and initialize the build directory (depending on whether a manifest was found in the source tree). 2. In the create_launcher function, code was added to use the options and environment variables specified in the manifest to form a "flatpak build ..." command. 3. In the postinstall worker, multiple subprocesses are created to run "flatpak build-finish" to finish the build directory, "flatpak build-export" to export the build to the repo created earlier, "flatpak uninstall" in case the app was previously installed, and "flatpak install" to install it on the host system. 4. Finally, a create_runner function was added that builds a "flatpak run ..." command to run the app with access to the display even if that wasn't granted in the build-finish step. In this way we can exert more control over the build process than if we let flatpak-builder do all the work for us. NOTE: Currently this process only works if a manifest is present (so we know the app id) and at least one git commit has occurred (so flatpak-builder doesn't complain).
Created attachment 339357 [details] [review] autotools: Fix build target discovery for flatpak runtimes ide_makecache_get_build_targets_worker() tries to find build targets by spawning make processes in each directory with a Makefile, but since the flatpak-build command specifies the directory it is unaffected by the process's cwd. So finding build targets in subdirectories fails for flatpak runtimes. This commit fixes that by using the -C option on make to switch to the appropriate subdirectory.
Created attachment 339358 [details] [review] tests: Add tests for IdeSubprocessLauncher
Created attachment 339541 [details] [review] flatpak: Show prebuild/postbuild output in Builder In case something goes wrong in one of the prebuild or postbuild steps, it's useful for the user to see their output in the Build Output window of Builder. This commit makes that happen by adding an IdeBuildResult parameter to the runtime's async vfuncs and using it to call ide_build_result_log_subprocess from the flatpak runtime.
Created attachment 339542 [details] [review] flatpak: Discover flatpak manifests and add them as runtimes When a repo includes a flatpak manifest, GNOME Builder should know how to use it with flatpak-builder to build the project's dependencies so the build will be more likely to work. This commit teaches GbpFlatpakRuntimeProvider how to identify manifests (by file name and doing a sanity check on the contents) and add them to the list of runtimes.
Created attachment 339853 [details] [review] omnibar: Update display name when runtimes are added Previously, runtimes were loaded strictly before a project was opened so the IdeOmniBarRow appended "(missing)" to the end of the selected runtime name if it wasn't found in the IdeConfiguration. But now that GbpFlatpakRuntimeProvider treats a flatpak manifest file in a project's source tree as a runtime, we need to be able to update the runtime's display name whenever it might be discovered asynchronously, so that it doesn't erroneously appear to be missing to the user. This commit accomplishes that by connecting to the IdeRuntimeManager's "items-changed" signal, which is fired whenever the list of runtimes is modified.
Created attachment 339857 [details] [review] config-manager: Read/write app ID to .buildconfig
This bug was fairly open ended, but I think its fine to close now. Thanks for all the patches and hard work!