-
Notifications
You must be signed in to change notification settings - Fork 30k
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
Support building Flatpak application bundles #16169
Conversation
Hi @aperezdc, I'm your friendly neighborhood Microsoft Pull Request Bot (You can call me MSBOT). Thanks for your contribution! TTYL, MSBOT; |
], | ||
finishArgs: [ | ||
'--share=ipc', '--socket=x11', // Allow showing X11 windows. | ||
'--filesystem=home', // Allow access to home directory. |
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.
I think for a text editor, exposing as much of the filesystem as possible would be a good idea. So use --filesystem=host
instead. That's what gnome-builder does, for example.
finishArgs: [ | ||
'--share=ipc', '--socket=x11', // Allow showing X11 windows. | ||
'--filesystem=home', // Allow access to home directory. | ||
'--device=dri', // Allow OpenGL rendering. |
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.
Other finish args to consider
--socket=pulseaudio
if the app or any app extensions need audio output--share=network
to install extensions using the network connection--talk-name=org.freedesktop.Notifications
if this even uses libnotify for desktop notifications
Lastly a bit weird, but adding --filesystem=/tmp
will make sure you get the system tmp directory, which chromium/electron is using for its single check. https://github.com/electron/electron/blob/master/chromium_src/chrome/browser/process_singleton_posix.cc#L12
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.
I think we can add all of these:
- Indeed
--share=network
is a good idea, somehow I missed the fact that the application goes online to fetch extensions. Not to mention that it may want to push/pull from Git repositories! - As for notifications, VSCode itself does not use them, and checking the popular extensions page I do not see how any of them would like to use them, and searching for “notification” brings up zero results... but anyway it's a small allowance, but it doesn't hurt having it.
- Funnily enough, there's a few extensions which use audio devices, so let's add that, too!
Pushed an updated version with @mattdangerw's suggestions applied. |
...and updated again to symlink flatpak run com.microsoft.CodeOSS --wait --goto file.js:10:3 (Probably people using the Flatpak will want to do |
@aperezdc we can't automatically install a reliable bin command due to the lack of |
@Tyriar Sorry, I do not understand what do you mean exactly... Are you wondering about the addons:
apt:
packages:
- imagemagick
- flatpak I'll try to add the needed bits to the Or do you mean something else? |
...but I see that @Tyriar I think now I get what you meant. And no, we cannot install things in the host machine when using Flatpak, because Flatpak bundles are self-contained and the applications run in a sandbox. Not even the libraries from the host are used: the ones from the runtime/version specified at build time are used, which ensures the application will keep running regardless of what changes in the host. This is by design. On the upside, notice how the |
@aperezdc I was mainly just wondering is that's typical for using applications installed via flatpak, I have my answer now 😄 I'll look at merging this after the November/December end game happens as things are a bit busy currently. Could you sign the CLA in #16169 (comment) in the meantime? Thanks again! |
I have signed the CLA a few minutes ago. Does @msftclas notice automatically or do we have to poke it somehow? |
@aperezdc it normally happens automatically I think, I'll close and reopen the PR and see if that makes a difference. |
Hi @aperezdc, I'm your friendly neighborhood Microsoft Pull Request Bot (You can call me MSBOT). Thanks for your contribution! The agreement was validated by Microsoft and real humans are currently evaluating your PR. TTYL, MSBOT; |
There we go 😄 |
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.
Most of the 1.8 release stuff is out of the way so I can look at this again 😄
const linuxPackageRevision = Math.floor(new Date().getTime() / 1000); | ||
|
||
const flatpakManifest = { | ||
appId: 'com.microsoft.CodeOSS', |
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.
Our application id in the other packages is code[-insiders].desktop
, should this be the same or is this a new ID we would be introducing? This should be sourced from https://github.com/Microsoft/vscode/blob/master/product.json
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.
Flatpak application identifiers need to be reverse-domain style. Now that I come back to review this, I see that we could reuse com.visualstudio.code.oss
(it is the value for darwinBundleIdentifier
).
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.
Yeah reusing that should be fine for now.
appId: 'com.microsoft.CodeOSS', | ||
sdk: 'org.freedesktop.Sdk', | ||
runtime: 'org.freedesktop.Sdk', | ||
runtimeVersion: '1.4', |
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.
What version is this for?
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.
This is the version of the runtime bundle that the application needs. It is used both for running (org.freedesktop.Platform
) and building (org.freedesktop.Sdk
). The current version is 1.4
. Setting this ensures that the application will be always ran using an userland which conforms exactly to the ABI exposed by the 1.4 version of the runtime.
runtimeVersion: '1.4', | ||
base: 'io.atom.electron.BaseApp', | ||
baseFlatpakref: 'https://s3-us-west-2.amazonaws.com/electron-flatpak.endlessm.com/electron-base-app-master.flatpakref', | ||
command: "code-oss", |
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.
command: product.applicationName
baseFlatpakref: 'https://s3-us-west-2.amazonaws.com/electron-flatpak.endlessm.com/electron-base-app-master.flatpakref', | ||
command: "code-oss", | ||
symlinks: [ | ||
['/share/code-oss/bin/code-oss', '/bin/code-oss'], |
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.
This should use product.applicationName
instead of code-oss
const destination = '.build/linux/flatpak/' + flatpakArch; | ||
|
||
return function () { | ||
const all = [16, 24, 32, 48, 64, 128, 192, 256, 512].map(function (size) { |
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.
Are all these sizes necessary? The smaller ones in particular would probably look pretty crappy being scaled like this?
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.
<image>https://code.visualstudio.com/home/home-screenshot-linux-lg.png</image> | ||
<caption>Editing a TypeScript file and searching for extensions.</caption> | ||
</screenshot> | ||
<screenshot> |
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.
Let's just keep the default screenshot for now, I've created #17131 to follow this up.
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.
Updated to have only the default screenshot.
<summary>Code editor for developers supporting integration with existing tools</summary> | ||
<description> | ||
<p> | ||
Visual Studio Code is a lightweight but powerful source code editor which |
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.
Let's go with the standard description now as defined in #17130, also could you put it all one line? <p>Visual Studio...
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.
Using the description from #17130 results in:
vscode % appstream-util validate resources/linux/code.appdata.xml
resources/linux/code.appdata.xml: FAILED:
• tag-missing : <translation> not specified
• tag-missing : <update_contact> is not present
• style-invalid : <p> cannot contain a hyperlink [Visual Studio Code is a new choice of tool that combines the simplicity of a code editor with what developers need for the core edit-build-debug cycle. See https://code.visualstudio.com/docs/setup/linux for installation instructions and FAQ.]
Using validate-relax
still passes. Should we keep the URL in the description, or can I just remove the last sentence?
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.
Removing the last line would be great. I'll fix up <translation>
later and can you add this:
<update_contact>vscode-linux@microsoft.com</update_contact>
<?xml version="1.0" encoding="UTF-8"?> | ||
<component type="desktop"> | ||
<id>@@NAME@@.desktop</id> | ||
<metadata_license>CC-BY-SA-3.0</metadata_license> |
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.
@chrisdias do we have a preferred license for metadata files like these?
<project_license>MIT</project_license> | ||
<name>@@NAME_LONG@@</name> | ||
<url type="homepage">https://code.visualstudio.com</url> | ||
<summary>Code editor for developers supporting integration with existing tools</summary> |
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.
As in #17130, let's make this "Code editing. Redefined." for now
<component type="desktop"> | ||
<id>@@NAME@@.desktop</id> | ||
<metadata_license>CC-BY-SA-3.0</metadata_license> | ||
<project_license>MIT</project_license> |
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.
This will need to change to be based on product.json but I just realized we don't do it for the rpm package so this is work I'll need to follow up 😄 #17133
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.
Picking the value from product.json
once that PR is landed will be just perfect 👍
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.
Merged
@@ -0,0 +1,46 @@ | |||
<?xml version="1.0" encoding="UTF-8"?> |
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.
Could you check that this validates against:
appstream-util validate code.appdata.xml
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.
Checked, and fixed a couple of issues. Right now it says:
vscode % appstream-util validate resources/linux/code.appdata.xml
resources/linux/code.appdata.xml: FAILED:
• tag-missing : <translation> not specified
• tag-missing : <update_contact> is not present
Validation of files failed
These are optional tags, and validate-relax
succeds:
vscode % appstream-util validate-relax resources/linux/code.appdata.xml
resources/linux/code.appdata.xml: OK
vscode %
This adds support for building Flatpak application bundles, which should be runnable on any Linux distribution with Flatpak support. See the page at http://flatpak.org/getting.html New Gulp tasks are added to prepare a directory with the needed files (vscode-linux-${arch}-prepare-flatpak), assembling the Flatpak bundles (vscode-linux-${arch}-flatpak), and for cleaning the build directories (clean-vscode-linux-${arch}-flatpak). This mimics how the Debian and RPM package creation works. Hence, building an application bundle is done with: $ gulp vscode-linux-x64-flatpak [...] $ ls *.flatpak com.visualstudio.code.oss-x86_64.flatpak Installing and running the application is achieved with: $ flatpak --user install --bundle com.visualstudio.code.oss-x86_64.flatpak $ flatpak run com.visualstudio.code.oss (Removing "--user" would install the application system-wide. Also note that the "org.freedesktop.Platform" runtime needs to be installeed, check the Flatpak website for instructions.) The "flatpak-bundler" development dependency provides an asynchronous API to invoke "flatpak-builder". The "gulp-image-resize" module is used to incorporate scaling of the application icon to multiple sizes, and uses ImageMagick (or GraphicsMagick) under the hood, which is commonly available on most GNU/Linux distributions. Instead of building the Electron dependencies ourselves, this uses the io.atom.electron.BaseApp bundle as base application. Instead of building the Electron dependencies, this reuses the "io.atom.electron.BaseApp" bundle as base application. The contents of the base application are imported at build time, and then Electron and the prepared files added on top. The prebuilt base application will be fetched automatically using the URL specified in the "baseFlatpakref" attribute of the manifest, and it is kindly hosted at Amazon S3 by Endless Computers (https://endlessm.com). The sources for the base application can be found at: https://github.com/endlessm/electron-flatpak-base-app/
@Tyriar: I have pushed again, this time only with the commit that adds the initial support (the rest of the commits were stashed to a flatpak-follow-up branch, to be submitted later). All the issues mentioned in the review (thanks!) were addressed. |
@aperezdc thanks for the great work 😄 we probably won't distribute flatpaks just yet, but they should now be available for OSS builds! Looking forward to getting appdata.xml in as well. |
This adds support for building Flatpak application bundles, which should be runnable on any Linux distribution with Flatpak support.
New Gulp tasks are added to prepare a directory with the needed files (
vscode-linux-${arch}-prepare-flatpak
), assembling the Flatpak bundles (vscode-linux-${arch}-flatpak
), and for cleaning the build directories (clean-vscode-linux-${arch}-flatpak
). This mimics how the Debian and RPM package creation works. Hence, building an application bundle is done with:Installing and running the application is achieved with:
(Removing
--user
would install the application system-wide.)The
org.freedesktop.Platform
runtime needs to be installed for running the application, and for making builds theorg.freedesktop.Sdk
runtime is needed as well. There are installation instructions in the Flatpak website; the short version is:The flatpak-bundler development dependency provides an asynchronous API to invoke
flatpak-builder
. The gulp-image-resize module is used toincorporate scaling of the application icon to multiple sizes, and uses ImageMagick (or GraphicsMagick) under the hood, which is commonly available on most GNU/Linux distributions.
Instead of building the Electron dependencies, this reuses the
io.atom.electron.BaseApp
(source) bundle as base application. The contents of the base application are imported at build time, and then Electron and the prepared files added on top. The prebuilt base application will be fetched automatically using the URL specified in thebaseFlatpakref
attribute, and it is kindly hosted at Amazon S3 by Endless Computers.Merging this would tackle issue #7112.