talonhub / community

Voice command set for Talon, community-supported.
MIT License
645 stars 783 forks source link

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

Open kazimuth opened 2 years ago

kazimuth commented 2 years ago

A random piece of software on my machine (Windows 10) had set this the system environment variable TMPDIR, which caused the vscode integration to not accept commands. I believe this is because the function get_communication_dir_path() uses gettempdir(): https://github.com/knausj85/knausj_talon/blob/main/apps/vscode/command_client/command_client.py#L211-L224

which checks TMPDIR, TEMP, and TMP, in order, to choose a temporary directory.

On the extension side, node's os.tmpdir() is used to select a temporary directory: https://github.com/pokey/command-server/blob/main/src/paths.ts#L4-L12

It's not documented, but I believe this must not check the TMPDIR env variable, causing the two ends of the connection to choose different directories and miss each other.

(Not sure how to fix this. You could override the directory lookup, or just switch to using a socket lol.)

kazimuth commented 2 years ago

(Deleting the environment variable fixed the problem, FWIW.)

rntz commented 2 years ago

Probably we should fix this on the vscode extension side by wrapping node's function. @pokey what do you think?

pokey commented 2 years ago

@rntz could you elaborate?

Alternately, we could use a hidden dir in the user directory.

Cc/ @lunixbochs any ideas?

rntz commented 2 years ago

We should wrap node's os.tmpdir() to have the same behavior as Python's gettempdir(), is what I'm suggesting. That way, unless the environment variables differ, the VSCode plugin and talon will look in the same place.

pokey commented 2 years ago

That could work, tho fwiw we have seen cases where the environment variables were different. Eg there is some VSCode extension that sets TMPDIR for some reason

auscompgeek commented 2 years ago

there is some VSCode extension that sets TMPDIR for some reason

That was probably just me with the Nix Environment Selector extension. Nix unsets $TMPDIR on macOS if it's pointing to /var/folders/.... Thankfully I don't need that extension.

pokey commented 2 years ago

Ah right yeah forgot that was you. Is there a downside to using a hidden dir in home directory? It's fairly common

rntz commented 2 years ago

I mildly prefer using the temporary directory. I've put my temp dir on an in-memory filesystem so that all this command client traffic doesn't have to hit my hard drive at all.

pokey commented 2 years ago

I'm just a bit nervous about relying on tmp dir, as it seems like it's maybe not very stable between different execution environments. Might be better to use a specific path for reliability's sake

Maybe we could make the path configurable somehow, so that users can switch it to somewhere in tmp dir if they prefer that? Could allow them to specify arbitrary parent dir, or even just a flag that switches it to tmp dir. Would need to figure out how to enable configuring both client and server. Ideally they'd have the same source of truth. Maybe they could just both read some configuration file in home dir? Would want to make sure that config file is only readable / writeable by user

auscompgeek commented 2 years ago

Can we use the Talon home? Should be a predictable path locked down on all systems.

pokey commented 2 years ago

Can we rely on it never moving? From Talon side we should be fine but from server (ie vscode) side, we'd need to hardcode the locations

It's in user dir as well, so doesn't really help with @rntz's concerns

auscompgeek commented 2 years ago

Keeping things in ramdisks would be a good thing, yeah. How about this:

pokey commented 2 years ago

@auscompgeek would that fix the \1 thing from Slack?

auscompgeek commented 2 years ago

On further investigation in that Slack thread, unfortunately no - TEMP is somehow inconsistent between apps despite being on Windows 😰

On Windows ramdisks tend to not be a thing anyway, so maybe we can use the Talon home on Windows and my above suggestion elsewhere?

rntz commented 2 years ago

I would rather avoid platform-specific behavior if at all possible - unnecessary complexity. (Although I guess tempdirs are kind of platform-specific already...) Is there any problem with just making sure both sides look for temporary directories in the same order? i.e. either adjust things on the node end or on the Python end?

auscompgeek commented 2 years ago

FWIW there's already platform-specific behaviour here: on *nix the UID is in the directory name, but not on Windows.

pokey commented 1 year ago

I would be tempted to default to the home directory, but make it configurable. The user would need to make sure that they use the same setting on Talon and ide side, so slightly more involved / finicky in that case, but I think I'd rather have the common / beginner case just work at the expense of slightly more complication for power users. We've had lots of issues now with users struggling to get started due to tmp dir problems. WDYT @rntz @auscompgeek

rntz commented 1 year ago

I still don't understand what the issue is with making sure both sides look for temporary directories in the same order. Then we don't need any settings and things will just work for everyone.

rntz commented 1 year ago

Could you give more detail about what the issues you've seen are?

pokey commented 1 year ago

I guess we could use the home dir to negotiate a location, preferring that location to be somewhere in tmp dir, tho that wouldn't solve the flatpak access problem

auscompgeek commented 1 year ago

If we put anything in the home dir, we'd also have to put it outside of a dot directory, since we've also seen snaps prevent access to dotfiles.

rntz commented 1 year ago
  1. Have the tmpdir env vars differed in any case except for @auscompgeek with Nix?
  2. What is the \1 issue referred to previously?
  3. If flatpaks can't access dot dirs, can they access the talon home/user directories? If not, neither tmp nor talon home are usable solutions! (I guess the reason we care is because some users install VSCode as a flatpak?)
rntz commented 1 year ago

to be clear, if we need to put this somewhere other than a temp directory to make it work for more people out of the box, I think we should do that. I just want to (a) avoid overcomplicating things if we don't have to (b) make sure the alternative solution does work for more people out of the box. being able to use a ramdisk/tmpfs is a nice-to-have not a necessity.

fidgetingbits commented 10 months ago

Hi, I opened https://github.com/talonhub/community/pull/1362 and https://github.com/pokey/command-server/pull/21 to get the ball rolling to address this.

DonnerWolfBach commented 6 months ago

I had the same problem/symptoms, just that in my case it was XDG_RUNTIME_DIR that talon looked into but not vscode/the cursorless/command server plugin

AndreasArvidsson commented 6 months ago

If we put anything in the home dir, we'd also have to put it outside of a dot directory, since we've also seen snaps prevent access to dotfiles.

If this is the case that disqualifies the Talon .talon directory right?

fidgetingbits commented 6 months ago

not necessarily. see discussion in the PR