fix #966 by using fixed folders #1362

fidgetingbits · 2024-01-19T01:27:09Z

This is a fix for #966. I've got three different systems where I run into this issue, so want to finally address it for everyone.

There is a corresponding PR for command-server at pokey/command-server#21

I haven't actually tested this on windows, as I don't have it, so I'm not sure if the path I used is sufficient. On this side it is TALON_HOME, and on command-server side it is hardcoded which I think is the same but not positive. Also in the discussion there was a suggestion to make the folder hidden. I left a commented out cmd for setting the folder as hidden, but again not sure if it is right and can't test it, so feedback on if that's the right approach is welcome. Will be good if someone can test/verify the windows side (I added a few windows users as requested reviewers).

Its also worth noting that in #966 discussion, @auscompgeek suggested to use XDG_RUNTIME_DIR on Linux, which I had implemented and tested originally. However, when looking into snaps I ran across https://forum.snapcraft.io/t/rethinking-how-we-handle-xdg-runtime-dir/22223 which suggests that they may want to change snaps to no longer have access to the same XDG_RUNTIME_DIR folder. If they actually ever implemented that then this would break, so in the end I opted not to use it.

There's also per user temp directories on mac, which you can query using getconf DARWIN_USER_TEMP_DIR, but I decided not to keep that code in the end since it keeps the mac/linux case simpler both using /tmp, especially in light of not using XDG_RUNTIME_DIR in the end.

ATM I cache the result in a global, and this was mostly because originally I had code that was calling getconf DARWIN_USER_TEMP_DIR, but I chose to just leave the global afterwards anyway.

I've tested the changes using /tmp on both Linux and Mac and it works fine.

Test on Windows
Test on Mac
Test on Linux

lunixbochs · 2024-01-19T06:05:09Z

please don't merge until I do security review

pokey

Looks fine but I haven't tried it locally. See comments inline

apps/vscode/command_client/command_client.py

pokey · 2024-01-21T08:52:07Z

apps/vscode/command_client/command_client.py

+    home_dir = Path(os.path.expanduser("~"))
+    if app.platform == "linux" or app.platform == "mac":
+        return (
+            Path(home_dir) / f".talon/.comms/{actions.user.command_server_directory()}"


I thought accessing a hidden folder was also a problem for snaps, no? Cc/ @auscompgeek

This is true for most snaps by default iirc, but afaik vscode uses something called classic confinement, which has unrestricted access to files. This makes sense since it's an IDE and users would expected to be able to access anything as their user anyway. So I think we should be okay here (though I haven't tested with the snap yet).

apps/vscode/command_client/command_client.py

for more information, see https://pre-commit.ci

pokey · 2024-01-23T14:01:22Z

Once everything is reviewed and ready to merge, we should do one final test on each platform, given the potential repercussions of this code breaking 😅

apps/vscode/command_client/command_client.py

nriley · 2024-02-03T16:55:30Z

apps/vscode/command_client/command_client.py

+        return home_dir / ".talon" / " .comms" / directory_name
+    elif app.platform == "windows":
+        # subprocess.run(["attrib","+H","myfile.txt"],check=True)
+        return home_dir / "AppData" / "Roaming" / "talon" / ".comms" / directory_name


This also makes little sense on Windows where the dot prefix doesn't "do" anything.

Originally I didn't have it prefixed I think, but it was switched for consistency of paths across all platforms.

I don't think it matters much. But yeah the talon user path has no prefixed dot either so there is no consistency at that level anyway. So in this case i think actually using "comms" without dot on Windows is probably better? Happy either way.

@nriley Doesn't the dot prefix still hide a directory on windows in certain views?
Personally I would slightly prefer .comms everywhere for consistency if you compare the talon settings folder on different systems - even though of course the name and path of the talon settings folder will be widely different.

Doesn't the dot prefix still hide a directory on windows in certain views?

Nope, the hidden bit is separate metadata to the filename on Windows.

I would personally vote for the consistency on this bit fwiw, as it is harmless.

nriley · 2024-02-03T16:56:16Z

apps/vscode/command_client/command_client.py

