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

feat: make --no-sandbox optional for building with AppImage #6429

Merged

Conversation

devinbinnie
Copy link
Contributor

I've noticed that --no-sandbox is hardcoded into the arguments list when building an AppImage. This seems to be a good default for many implementations, but doesn't work for all of them. Example being the Mattermost Desktop App (see this issue: mattermost/desktop#1804)

I've created this PR in order to potentially add an override option for users to specify their own arguments when launching the AppImage, using the executableArgs field which doesn't appear to be used by the AppImage at this time.

Let me know if this change makes sense and if I can make any further edits to support this.

@changeset-bot
Copy link

changeset-bot bot commented Nov 17, 2021

⚠️ No Changeset found

Latest commit: e8eec64

Merging this PR will not cause a version bump for any packages. If these changes should not result in a new version, you're good to go. If these changes should result in a version bump, you need to add a changeset.

This PR includes no changesets

When changesets are added to this PR, you'll see the packages that this PR includes changesets for and the associated semver types

Click here to learn what changesets are, and how to add one.

Click here if you're a maintainer who wants to add a changeset to this PR

@@ -20,7 +20,7 @@ export default class AppImageTarget extends Target {
super("appImage")

this.desktopEntry = new Lazy<string>(() =>
helper.computeDesktopEntry(this.options, "AppRun --no-sandbox %U", {
helper.computeDesktopEntry(this.options, `AppRun ${[...(this.options.executableArgs ?? ["--no-sandbox"]), "%U"].join(" ")}`, {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Can you please split in to its own const?

const args = this.options.executableArgs?.join(" ") || "--no-sandbox"
helper.computeDesktopEntry(this.options, `AppRun ${args} %U`, {

@mmaietta mmaietta changed the title Make --no-sandbox optional for building with AppImage feat: make --no-sandbox optional for building with AppImage Nov 19, 2021
@devinbinnie devinbinnie requested a review from mmaietta November 22, 2021 14:44
@mmaietta
Copy link
Collaborator

Can you please rebase off latest master? That should resolve the test failures

@devinbinnie devinbinnie force-pushed the appimage_optional_no_sandbox branch from 8d776e8 to 4838fcf Compare November 23, 2021 14:08
@devinbinnie
Copy link
Contributor Author

@mmaietta Done, I think I accidentally wiped out one of the .md files you added though. Might need to be re-added, let me know if I can help with that, apologies.

@mmaietta mmaietta merged commit e95afc1 into electron-userland:master Nov 25, 2021
taratatach added a commit to cozy-labs/cozy-desktop that referenced this pull request Dec 9, 2022
  This runtime option disables the chromium sandbox. It was added as a
  default option in `electron-builder` v22.10.3 (see
  electron-userland/electron-builder#4496)
  because the required kernel privileges might not be available on the
  user's computer (e.g. in Debian).

  However, these privileges might be available on some computers and the
  sandbox should be used in this case.
  Thankfully, `electron-builder` v22.14.8 makes this optional when using
  the build option `executableArgs` (see
  electron-userland/electron-builder#6429).

  Since we don't have any arguments to pass to Cozy Desktop, we'll just
  add an empty argument to the list to prevent the use of `--no-sandbox`
  in the Desktop entry.
  We'll still add the option ourselves when necesary (see
  ./build/launcher-script.sh`).
taratatach added a commit to cozy-labs/cozy-desktop that referenced this pull request Dec 12, 2022
  This runtime option disables the chromium sandbox. It was added as a
  default option in `electron-builder` v22.10.3 (see
  electron-userland/electron-builder#4496)
  because the required kernel privileges might not be available on the
  user's computer (e.g. in Debian).

  However, these privileges might be available on some computers and the
  sandbox should be used in this case.
  Thankfully, `electron-builder` v22.14.8 makes this optional when using
  the build option `executableArgs` (see
  electron-userland/electron-builder#6429).

  Since we don't have any arguments to pass to Cozy Desktop, we'll just
  add an empty argument to the list to prevent the use of `--no-sandbox`
  in the Desktop entry.
  We'll still add the option ourselves when necesary (see
  ./build/launcher-script.sh`).
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants