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.

Sign up for GitHub

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: Extending linux executableArgs option to be utilized for Snap target (fixes #4587) #7198

Merged
merged 2 commits into from
Oct 20, 2022
Merged

feat: Extending linux executableArgs option to be utilized for Snap target (fixes #4587) #7198

Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
8ef0f19
Fix #4587 executableArgs option ignored for snap target bug
fangpenlin Oct 20, 2022
7bf0f81
Create empty-ligers-turn.md
mmaietta Oct 20, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
9 changes: 8 additions & 1 deletion packages/app-builder-lib/src/targets/snap.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -195,12 +195,19 @@ export default class SnapTarget extends Target {
Icon: "${SNAP}/meta/gui/icon.png",
})

const extraAppArgs: Array<string> = options.executableArgs ?? []
if (this.isElectronVersionGreaterOrEqualThan("5.0.0") && !isBrowserSandboxAllowed(snap)) {
args.push("--extraAppArgs=--no-sandbox")
const noSandboxArg = "--no-sandbox"
if (!extraAppArgs.includes(noSandboxArg)) {
extraAppArgs.push(noSandboxArg)
}
if (this.isUseTemplateApp) {
args.push("--exclude", "chrome-sandbox")
}
}
if (extraAppArgs.length > 0) {
args.push("--extraAppArgs=" + extraAppArgs.join(" "))
Copy link
Collaborator

@mmaietta mmaietta Oct 20, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

If extraAppArgs = ['--test', '--test2'], then this would fail, right?
It would be --extraAppArgs=--test --test2. Do we need to wrap it in double quotes? (i.e. --extraAppArgs="--test --test2 --no-sandbox")

Copy link
Collaborator

Choose a reason for hiding this comment

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

I would simplify this to be:

const executableArgs: string[] = options.executableArgs ?? []
if (this.isElectronVersionGreaterOrEqualThan("5.0.0") && !isBrowserSandboxAllowed(snap)) {
  executableArgs.push('--no-sandbox')
  if (this.isUseTemplateApp) {
    args.push("--exclude", "chrome-sandbox")
  } 
}
const extraArgs = new Array(new Set(executableArgs)) // remove all duplicate entries
if (extraArgs.length) {
  args.push(`--extraAppArgs="${extraArgs.join(" ")}"`)  
}

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I tested it with

executableArgs: ["--no-sandbox", "--disable-seccomp-filter-sandbox"]

for solving a problem of my app similar to this one here and it's working as expected for my app. The args are passed to underlaying os execv-like system call as string array as the argument instead of a piece of shell argument string, the app-builder-bin shall receive the value as is, so we probably don't need to escape the value there

for removing duplicate arguments, personally I would prefer to lean toward more of keeping the argument as is, because

  • the order of argument might matter, like ["--run-a", "--run-b"] and ["--run-b", "--run-a"] might have different implication
  • repeating argument might be meaningful for some cases, such as ["--verbose", "--verbose", "--verbose"], while this might not be the best example and best argument design in the world, there might still be some legit use cases where you want to pass repeating argument, like you want to test your electron argument handler and ensure when seeing duplicate argument it throws error

Copy link
Collaborator

Choose a reason for hiding this comment

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

Very interesting and great reasoning. I like it 👍

Copy link
Collaborator

Choose a reason for hiding this comment

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

Quick note: Since this could impact all linux builds with executableArgs set and this was never supported for Snap, I would consider this more of a feature or breaking change. (Luckily we have v24-alpha en route so we can include this there as a Breaking Change just to play it safe)

}

if (snap.compression != null) {
args.push("--compression", snap.compression)
Expand Down