+    if app.platform == "linux" or app.platform == "mac":
+        return home_dir / ".talon" / " .comms" / directory_name
+    elif app.platform == "windows":
+        # subprocess.run(["attrib","+H","myfile.txt"],check=True)


This seems like it may have been some testing code?

I left this in for someone with access to Windows in case they want to test using a hidden folder, but didn't uncomment it because I can't test if it works.

apps/vscode/command_client/command_client.py

fidgetingbits · 2024-03-21T05:33:09Z

Aside from the Path.home() bit which will be quick to fix, this is still mostly waiting on someone who uses Windows to test that it actually works as intended. Anyone have time to try?

DonnerWolfBach · 2024-05-08T17:32:05Z

@fidgetingbits I will hopefully have time (about 2-6h) to try it on windows on Saturday and I can find more time after that.
I assume I would just have to apply this PR to my personal talon community fork (and remove the dirty fixes I Currently Have in Place).
However, how would I go about applying pokey/command-server#21 ?

fidgetingbits · 2024-05-09T00:09:12Z

@fidgetingbits I will hopefully have time (about 2-6h) to try it on windows on Saturday and I can find more time after that. I assume I would just have to apply this PR to my personal talon community fork (and remove the dirty fixes I Currently Have in Place). However, how would I go about applying pokey/command-server#21 ?

Hey @DonnerWolfBach thanks! Reminds me @saidelike recently said they could also test this on Windows.

Easiest is to merge the PR into some temporary branch you want to build from:

git clone https://github.com/pokey/command-server
cd command-server
git switch -c dev-build
git fetch origin pull/21/head:static-tmp-dir
git merge static-tmp-dir -m "Merge branch 'use-static-tmp-dir' into dev"

Then you need to build and install. I do it on nix on both macOS and linux, so merge yet another PR so I can just run nix build. I do it like this:

git fetch origin pull/22/head:nix-flake
git merge nix-flake -m "Merge branch 'nix-flake' into dev"
nix build
code --force --install-extension result/*.vsix

If you have a nix box and can do the above, I think the .vsix file will actually work on windows too.

But, if you have to build the extension on windows, presumably you need to run the same yarn commands as what the flake PR does. But note that I also had to update package.json to add vsce to yarn dependencies so it didn't have to be installed separately (not sure off the top of my head where it comes from otherwise). So if you don't want to find where vsce comes from, you may want to do that too, and then you just need to install yarn on windows (however one does that).

Then the manual build will be something like this:

yarn run compile
yarn vsce package --yarn -o ./command-server.vsix
code --force --install-extension ./command-server.vsix

@pokey might be worth adding a small note to the command-server README about how to "officially" build it. I'll file an issue.

auscompgeek · 2024-05-09T11:26:11Z

apps/vscode/command_client/command_client.py

+    home_dir = Path(os.path.expanduser("~"))
+    directory_name = actions.user.command_server_directory()
+    if app.platform == "linux" or app.platform == "mac":
+        return home_dir / ".talon" / " .comms" / directory_name


Don't snaps prevent access to ~/.*?

In the default containment profiles yes (I believe you need to request personal confinement to access dotfiles), but from what I read vscode should be okay due to it using classic confinement (which is effectively no confinement). See here from this same PR where I said similar: #1362 (comment)

@DonnerWolfBach said he was using VSCodium. I suspect since it's just an open source build of VSCode (?) and an IDE when packaged as a snap it would use the same classic confinement. Maybe if he's using the snap, he could confirm though.

From what I've seen they tend to grant classic confinement to all IDEs under the assumption they will be used to edit any files. For instance neovim snap also has classic confinement.

I think the only other option if we want to assume we need to communicate with a snaps with restricted confinement is /tmp, and the discussion on #966 people couldn't agree, and it seemed some (pokey and rntz at least iirc) were leaning towards using $HOME over /tmp, so I just used $HOME for the sake of getting anything going.

I don't use VSCodium in a snap unfortunately. I use the nix package

DonnerWolfBach · 2024-05-11T09:00:34Z

git merge origin/static-tmp-dir -m "Merge branch 'use-static-tmp-dir' into dev"

The origin in there is a typo isn't it? With it it doesn't work, but without it does, at least on my machine that branch was created locally from your PR

edit: @fidgetingbits (sorry, I am unfamiliar with how github commenting works, not sure whether this is the right way to point out such a small thing)^^'

fidgetingbits · 2024-05-11T09:06:09Z

git merge origin/static-tmp-dir -m "Merge branch 'use-static-tmp-dir' into dev"
The origin in there is a typo isn't it? With it it doesn't work, but without it does, at least on my machine that branch was created locally from your PR

edit: @fidgetingbits (sorry, I am unfamiliar with how github commenting works, not sure whether this is the right way to point out such a small thing)^^'

I modified the commands from a script that uses my local branches rather than the PRs, so likely why. I'll edit the comment in case anyone else tries to use it.

DonnerWolfBach · 2024-05-11T10:48:53Z

yarn vsce package --yarn -o ./command-server.vsix

Here I am running into troubles once again:

PS C:\Users\Sebastian\Repos\command-server> corepack yarn vsce package --yarn -o ./command-server.vsix
Executing prepublish script 'yarn run vscode:prepublish'...
 ERROR  Command failed: yarn list --prod --json

PS C:\Users\Sebastian\Repos\command-server> corepack yarn list --prod --json

Usage Error: Couldn't find a script named "list".

$ yarn run [--inspect] [--inspect-brk] [-T,--top-level] [-B,--binaries-only] [--require #0] <scriptName> ...
PS C:\Users\Sebastian\Repos\command-server>

Due to lacking knowledge of yarn etc. I don't think I am able to troubleshoot that myself farther than this.

Version info:

PS C:\Users\Sebastian\Repos\command-server> corepack yarn vsce --version
2.15.0
PS C:\Users\Sebastian\Repos\command-server> corepack yarn --version
4.2.2
PS C:\Users\Sebastian\Repos\command-server> node --version
v20.13.1

(cartago delenda est - I mean I think pokey/command-server#26 really makes sense)

fidgetingbits · 2024-05-11T11:10:22Z

Any chance the instructions saidelike posted on that other issue would help building on windows? He implies 'pnpm compile' is enough. I don't even remember why I used yarn anymore..

saidelike · 2024-05-12T05:57:11Z

Any chance the instructions saidelike posted on that other issue would help building on windows? He implies 'pnpm compile' is enough. I don't even remember why I used yarn anymore..

Fwiw i used "pnpm" for neovim's command-server because cursorless used "pnpm" and i didn't realise command-server was using "yarn" by default (as indicated in the package.json). Maybe @pokey can indicate how he builds the vscode's command-server?

Btw, I am happy to test these command-client's changes along with corresponding changes in my neovim command-server extension.

DonnerWolfBach · 2024-06-15T08:30:59Z

@fidgetingbits managed to test it on windows 10 with the newest version of both branches. 2024-06-15 10:28:07.578 IO communication_dir_path=WindowsPath('C:/Users/Sebastian/AppData/Roaming/talon/.comms/vscode-command-server') and indeed cursorless works

@saidelike thanks for the hint, changing yarn to pnpm in the package.json solved the problem

Interestingly it also worked if I just changed the command server and not the talon user files. there was still a command server in the temporary directory and it was working...

knausj85 · 2024-11-03T14:11:16Z

I'm going through some of the older PRs. This looks like it's ready to be merged to me? Something I'm missing?

fidgetingbits · 2024-11-08T02:39:40Z

I'm going through some of the older PRs. This looks like it's ready to be merged to me? Something I'm missing?

I think so, with the caveat I think the @pokey command server vscode extension will have to merge the PR and release an updated version first. Otherwise it will break RPC for users.

There was the snap concern about home access, which I've noted I don't think is valid for editors.

There is also the commented code for setting a hidden file attribute for windows, which I don't know if windows people want and couldn't test, but happy to just have that deleted before merge.

fidgetingbits · 2024-11-08T03:25:27Z

Also @lunixbochs had commented they wanted to do a security review before this gets merged, so will need to take a look.

fix talonhub#996 by using fixed folders

8f41821

fidgetingbits requested review from rntz, pokey, AndreasArvidsson and knausj85 January 19, 2024 01:27

fidgetingbits mentioned this pull request Jan 19, 2024

If TMPDIR environment variable is set, vscode integration does not accept commands #966

Open

lunixbochs self-requested a review January 19, 2024 06:04

pokey reviewed Jan 19, 2024

View reviewed changes

apps/vscode/command_client/command_client.py Outdated Show resolved Hide resolved

apps/vscode/command_client/command_client.py Outdated Show resolved Hide resolved

pokey reviewed Jan 19, 2024

View reviewed changes

apps/vscode/command_client/command_client.py Outdated Show resolved Hide resolved

fidgetingbits added 2 commits January 20, 2024 19:00

move suffix inline for mac/linux

8386fe3

move to home-based folder and remove suffix

aae074c

pokey reviewed Jan 21, 2024

View reviewed changes

fidgetingbits and others added 3 commits January 22, 2024 09:01

fix windows path and keep old function for easier testing

442e404

[pre-commit.ci] auto fixes from pre-commit.com hooks

040f5d0

for more information, see https://pre-commit.ci

cleanup to match VSCode side

356a4f7

remove legacy code

5b224a9

nriley changed the title ~~fix #996 by using fixed folders~~ fix #966 by using fixed folders Feb 3, 2024

nriley reviewed Feb 3, 2024

View reviewed changes

apps/vscode/command_client/command_client.py Show resolved Hide resolved

nriley requested changes Feb 3, 2024

View reviewed changes

fidgetingbits mentioned this pull request May 9, 2024

Document how someone is supposed to build command-server pokey/command-server#26

Open

auscompgeek reviewed May 9, 2024

View reviewed changes

switch os.path.expanduser to Path.home()

489a20e

Merge branch 'main' into fix-996

f9baf0e

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix #966 by using fixed folders #1362

fix #966 by using fixed folders #1362

fidgetingbits commented Jan 19, 2024 •

edited by pokey

Loading

lunixbochs commented Jan 19, 2024

pokey left a comment

pokey Jan 21, 2024

fidgetingbits Jan 21, 2024

pokey commented Jan 23, 2024

nriley Feb 3, 2024

fidgetingbits Feb 4, 2024

saidelike May 12, 2024

DonnerWolfBach Jun 15, 2024

auscompgeek Jun 18, 2024

knausj85 Nov 3, 2024

nriley Feb 3, 2024

fidgetingbits Feb 4, 2024

fidgetingbits commented Mar 21, 2024

DonnerWolfBach commented May 8, 2024

fidgetingbits commented May 9, 2024 •

edited

Loading

auscompgeek May 9, 2024

fidgetingbits May 9, 2024

DonnerWolfBach Jun 15, 2024

DonnerWolfBach commented May 11, 2024 •

edited

Loading

fidgetingbits commented May 11, 2024

DonnerWolfBach commented May 11, 2024

fidgetingbits commented May 11, 2024

saidelike commented May 12, 2024

DonnerWolfBach commented Jun 15, 2024 •

edited

Loading

knausj85 commented Nov 3, 2024

fidgetingbits commented Nov 8, 2024

fidgetingbits commented Nov 8, 2024

fix #966 by using fixed folders #1362

Are you sure you want to change the base?

fix #966 by using fixed folders #1362

Conversation

fidgetingbits commented Jan 19, 2024 • edited by pokey Loading

lunixbochs commented Jan 19, 2024

pokey left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pokey commented Jan 23, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

fidgetingbits commented Mar 21, 2024

DonnerWolfBach commented May 8, 2024

fidgetingbits commented May 9, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

DonnerWolfBach commented May 11, 2024 • edited Loading

fidgetingbits commented May 11, 2024

DonnerWolfBach commented May 11, 2024

fidgetingbits commented May 11, 2024

saidelike commented May 12, 2024

DonnerWolfBach commented Jun 15, 2024 • edited Loading

knausj85 commented Nov 3, 2024

fidgetingbits commented Nov 8, 2024

fidgetingbits commented Nov 8, 2024

fidgetingbits commented Jan 19, 2024 •

edited by pokey

Loading

fidgetingbits commented May 9, 2024 •

edited

Loading

DonnerWolfBach commented May 11, 2024 •

edited

Loading

DonnerWolfBach commented Jun 15, 2024 •

edited

Loading