Skip to content

Conversation

@lgarron
Copy link

@lgarron lgarron commented Oct 20, 2022

If it's not available, fall back to reading and/or creating ~/.[product-name]/argv.json.

This addresses #162712

Unfortunately, there is an issue in that
AbstractNativeEnvironmentService.argvResource() does not have access
to the filesystem directly. It will need further work to fall back from
one location to another.

@lgarron lgarron changed the title Look for argv.json at ~/.config/[product-name]/argv.json. Look for argv.json at ~/.config/[product-name]/argv.json (XDG convention) Oct 20, 2022
@lgarron
Copy link
Author

lgarron commented Oct 20, 2022

@lgarron please read the following Contributor License Agreement(CLA). If you agree with the CLA, please reply with the following information.

I work for GitHub, therefore I'm also a Microsoft employee.

@microsoft-github-policy-service agree [company="Microsoft"]

@lgarron
Copy link
Author

lgarron commented Oct 20, 2022

@microsoft-github-policy-service agree company="Microsoft"

…vention).

If it's not available, fall back to reading and/or creating `~/.[product-name]/argv.json`.

See `$XDG_CONFIG_HOME` at https://specifications.freedesktop.org/basedir-spec/basedir-spec-latest.html
This will allow VSCode to avoid creating new dirs in the home folder, matching a large number of other programs: https://wiki.archlinux.org/index.php/XDG_Base_Directory_support

This addresses microsoft#162712

Unfortunately, there is an issue in that
`AbstractNativeEnvironmentService.argvResource()` does not have access
to the filesystem directly. It will need further work to fall back from
one location to another.
Comment on lines +109 to +110
// TODO: how do we fall back to this?
// return joinPath(this.userHome, this.productService.dataFolderName, 'argv.json');
Copy link
Author

Choose a reason for hiding this comment

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

This file seems to use an abstract environment that does not know about the underlying file system (or similar abstraction). I could use guidance on the best way to implement a fallback.

@lgarron lgarron marked this pull request as ready for review October 26, 2022 03:05
@lgarron
Copy link
Author

lgarron commented Oct 31, 2022

poke :-D

@bpasero bpasero added this to the Backlog milestone Nov 7, 2022
@bpasero
Copy link
Member

bpasero commented Nov 8, 2022

Thanks for the PR, I cannot make promises when I get a chance for review though.

@bpasero bpasero changed the title Look for argv.json at ~/.config/[product-name]/argv.json (XDG convention) Linux: Support XDG_CONFIG_HOME for argv.json location Dec 7, 2022
Copy link
Member

@bpasero bpasero left a comment

Choose a reason for hiding this comment

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

This change seems to attempt to do two things that are mutually exclusive:

  • allow to set the user data dir via argv.json
  • support XDG for the location of argv.json

I do not think both changes should be in one PR so I suggest to focus on the XDG support here. The other change for supporting user data dir in argv.json does not work anyway in this form.

As for the location of argv.json I think the solution we use here should be copied/applied, not something new invented:

appDataPath = process.env['XDG_CONFIG_HOME'] || path.join(os.homedir(), '.config');

@lgarron
Copy link
Author

lgarron commented Dec 7, 2022

Thanks for the review, @bpasero!

This change seems to attempt to do two things that are mutually exclusive:

  • allow to set the user data dir via argv.json
  • support XDG for the location of argv.json

I do not think both changes should be in one PR so I suggest to focus on the XDG support here.

That sounds reasonable to me. What about allowing the part about user extensions to be set via argv.json — I assume you meant to cover that in the first bullet?
I'm happy to submit it in a separate PR, but both changes are necessary to avoid the creation of a ~/.vscode/ folder, which is my only real goal.

As for the location of argv.json I think the solution we use here should be copied/applied, not something new invented:

appDataPath = process.env['XDG_CONFIG_HOME'] || path.join(os.homedir(), '.config');

Could I ask you to clarify how that would work?

