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

[kazimuth]

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]

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

[rntz]

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

[pokey]

@rntz could you elaborate?

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

Cc/ @lunixbochs any ideas?

[rntz]

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]

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]

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]

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

[rntz]

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]

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]

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

[pokey]

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]

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

• On *nix, try $XDG_RUNTIME_DIR and /tmp. Those are both typically ramdisks on Linux.
• On Windows, only try TEMP?

[pokey]

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

[auscompgeek]

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]

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]

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

[pokey]

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]

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]

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

[pokey]

• We've seen cases where the environment variable differed between vscode process and Talon process
• For Linux flatpaks, you need to play some games to get it to be able to access the tmp dir (see eg https://talonvoice.slack.com/archives/C026KPTJE6T/p1682623939079009)
• We've seen other issues as well, but I don't remember the details cc/ @auscompgeek

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]

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]

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]

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]

Hi, I opened #1362 and cursorless-dev/command-server#21 to get the ball rolling to address this.

[DonnerWolfBach]

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]

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]

not necessarily. see discussion in the PR

[nriley]

On the community backlog session we decided that keeping the current behavior of using the OS native API to retrieve the temporary directory and permitting override via environment variable is likely to cause the fewest issues. If there are any inconsistencies between the behavior of python and node then these should be filed as separate issues and fixed.

[fidgetingbits]

On the community backlog session we decided that keeping the current behavior of using the OS native API to retrieve the temporary directory and permitting override via environment variable is likely to cause the fewest issues. If there are any inconsistencies between the behavior of python and node then these should be filed as separate issues and fixed.

Can you elaborate on how this fixes the problem? The suggestion is to manually set TMPDIR or?

[AndreasArvidsson]

On the community backlog session we decided that keeping the current behavior of using the OS native API to retrieve the temporary directory and permitting override via environment variable is likely to cause the fewest issues. If there are any inconsistencies between the behavior of python and node then these should be filed as separate issues and fixed.

Can you elaborate on how this fixes the problem? The suggestion is to manually set TMPDIR or?

Our suggestion is that server side should check the same environment variables and in the same order.

[fidgetingbits]

But one of the problems is TMPDIR sometimes is not the same value between the vscode process and talon process, so even if checked in the same order they may not have the same location and still won't work. Maybe I'm missing something still

[AndreasArvidsson]

That could be true of the home directory or any setting or variable. The user needs to make sure that this doesn't happen. Is this really a common occurrence in practice though?
The last resort would be to introduce a vscode setting as an override.

[fidgetingbits]

I don't think it would ever happen for HOME, as changing it doesn't really make sense, but of course I can't say with 100% certainty until I were to run into it. Changing temporary directory env vars I think is at least a reasonable possibility, and unlike others we know for sure it definitely happens in practice (as noted in various places in this issue/PR by different users).

As I mentioned in the PR thread it's not necessarily at the users discretion if TMPDIR changes, so disagree making sure it doesn't happen is really sufficient resolution. I have 3 systems all of which I use talon/cursorless on that all won't work without this fix because TMPDIR differs, and as noted in the PR I am not changing TMPDIR myself, it's because of extensions running in vscode, which are outside of my control.

Of course one could just say don't use the extensions, but I use them in order to load nix-based development environments for both personal and work projects, so not really an option for me. Also not always clear to users what these development tools/extensions are tweaking under the hood and likely infeasible for some users to investigate, so worth accommodating known areas where the problem exist imo.

I do agree if someone was just changing TMPDIR for no reason on their own it shouldn't be a use case we should worry about/accommodate, but this is different.

[AndreasArvidsson]

In that case a vscode setting sounds much simpler. I think it's absurd that vscode extension can change environment variables for other extensions though. That is definitely a red flag.

The ability to move temp to a ram disk or similar is quite nice so we should probably still respect env by default.

[fidgetingbits]

Okay. So the vscode setting would be in command-server and would just hardcode the rpc base path that you want, then also hardcode the same in another setting in talon client side? Seems reasonable to me

[AndreasArvidsson]

Something like that. We probably don't need the corresponding setting Talon side if this is just a problem where one vscode extensions incorrectly sets an environment variable.

So the change on the command server would be to use this order:

