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

Add build and run controls for Flatpak based projects #940

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

Conversation

meisenzahl
Copy link
Member

@meisenzahl meisenzahl commented Feb 6, 2021

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.

Bildschirmfoto von 2021-02-06 09 20 34

@hanaral
Copy link

hanaral commented Feb 6, 2021

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.
I'm going to post some more screenshots in the main issue related to this

@meisenzahl
Copy link
Member Author

@hanaral I separated the run button into it's own section.

Bildschirmfoto von 2021-02-06 09 41 47

I have to implement some more logic to add a stop button.

@hanaral
Copy link

hanaral commented Feb 6, 2021

@hanaral I separated the run button into it's own section.

I meant something a bit more like this:
107103342-3c49bf00-6815-11eb-86c3-7a2063f63184

I have to implement some more logic to add a stop button.

Maybe add dummy buttons for now since I don't see it being optimal to PR as-is (from a UX standpoint)

@meisenzahl
Copy link
Member Author

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 meisenzahl marked this pull request as ready for review February 6, 2021 10:46
@jeremypw
Copy link
Collaborator

jeremypw commented Feb 6, 2021

@meisenzahl Thanks for working on this - it will be useful. Not sure if it is still in progress and should be marked "draft"?
Some comments after an initial try:

  • "Run" button should change appearance (or show/hide) depending on whether a compilable project is loaded/focused
  • Need to be able to choose to build only or build and install
  • Tooltip should indicate what action will be taken on which project
  • "Run" button is not active unless you click on a file in the sidebar even if a suitable ("meson.build") file is loaded and highlighted in the sidebar.
  • The "Run" button is not active when the project is collapsed
  • Maybe put "Build" and "Install" options in the project folder context menu?
  • Cannot see output unless running Code in a terminal

@davidmhewitt
Copy link
Member

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.

@jeremypw
Copy link
Collaborator

jeremypw commented Feb 6, 2021

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.

@jeremypw
Copy link
Collaborator

jeremypw commented Feb 6, 2021

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.

Could you expand a bit more how this relates to using Code? Do you mean Code should only build Flatpaks?

@davidmhewitt
Copy link
Member

davidmhewitt commented Feb 6, 2021

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.

@meisenzahl meisenzahl marked this pull request as draft February 7, 2021 06:36
@meisenzahl
Copy link
Member Author

@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.

@meisenzahl meisenzahl changed the title Add button to run project Add build and run controls Feb 7, 2021
@meisenzahl
Copy link
Member Author

meisenzahl commented Feb 7, 2021

Current state

  • Emit a signal for every command line output in ProjectManager
    • could be used to show it in a view in a following PR
  • Add build button
    • only build the project
  • Add stop button
    • stop a process if the project is building or has started
  • Set sensitive for build, run and stop buttons
Peek.2021-02-07.09-54.mp4

I'm open to suggestions for the build icon.

@jeremypw
Copy link
Collaborator

...I prefer to not make assumptions about what the host have installed.

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?

@meisenzahl
Copy link
Member Author

@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 flatpak-spawn on the host.

If flatpak-builder is not installed, the buttons are grayed out.

This was the easiest way to test the functionality.

I am looking forward to your feedback :)

Copy link
Collaborator

@jeremypw jeremypw left a 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!

src/Services/ProjectManager.vala Outdated Show resolved Hide resolved
Copy link
Contributor

@Marukesu Marukesu left a 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.

Comment on lines +38 to +54
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"
});
}
}
}
Copy link
Contributor

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.

Comment on lines +256 to +264
if (is_running_flatpaked) {
return yield run_command_async ({
"flatpak-spawn",
"--host",
"flatpak-builder",
"--force-clean",
flatpak_manifest.build_dir,
flatpak_manifest.manifest
});
Copy link
Contributor

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.

Copy link
Member Author

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.

Comment on lines +281 to +290
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
});
Copy link
Contributor

Choose a reason for hiding this comment

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

same as above

@meisenzahl
Copy link
Member Author

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.

Yes, it is intended that the built app is not installed.

These rofiles errors only appear with some apps that require extended permissions.

--disable-rofiles-fuse
Disable the use of rofiles-fuse to optimize the cache use via hardlink checkouts.

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}

Need to think about showing progress somehow - preferably show terminal output.

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 ProjectManager are there...

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.

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.

@meisenzahl
Copy link
Member Author

meisenzahl commented Oct 17, 2021

I added Installer's Terminal widget to show the output of ProjectManager.

Bildschirmfoto von 2021-10-17 09 16 49
Bildschirmfoto von 2021-10-17 09 17 16

I put the terminal widget in a Gtk.Revealer and put it in a Gtk.Grid together with a Gtk.Stack which holds the views. However, an area is always kept free for the Gtk.Revealer in the Gtk.Grid in gray. I would be glad about help.

Bildschirmfoto von 2021-10-17 09 17 39
Bildschirmfoto von 2021-10-17 09 17 24

I would appreciate feedback from @elementary/UX on the implementation.

@meisenzahl meisenzahl added the Needs Design Waiting for input from the UX team label Oct 17, 2021
@meisenzahl
Copy link
Member Author

I now use a Gtk.Paned instead of a Gtk.Grid. This allows to adjust the height of the terminal output.

With the visible property of the Gtk.Revealer I could achieve that no area is grayed out as placeholder for the widget.

Bildschirmfoto von 2021-10-17 11 37 18

@@ -353,6 +398,11 @@ namespace Scratch {
on_plugin_toggled (bottombar);
});

// Terminal
Copy link
Collaborator

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.

Copy link
Contributor

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?

@jeremypw
Copy link
Collaborator

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
.../calculator/.flatpak-builder/rofiles/rofiles-aTZhPj not initialised, use flatpak build-init Trying different options in build_project () including --disable-rofiles-fuse does not fix it. Nor does adding flatpak-build-init in run_project () (that results in an already initialized error).

Not sure where to go from here. Is this an upstream problem perhaps?

@meisenzahl
Copy link
Member Author

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 rofiles errors?

@Marukesu
Copy link
Contributor

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 .../calculator/.flatpak-builder/rofiles/rofiles-aTZhPj not initialised, use flatpak build-init Trying different options in build_project () including --disable-rofiles-fuse does not fix it. Nor does adding flatpak-build-init in run_project () (that results in an already initialized error).

are you running on host? that's a problem with the --metadata arg in the manifest, older versions of flatpak-builder do not removed them when calling flatpak build and --metadata has a different meaning there, but that was already fixed upstream. maybe the version on ubuntu repositories don't have the fix.

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 flatpak build directly instead of flatpak-builder for run, we could fixes both issues this way.

@igordsm
Copy link
Contributor

igordsm commented Nov 27, 2021

@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?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Needs Design Waiting for input from the UX team
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants