Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
6 changes: 6 additions & 0 deletions app/src/lib/editors/found-editor.ts
Original file line number Diff line number Diff line change
@@ -1,5 +1,11 @@
export interface IFoundEditor<T> {
readonly editor: T
readonly path: string
/**
* Indicate to Desktop to launch the editor with the `shell: true` option included.

Copy link
Copy Markdown
Owner Author

Choose a reason for hiding this comment

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

🎨 I plan to upstream this change as this context was helpful and perhaps overlooked at the time

*
* This is available to all platforms, but is only currently used by some Windows
* editors as their launch programs end in `.cmd`
*/
readonly usesShell?: boolean
}
22 changes: 21 additions & 1 deletion app/src/lib/editors/launch.ts
Original file line number Diff line number Diff line change
@@ -1,7 +1,25 @@
import { spawn, SpawnOptions } from 'child_process'
import { pathExists } from 'fs-extra'
import { pathExists as pathExistsDefault } from 'fs-extra'
import { pathExists as pathExistsLinux, spawnEditor } from '../helpers/linux'
import { ExternalEditorError, FoundEditor } from './shared'

/**
* Use a platform-specific pathExists based on the platform, to simplify changes
* to the application logic
*
* @param path the location of some program on disk
*
* @returns `true` if the path exists on disk, or `false` otherwise
*
*/
function pathExists(path: string) {
if (__LINUX__) {
return pathExistsLinux(path)
} else {
return pathExistsDefault(path)
}
}

/**
* Open a given file or folder in the desired external editor.
*
Expand Down Expand Up @@ -35,6 +53,8 @@ export async function launchExternalEditor(
// In macOS we can use `open`, which will open the right executable file
// for us, we only need the path to the editor .app folder.
spawn('open', ['-a', editorPath, fullPath], opts)
} else if (__LINUX__) {
spawnEditor(editorPath, fullPath, opts)
} else {
spawn(editorPath, [fullPath], opts)
}
Expand Down
8 changes: 6 additions & 2 deletions app/src/lib/editors/linux.ts
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
import { pathExists } from 'fs-extra'
import { pathExists } from '../helpers/linux'

import { IFoundEditor } from './found-editor'

Expand Down Expand Up @@ -34,7 +34,11 @@ const editors: ILinuxExternalEditor[] = [
},
{
name: 'VSCodium',
paths: ['/usr/bin/codium', '/var/lib/flatpak/app/com.vscodium.codium'],
paths: [
'/usr/bin/codium',
'/var/lib/flatpak/app/com.vscodium.codium',
'/usr/share/vscodium-bin/bin/codium',
],
},
{
name: 'Sublime Text',
Expand Down
93 changes: 93 additions & 0 deletions app/src/lib/helpers/linux.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,93 @@
import { join } from 'path'
import { pathExists as pathExistsInternal } from 'fs-extra'
import {
ChildProcess,
spawn,
SpawnOptionsWithoutStdio,
SpawnOptions,
} from 'child_process'

export function isFlatpakBuild() {
return __LINUX__ && process.env.FLATPAK_HOST === '1'

Copy link
Copy Markdown
Owner Author

Choose a reason for hiding this comment

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

I started with a helper method to detect whether we're in a flatpak build. This is now all internal, so I can drop the export above if we're happy with the other changes.

}

/**
* Convert an executable path to be relative to the flatpak host
*
* @param path a path to an executable relative to the root of the filesystem
*
*/
export function convertToFlatpakPath(path: string) {
if (!__LINUX__) {
return path
}

if (path.startsWith('/opt/')) {
return path
}

return join('/var/run/host', path)

Copy link
Copy Markdown
Owner Author

Choose a reason for hiding this comment

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

🤔 if there are cases where we shouldn't be applying this pattern let me know, ideally with the expected inputs and resulting paths. Having to adapt this behaviour based on the path type might make this a bit more complex, but I still think this approach is feasible...

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

/opt/ is the only path we don't need to apply this logic too

Copy link
Copy Markdown
Owner Author

Choose a reason for hiding this comment

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

Oh cool, you answered the question I just posted here #555 (comment) - thanks!

}

/**
* Checks the file path on disk exists before attempting to launch a specific shell
*
* @param path
*
* @returns `true` if the path can be resolved, or `false` otherwise
*/
export async function pathExists(path: string): Promise<boolean> {
if (isFlatpakBuild()) {
path = convertToFlatpakPath(path)

Copy link
Copy Markdown
Owner Author

Choose a reason for hiding this comment

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

🤔 as I understand it, we want to convert program paths before checking if we're running in a flatpak context as they exist in a special location, but we don't need to do this when we want to launch the program - flatpak-spawn will do that itself as long as we call it with --host.

Let me know if this logic is incorrect, or if I've misunderstood.

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

yup this is correct. the path outside the container should be real but detection for binaries in /usr should be pointed to /var/run/host

Copy link
Copy Markdown
Owner Author

Choose a reason for hiding this comment

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

What about for other special directories we're using for editors:

  • /opt
  • /snap
  • /var/lib/flatpak (launching another flatpak program)

Should those also go through this conversion process before we try and open them with flatpak-spawn?

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

I think /var/lib/flatpak might be blocked by flatpak not sure(I'll check and update this), /snap is one that won't work since this dir won't be mounted

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

yeah /var/lib/flatpak is blocked

Copy link
Copy Markdown
Owner Author

Choose a reason for hiding this comment

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

👍🏻 I'm assuming that the try-catch below will ensure these errors will be handled and swallowed by the app, so I'm going to try and avoid getting too complex with the logic in here for now for paths that we know are inaccessible in the flatpak context.

}

try {
return await pathExistsInternal(path)
} catch {
return false
}
}

/**
* Spawn a particular shell in a way that works for Flatpak-based usage
*
* @param path path to shell, relative to the root of the filesystem
* @param args arguments to provide to the shell
* @param options additional options to provide to spawn function
*
* @returns a child process to observe and monitor
*/
export function spawnShell(
path: string,
args: string[],
options?: SpawnOptionsWithoutStdio
): ChildProcess {
if (isFlatpakBuild()) {
return spawn('flatpak-spawn', ['--host', path, ...args], options)

Copy link
Copy Markdown
Owner Author

Choose a reason for hiding this comment

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

🤔 I think this is the convention for mapping the original command (and parameters) to work with flatpak-spawn, but now that it's all centralized we can tweak this to suit.

}

return spawn(path, args, options)
}

/**
* Spawn a given editor in a way that works for Flatpak-based usage
*
* @param path path to editor, relative to the root of the filesystem
* @param workingDirectory working directory to open initially in editor
* @param options additional options to provide to spawn function
*/
export function spawnEditor(
path: string,
workingDirectory: string,
options: SpawnOptions
): ChildProcess {
if (isFlatpakBuild()) {
return spawn(
'flatpak-spawn',
['--host', path, `"${workingDirectory}"`],
options
)
} else {
return spawn(path, [workingDirectory], options)
}
}
26 changes: 15 additions & 11 deletions app/src/lib/shells/linux.ts
Original file line number Diff line number Diff line change
@@ -1,8 +1,8 @@
import { spawn, ChildProcess } from 'child_process'
import { pathExists } from 'fs-extra'
import { ChildProcess } from 'child_process'
import { assertNever } from '../fatal-error'
import { IFoundShell } from './found-shell'
import { parseEnumValue } from '../enum'
import { pathExists as pathExistsLinux, spawnShell } from '../helpers/linux'

export enum Shell {
Gnome = 'GNOME Terminal',
Expand All @@ -27,7 +27,7 @@ export function parse(label: string): Shell {
}

async function getPathIfAvailable(path: string): Promise<string | null> {
return (await pathExists(path)) ? path : null
return (await pathExistsLinux(path)) ? path : null
}

function getShellPath(shell: Shell): Promise<string | null> {
Expand Down Expand Up @@ -164,21 +164,25 @@ export function launch(
case Shell.Terminator:
case Shell.XFCE:
case Shell.Alacritty:
return spawn(foundShell.path, ['--working-directory', path])
return spawnShell(foundShell.path, ['--working-directory', path])
case Shell.Urxvt:
return spawn(foundShell.path, ['-cd', path])
return spawnShell(foundShell.path, ['-cd', path])
case Shell.Konsole:
return spawn(foundShell.path, ['--workdir', path])
return spawnShell(foundShell.path, ['--workdir', path])
case Shell.Xterm:
return spawn(foundShell.path, ['-e', '/bin/bash'], { cwd: path })
return spawnShell(foundShell.path, ['-e', '/bin/bash'], { cwd: path })
case Shell.Terminology:
return spawn(foundShell.path, ['-d', path])
return spawnShell(foundShell.path, ['-d', path])
case Shell.Deepin:
return spawn(foundShell.path, ['-w', path])
return spawnShell(foundShell.path, ['-w', path])
case Shell.Elementary:
return spawn(foundShell.path, ['-w', path])
return spawnShell(foundShell.path, ['-w', path])
case Shell.Kitty:
return spawn(foundShell.path, ['--single-instance', '--directory', path])
return spawnShell(foundShell.path, [
'--single-instance',
'--directory',
path,
])
default:
return assertNever(shell, `Unknown shell: ${shell}`)
}
Expand Down
20 changes: 19 additions & 1 deletion app/src/lib/shells/shared.ts
Original file line number Diff line number Diff line change
@@ -1,9 +1,10 @@
import { ChildProcess } from 'child_process'
import { pathExists } from 'fs-extra'
import { pathExists as pathExistsDefault } from 'fs-extra'

import * as Darwin from './darwin'
import * as Win32 from './win32'
import * as Linux from './linux'
import { pathExists as pathExistsLinux } from '../helpers/linux'
import { IFoundShell } from './found-shell'
import { ShellError } from './error'

Expand Down Expand Up @@ -72,6 +73,23 @@ export async function findShellOrDefault(shell: Shell): Promise<FoundShell> {
}
}

/**
* Use a platform-specific pathExists based on the platform, to simplify changes
* to the application logic
*
* @param path the location of some program on disk
*
* @returns `true` if the path exists on disk, or `false` otherwise
*
*/
function pathExists(path: string) {
if (__LINUX__) {
return pathExistsLinux(path)
} else {
return pathExistsDefault(path)
}
}

/** Launch the given shell at the path. */
export async function launchShell(
shell: FoundShell,
Expand Down
30 changes: 30 additions & 0 deletions app/test/unit/helpers/linux-test.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,30 @@
import { convertToFlatpakPath } from '../../../src/lib/helpers/linux'

describe('convertToFlatpakPath()', () => {
if (__LINUX__) {
it('converts /usr paths', () => {
const path = '/usr/bin/subl'
const expectedPath = '/var/run/host/usr/bin/subl'
expect(convertToFlatpakPath(path)).toEqual(expectedPath)
})

it('preserves /opt paths', () => {
const path = '/opt/slickedit-pro2018/bin/vs'
expect(convertToFlatpakPath(path)).toEqual(path)
})
}

if (__WIN32__) {
it('returns same path', () => {
const path = 'C:\\Windows\\System32\\Notepad.exe'
expect(convertToFlatpakPath(path)).toEqual(path)
})
}

if (__DARWIN__) {
it('returns same path', () => {
const path = '/usr/local/bin/code'
expect(convertToFlatpakPath(path)).toEqual(path)
})
}
})