1. Explicit users set vscode setting
2. TMPDIR env var
3. os.tmpDir()

[AndreasArvidsson]

I have just confirmed the that os.tmpdir() checks TEMP and then TMP, but not TMPDIR.

[fidgetingbits]

Something like that. We probably don't need the corresponding setting Talon side if this is just a problem where one vscode extensions incorrectly sets an environment variable.

So the change on the command server would be to use this order:

1. Explicit users set vscode setting

2. TMPDIR env var

3. os.tmpDir()

That implies you always know in advance what the talon side will use and set that on vscode side, but if talon side is using env variables that could change (even system wide or whatever), then you won't know no? Hardcoding on both sides seems less error prone to me, but maybe I'm missing something.

[AndreasArvidsson]

Something like that. We probably don't need the corresponding setting Talon side if this is just a problem where one vscode extensions incorrectly sets an environment variable.
So the change on the command server would be to use this order:

1. Explicit users set vscode setting

2. TMPDIR env var

3. os.tmpDir()

That implies you always know in advance what the talon side will use and set that on vscode side, but if talon side is using env variables that could change (even system wide or whatever), then you won't know no? Hardcoding on both sides seems less error prone to me, but maybe I'm missing something.

The client will give you an error message with the path it's trying to use so you can just mirror that in the extension.
I don't want to make things more complicated than they have to be so until we have an actual need for the setting client side I would prefer to not have it.

[phillco]

tldr:

• Make list of temporary directory env vars checked match what Python uses cursorless-dev/talon-rpc#4 unifies the TMPDIR environment variable usage. This library isn't actually used yet, but it will be integrated into the command server extension. There will be another review for that.
• Andreas is going to prep a change to do that and will also add setting for @fidgetingbits et all to be able to configure the directory explicitly if there are further conflicts. We don't want to change the default behavior.
• Command server: manually specify the list of temporary directory environment variable names #1692 to discuss pinning the list of environment variables on both sides for consistency/safety reasons

@fidgetingbits if I understand right you're not on Windows, so TMPDIR was always checked first anyway and adding it to the list won't actually fix anything for you specifically if this was causing problems (i.e. you're not dependent on the future setting being released at the same time, though it most likely will). Do I understand right that your concern is more about ensuring the logic here is robust in general, or did you fix this a different way? Want to make sure we're not missing anything.

Edit: ah you're saying $TMPDIR could just be different between the Python/Node processes entirely even if the checking behavior is the same. Yup, agreed a setting is the only way to fix that.

[fidgetingbits]

tldr:

* [Make list of temporary directory env vars checked match what Python uses cursorless-dev/talon-rpc#4](https://github.com/cursorless-dev/talon-rpc/pull/4) unifies the TMPDIR environment variable usage. This library isn't actually used yet, but it will be integrated into the command server extension. There will be another review for that.

* Andreas is going to prep a change to do that and will also add setting for [@fidgetingbits](https://github.com/fidgetingbits) et all to be able to configure the directory explicitly if there are further conflicts. We don't want to change the default behavior.

* [Command server: manually specify the list of temporary directory environment variable names #1692](https://github.com/talonhub/community/issues/1692) to discuss pinning the list of environment variables on both sides for consistency/safety reasons

@fidgetingbits if I understand right you're not on Windows, so TMPDIR was always checked first anyway and adding it to the list won't actually fix anything for you specifically if this was causing problems (i.e. you're not dependent on the future setting being released at the same time, though it most likely will). Do I understand right that your concern is more about ensuring the logic here is robust in general, or did you fix this a different way? Want to make sure we're not missing anything.

Edit: ah you're saying $TMPDIR could just be different between the Python/Node processes entirely even if the checking behavior is the same. Yup, agreed a setting is the only way to fix that.

Yep. I'm using nix on linux and macos and the temp folder env variables can't be relied on to be the same between processes, so need a static folder.

[phillco]

@fidgetingbits just so I fully understand, the setting (proposed in cursorless-dev/command-server#40) is only going to add this to the VS Code side, Andreas isn't proposing to change the command server client, so effectively your only ability is to set that setting to the temporary directory that Python is looking for. You can't use this to provide a fixed folder, or something else. Does that match your understanding?