linux.ts: added new paths to executables #1115
Conversation
|
@shiftkey can you please review it? |
| paths: [ | ||
| '/snap/bin/atom', | ||
| '/usr/bin/atom', | ||
| '/usr/bin/env atom' |
There was a problem hiding this comment.
Have we tested this out? I feel like this is going to error because this string is expected to be a path, and having the additional parameter is not going to work...
There was a problem hiding this comment.
i've tested this so many times. this is how unix system works, and yea - you can try it yourself, just type env <binaryname> and it will start! (ofc untill this binary is exist into PATH. you can check path with env also, typing it into current terminal.)
and yes - this will fix nixos/nix issue that i've mentioned, i've also tested it localy.
There was a problem hiding this comment.
Sorry, not tested the approach but instead tested the app can handle this parameter. I fear it might blow up, but I also fear that it might skip over this scenario because this is doing a "path exists" check - defeating the intent of the change...
There was a problem hiding this comment.
ah, i haven't stumble across this issue. we always can create testing branch, merge it into and test it, i will help as i need it.
There was a problem hiding this comment.
I think I can figure out how to test this locally, but it's gonna take some focus time to dig into it. Perhaps later this week, otherwise definitely on the weekend...
There was a problem hiding this comment.
i want to help as much as i can bc i wan't to PR into nixpkgs it, but as you wish, my friend. as you say, but if i can help you - just ping me. :)
There was a problem hiding this comment.
In it's current form this approach doesn't work, as the new entries are considered paths rather than commands to run. This is where each of these arrays of paths are checked:
desktop/app/src/lib/editors/linux.ts
Lines 217 to 225 in 3d20d0d
And the pathExists method here is a wrapper to handle Flatpak-specific issues with paths:
desktop/app/src/lib/helpers/linux.ts
Lines 48 to 58 in 3d20d0d
But ultimately we call into this pathExists method which wraps the NodeJS fs to check if a file exists, which works like this:
function pathExists (path) {
return fs.access(path).then(() => true).catch(() => false)
}It'll catch any errors related to filesystem access or unexpected values and just return false.
|
I'm not familiar enough with this area to really understand what supporting this means, but here's a different approach that might work for what you need. The idea here is to handle these aliases properly (rather than trying to shove commands into paths), and check the output of diff --git a/app/src/lib/editors/linux.ts b/app/src/lib/editors/linux.ts
index 30c3c1e629..596be16694 100644
--- a/app/src/lib/editors/linux.ts
+++ b/app/src/lib/editors/linux.ts
@@ -1,3 +1,4 @@
+import { spawnSync } from 'child_process'
import { pathExists } from '../helpers/linux'
import { IFoundEditor } from './found-editor'
@@ -9,6 +10,9 @@ interface ILinuxExternalEditor {
/** List of possible paths where the editor's executable might be located. */
readonly paths: string[]
+
+ /** TODO: document what this field represents */
+ readonly nixPathAlias?: string
}
/**
@@ -18,25 +22,18 @@ interface ILinuxExternalEditor {
const editors: ILinuxExternalEditor[] = [
{
name: 'Atom',
- paths: [
- '/snap/bin/atom',
- '/usr/bin/atom',
- '/usr/bin/env atom'
- ],
+ paths: ['/snap/bin/atom', '/usr/bin/atom'],
+ nixPathAlias: 'atom',
},
{
name: 'Neovim',
- paths: [
- '/usr/bin/nvim',
- '/usr/bin/env nvim'
- ],
+ paths: ['/usr/bin/nvim'],
+ nixPathAlias: 'nvim',
},
{
name: 'Neovim-Qt',
- paths: [
- '/usr/bin/nvim-qt',
- '/usr/bin/env nvim-qt'
- ],
+ paths: ['/usr/bin/nvim-qt'],
+ nixPathAlias: 'nvim-qt',
},
{
name: 'Neovide',
@@ -299,13 +296,26 @@ const editors: ILinuxExternalEditor[] = [
},
]
-async function getAvailablePath(paths: string[]): Promise<string | null> {
+async function getAvailablePath(
+ paths: string[],
+ nixPathAlias?: string
+): Promise<string | null> {
for (const path of paths) {
if (await pathExists(path)) {
return path
}
}
+ if (nixPathAlias) {
+ const { stdout: resolvedPath } = spawnSync('/usr/bin/env', [nixPathAlias], {
+ encoding: 'utf-8',
+ })
+ // ??? what does this output look like ???
+ if (await pathExists(resolvedPath)) {
+ return resolvedPath
+ }
+ }
+
return null
}
@@ -315,7 +325,7 @@ export async function getAvailableEditors(): Promise<
const results: Array<IFoundEditor<string>> = []
for (const editor of editors) {
- const path = await getAvailablePath(editor.paths)
+ const path = await getAvailablePath(editor.paths, editor.nixPathAlias)
if (path) {
results.push({ editor: editor.name, path })
} |
|
seems fair enough. i'll close PR, not the issue. :D |
Fixes #1114
Fixes NixOS/nixpkgs#334253
Description
/usr/bin/env <binaryname>pathsScreenshots
Release notes
Notes: no-notes