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

linux.ts: added new paths to executables #1115

Closed
wants to merge 1 commit into from

Conversation

s0me1newithhand7s
Copy link

@s0me1newithhand7s s0me1newithhand7s commented Aug 19, 2024

Fixes #1114
Fixes NixOS/nixpkgs#334253

Description

  • added /usr/bin/env <binaryname> paths
  • edited to match global style

Screenshots

Release notes

Notes: no-notes

@s0me1newithhand7s
Copy link
Author

@shiftkey can you please review it?

paths: [
'/snap/bin/atom',
'/usr/bin/atom',
'/usr/bin/env atom'
Copy link
Owner

Choose a reason for hiding this comment

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

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...

Copy link
Author

Choose a reason for hiding this comment

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

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.

Copy link
Owner

Choose a reason for hiding this comment

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

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...

Copy link
Author

Choose a reason for hiding this comment

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

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.

Copy link
Owner

Choose a reason for hiding this comment

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

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...

Copy link
Author

Choose a reason for hiding this comment

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

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. :)

Copy link
Owner

@shiftkey shiftkey left a comment

Choose a reason for hiding this comment

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

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:

async function getAvailablePath(paths: string[]): Promise<string | null> {
for (const path of paths) {
if (await pathExists(path)) {
return path
}
}
return null
}

And the pathExists method here is a wrapper to handle Flatpak-specific issues with paths:

export async function pathExists(path: string): Promise<boolean> {
if (isFlatpakBuild()) {
path = convertToFlatpakPath(path)
}
try {
return await pathExistsInternal(path)
} catch {
return false
}
}

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.

@shiftkey
Copy link
Owner

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 /usr/bin/env [value] to see if it resolves to a path that exists on disk:

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 })
     }

@s0me1newithhand7s
Copy link
Author

seems fair enough. i'll close PR, not the issue. :D
thank you so much!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add Nixified (Nix) paths github-desktop: cannot find vscode installation
2 participants