-
Notifications
You must be signed in to change notification settings - Fork 528
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
introduce abstractions to make Flatpak integration easier #555
Conversation
@@ -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. |
There was a problem hiding this comment.
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
} from 'child_process' | ||
|
||
export function isFlatpakBuild() { | ||
return __LINUX__ && process.env.FLATPAK_HOST === '1' |
There was a problem hiding this comment.
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.
*/ | ||
export async function pathExists(path: string): Promise<boolean> { | ||
if (isFlatpakBuild()) { | ||
path = convertToFlatpakPath(path) |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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
?
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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.
options?: SpawnOptionsWithoutStdio | ||
): ChildProcess { | ||
if (isFlatpakBuild()) { | ||
return spawn('flatpak-spawn', ['--host', path, ...args], options) |
There was a problem hiding this comment.
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.
* | ||
*/ | ||
function convertToFlatpakPath(path: string) { | ||
return join('/var/run/host', path) |
There was a problem hiding this comment.
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...
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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!
unfortunately this is not able to detect binaries properly, also please add |
Konsole is detected but vscodium isn't due to the issue mentioned here #555 (comment), you have to patch the launch code here desktop/app/src/lib/shells/shared.ts Line 84 in 2846fd2
desktop/app/src/lib/editors/linux.ts Line 37 in 2846fd2
/usr/share/vscodium-bin/bin/codium so vscodium detection works as intended
|
#556 fixes both issued mentioned above |
Co-Authored-By: nullrequest <[email protected]>
Co-Authored-By: nullrequest <[email protected]>
722307d
to
3bbef90
Compare
Editor detection is still broken, shell's are detected and launched with zero issues |
This comment has been minimized.
This comment has been minimized.
Co-Authored-By: nullrequest <[email protected]>
Co-Authored-By: nullrequest <[email protected]>
Co-Authored-By: nullrequest <[email protected]>
Co-Authored-By: nullrequest <[email protected]>
Co-Authored-By: nullrequest <[email protected]>
Co-Authored-By: nullrequest <[email protected]>
Co-Authored-By: nullrequest <[email protected]>
…ration easier (#555 #558) Co-Authored-By: nullrequest <[email protected]>
…ration easier (#555 #558) Co-Authored-By: nullrequest <[email protected]>
…ration easier (#555 #558) Co-Authored-By: nullrequest <[email protected]>
…ration easier (#555 #558) Co-Authored-By: nullrequest <[email protected]>
…ration easier (#555 #558) Co-Authored-By: nullrequest <[email protected]>
…ration easier (#555 #558) Co-Authored-By: nullrequest <[email protected]>
…ration easier (#555 #558) Co-Authored-By: nullrequest <[email protected]>
…ration easier (#555 #558) Co-Authored-By: nullrequest <[email protected]>
…ration easier (#555 #558) Co-Authored-By: nullrequest <[email protected]>
…ration easier (#555 #558) Co-Authored-By: nullrequest <[email protected]>
…ration easier (#555 #558) Co-Authored-By: nullrequest <[email protected]>
…ration easier (#555 #558) Co-Authored-By: nullrequest <[email protected]>
…ration easier (#555 #558) Co-Authored-By: nullrequest <[email protected]>
…ration easier (#555 #558) Co-Authored-By: nullrequest <[email protected]>
…ration easier (#555 #558) Co-Authored-By: nullrequest <[email protected]>
…ration easier (#555 #558) Co-Authored-By: nullrequest <[email protected]>
…ration easier (#555 #558) Co-Authored-By: nullrequest <[email protected]>
…ration easier (#555 #558) Co-Authored-By: nullrequest <[email protected]>
…ration easier (#555 #558) Co-Authored-By: nullrequest <[email protected]>
…ration easier (#555 #558) Co-Authored-By: nullrequest <[email protected]>
…ration easier (#555 #558) Co-Authored-By: nullrequest <[email protected]>
…ration easier (#555 #558) Co-Authored-By: nullrequest <[email protected]>
…ration easier (#555 #558) Co-Authored-By: nullrequest <[email protected]>
…ration easier (#555 #558) Co-Authored-By: nullrequest <[email protected]>
…ration easier (#555 #558) Co-Authored-By: nullrequest <[email protected]>
…ration easier (#555 #558) Co-Authored-By: nullrequest <[email protected]>
…ration easier (#555 #558) Co-Authored-By: nullrequest <[email protected]>
Related to #554 but enables us to better combine regular usage as well as integration with the Flatpak host
I've moved the
pathExists
andspawn
usage into a standalone module, so we can keep this platform-specific code isolated from the rest of this app. Not sure how feasible this is, but I hope it makes things easier to reason about if they're funnelling through the same functions (which we can adapt based on the requirements).cc @AdvaithM for thoughts
I'll add some notes in the PR to help guide the discussion about these changes
TODO