Do you mean to suggest that:

  • appDataPath should be set to match Linux?
  • VSCode should search for argv.json in appDataPath?
  • Similar logic should be copied into getArgvConfigPath()?

In particular, how do you envision that users' existing argv.json files would be treated?

Also, unfortunately the use of the XDG_CONFIG_HOME env var for macOS configuration code is unreliable in practice, because macOS does not have a way to consistently launch a given app with certain env vars (that I know of):

  • You can inherit env vars when launched from the shell, but this doesn't work when VSCode launched because you clicked it on the dock, or opened a file/folder.
  • You can use launchctl to set an OS-wide env var, but this requires quite some technical expertise... and it also doesn't work reliably. If VSCode launches on login (e.g. due to the built-in macOS functionality to resume apps after boot), then it can (and does) easily launch before a launchctl daemon can set the env var.

@bpasero
Copy link
Member

bpasero commented Dec 7, 2022

My only suggestion here was to use XDG_CONFIG_HOME and then fallback to os.homedir() for argv.json. And maybe we find more locations where we wrongly just use os.homedir() when we should also check for XDG_CONFIG_HOME?

I wonder why node.js is not supporting XDG_CONFIG_HOME from their os.homedir() implementation, wouldn't that be reasonable to do?

@lgarron
Copy link
Author

lgarron commented Dec 7, 2022

  • VSCode should search for argv.json in appDataPath?

FWIW, I think this is a fairly idiomatic place for argv.json. I'd be happy to update the PR to that, although I'd need guidance on:

  • Which is used if both ~/Library/Application Support/argv.json and ~/.vscode/argv.json exist? The former, the latter, or maybe somehow combine settings from both?
  • Should an existing ~/.vscode/argv.json be moved to ~/Library/Application Support/argv.json on startup?

@lgarron
Copy link
Author

lgarron commented Dec 7, 2022

My only suggestion here was to use XDG_CONFIG_HOME and then fallback to os.homedir() for argv.json. And maybe we find more locations where we wrongly just use os.homedir() when we should also check for XDG_CONFIG_HOME?

Hmm, yeah, that doesn't help me at all, unfortunately. 😔

I wonder why node.js is not supporting XDG_CONFIG_HOME from their os.homedir() implementation, wouldn't that be reasonable to do?

Probably. However, I wouldn't look to node or npm for good practices in this regard, as they are some of the very few programs on my system that also don't use/respect the XDG dir conventions.

@bpasero
Copy link
Member

bpasero commented Dec 7, 2022

Maybe then I misunderstood the original request, why does this not help you?

@lgarron
Copy link
Author

lgarron commented Dec 7, 2022

Maybe then I misunderstood the original request, why does this not help you?

I want to avoid a ~/.vscode/ folder in my home directory.

Most programs now create their dot folders in ~/.config/ (or similar dirs) instead of directly in the home dir, or automatically/manually can be adapted to follow that convention:

As documented in #162712 , this is also a regular wish by other VSCode users.

Since VSCode is a macOS application, there is no reliable way to work around this using env vars, which is why I proposed this PR with its original title.

@bpasero
Copy link
Member

bpasero commented Dec 7, 2022

Sorry but I only own the part about argv.json not the extensions location. So I suggest to keep things separate.

@lgarron
Copy link
Author

lgarron commented Dec 7, 2022

Sorry but I only own the part about argv.json not the extensions location. So I suggest to keep things separate.

Sounds reasonable. If we can address the argv.json location here, I'm happy to open a separate PR.

@bpasero bpasero added the info-needed Issue requires more information from poster label Dec 8, 2022
@bpasero
Copy link
Member

bpasero commented Dec 9, 2022

Sounds good.

@vscodenpa
Copy link

This issue has been closed automatically because it needs more information and has not had recent activity. See also our issue reporting guidelines.

Happy Coding!

@vscodenpa vscodenpa closed this Dec 17, 2022
@github-actions github-actions bot locked and limited conversation to collaborators Jan 31, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

info-needed Issue requires more information from poster

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants