-
-
Notifications
You must be signed in to change notification settings - Fork 112
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
Add build and run controls for Flatpak based projects #940
base: master
Are you sure you want to change the base?
Add build and run controls for Flatpak based projects #940
Conversation
ngl I think that haveing a small 'section' of the titlebar to have a n almost media-controls-esque play, stop and a button to open a build settings menu in the sidebar or something. Panic's Nova is something with a smilar design in that it's a mac exclusive but also using native design to macOS, Code should be the same for elemetary OS. |
@hanaral I separated the run button into it's own section. I have to implement some more logic to add a stop button. |
I meant something a bit more like this:
Maybe add dummy buttons for now since I don't see it being optimal to PR as-is (from a UX standpoint) |
Okay let's keep it simple and focus on building, installing and running a project in this PR. @hanaral collect features in your issue. So that we can split this up into smaller PRs. |
@meisenzahl Thanks for working on this - it will be useful. Not sure if it is still in progress and should be marked "draft"?
|
I'm not entirely sold on the idea of installing projects systemwide from Code at all. In general, I avoid installing projects compiled from source as much as possible. Given the fact that we're expecting 3rd party applications to be built as flatpaks in the near future, and flatpak has a much more consistent and predictable build process that supports any build system (meson, cmake, GNU make etc), I think that would be a more worthwhile use of time. And installing/running built flatpaks doesn't have so much potential to mess up the system. |
But Code is for development not for installing things per se. The default should definitely be to build, not install. However, for testing some things need to be installed so that should be available. I agree that installing into a VM or something other than a production machine is probably wise. |
Could you expand a bit more how this relates to using Code? Do you mean Code should only build Flatpaks? |
I'm saying that this PR currently adds support for building and installing meson based projects. As you say, to test many of these, they have to be installed which you have to be aware of the consequences of. Given that most elementary repos now have a Flatpak manifest, and all 3rd party apps that a lot of people probably use Code to develop will be Flatpak based too. It might be more worthwhile having this PR add support for building and running Flatpak packages. It solves the problem of needing to install the apps (since they're sandboxed) and it has the bonus of being an easier build system format to support in Code. |
@jeremypw thanks for your comments! I will address the points. I will focus in this PR on being able to build and launch a project through the UI. @davidmhewitt has already noted that system-wide install is not ideal, but necessary in many cases. So I'm open to whether we build the project, install system-wide and launch it, or use Flatpak. |
Current state
Peek.2021-02-07.09-54.mp4I'm open to suggestions for the build icon. |
It looks from the above comment that integrating flatpak-builder inside a Code sandbox is not going to be feasible any time soon. I would be happy if Code were able to detect whether the host had flatpak-builder installed and either only showed the "build and run" tools if it is or gave a suitable error message if the tools were used in its absence. Is that possible? |
@jeremypw I have included a query to see if Code is running flatpaked. Accordingly, the commands are then executed. If Code is flatpaked, the commands are executed via If This was the easiest way to test the functionality. I am looking forward to your feedback :) |
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.
After correcting the issue with the private getter, this PR successfully built a flatpak of itself (took quite a long time). However, pressing "run" gave the error
error: Build directory /home/jeremy/Documents/Elementary/clones/meisenzahl/code/.flatpak-builder/rofiles/rofiles-MdFy9A not initialised, use flatpak build-init
The flatpak was not installed but presumably this is intended.
This was running this PR as a native install. Not tested in a flatpak yet.
Need to think about showing progress somehow - preferably show terminal output. Also about how to prevent the user editing source files in the middle of a build. Not sure what closing Code in the middle of build will do either.
Presumably "Run" runs the previously build flatpak - but what if the files have been edited? Either need to rebuild or maybe disable "run" until there is an up to date build.
Making progress!
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.
Some comments about when running in sandbox.
public bool has_dependencies_installed { | ||
get { | ||
if (is_running_flatpaked) { | ||
return run_command_sync ({ | ||
"flatpak-spawn", | ||
"--host", | ||
"flatpak-builder", | ||
"--version" | ||
}); | ||
} else { | ||
return run_command_sync ({ | ||
"flatpak-builder", | ||
"--version" | ||
}); | ||
} | ||
} | ||
} |
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.
There's no need to check if flatpak-builder is installed when sandboxed, we know it is because we build them in the manifest. However, flatpak-builder also have a flatpak version of itself, so we should check for flatpak-builder --version
and flatpak run org.flatpak.Builder --version
when running natively.
if (is_running_flatpaked) { | ||
return yield run_command_async ({ | ||
"flatpak-spawn", | ||
"--host", | ||
"flatpak-builder", | ||
"--force-clean", | ||
flatpak_manifest.build_dir, | ||
flatpak_manifest.manifest | ||
}); |
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.
no need for flatpak-spawn --host
here, flatpak-builder when sandboxed know how to call the flatpak on the host.
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.
If I don't call flatpak-build
via flatpak-spawn
, it seems to start differently. Because I get error messages then:
error: io.elementary.Sdk/x86_64/6 not installed
Failed to init: Unable to find sdk io.elementary.Sdk version 6
With this change it seems that construction will take place in the containerized environment. There io.elementary.Sdk
and io.elementary.Platform
are missing.
if (is_running_flatpaked) { | ||
return yield run_command_async ({ | ||
"flatpak-spawn", | ||
"--host", | ||
"flatpak-builder", | ||
"--run", | ||
flatpak_manifest.build_dir, | ||
flatpak_manifest.manifest, | ||
flatpak_manifest.command | ||
}); |
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.
same as above
Yes, it is intended that the built app is not installed. These rofiles errors only appear with some apps that require extended permissions.
Source: Flatpak Builder Command Reference Maybe @Marukesu can explain it better 😅 The logic also works as a Flatpak. I test the Flatpak version of Code with the following script: #!/bin/sh
# fail on first error
set -e
APP=io.elementary.code
flatpak-builder --repo=repo --disable-rofiles-fuse build/flatpak ${APP}.yml --force-clean
flatpak build-bundle repo ${APP}.flatpak --runtime-repo=https://flatpak.elementary.io/repo.flatpakrepo ${APP} master
flatpak install --user -y ${APP}.flatpak
flatpak run ${APP}
There is the terminal plugin that we could use, but that is disabled by default. If I'm honest, I'd rather implement the terminal output in a follow up PR. The history of this PR is already quite long and it's getting a bit confusing. The foundation stones with the signals of
If you close Code while building, the process still continues 😬 I will try to fix this. I wouldn't try to be too clever here either. All the IDEs I have used so far have not prevented the code from being edited while the project is being built. |
I added Installer's Terminal widget to show the output of I put the terminal widget in a I would appreciate feedback from @elementary/UX on the implementation. |
@@ -353,6 +398,11 @@ namespace Scratch { | |||
on_plugin_toggled (bottombar); | |||
}); | |||
|
|||
// Terminal |
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.
Its not really a terminal is it? Its a TextView. Calling it a terminal might cause confusion with the terminal plugin.
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.
How about something along the lines of CommandOutput
?
I am still having trouble getting the "Run" tool to work on e.g. the Calculator Flatpak (which seems to be one of the simplest with no unusual permissions). I am still getting an error in the form Not sure where to go from here. Is this an upstream problem perhaps? |
I'm also stuck on this. @Marukesu do you have any idea how we can prevent the |
are you running on host? that's a problem with the in the flatpak version, the issue is different, for some reason, it can't access the x11 display (the same happens with the org.flatpak.Builder flatpak). I saw that GNOME Builder uses |
@meisenzahl Some of these changes are not directly related to build/run Flatpak projects. How about we spin them off in a set of smaller PR so they can be approved faster and use this one only for flatpak related functions? |
Related to #939
It's been quite a long time since I played around with it. After merging
master
the logic is broken in some places.