-
Notifications
You must be signed in to change notification settings - Fork 2.4k
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
fix: use HOME dir if set #2177
base: master
Are you sure you want to change the base?
fix: use HOME dir if set #2177
Conversation
HOME directory should be preferred source of home directory information. It's a common pattern to run software with a custom HOME to temporarily isolate it from the system-set home directory. All Unix software I ever worked with would respect HOME. Preferring `user.Current()` (which I guess reads from `/etc/passwd`) make it impossible to temporarily change effective home, as changing `/etc/passwd` is not an option. I'm currently migrating things to a self-hosted github-actions runner, where one system user is used for all github actions runners, but each instance gets a different `HOME`, and is otherwise sandboxed to not even have permissions to touch the original home. Everything works except `lncli` which fails with: ``` [lncli] could not load global options: could not read profile file /home/github-runner/.lncli/profiles.json: could not load profile file /home/github-runner/.lncli/profiles.json: open /home/github-runner/.lncli/profiles.json: permission denied ``` and I traced it to this code.
Pull Request Test Coverage Report for Build 8811320930Warning: This coverage report may be inaccurate.This pull request's base commit is no longer the HEAD commit of its target branch. This means it includes changes from outside the original pull request, including, potentially, unrelated coverage changes.
Details
💛 - Coveralls |
Thanks for the PR. But I'm very hesitant to accept a change like this, since it seems it would change the default behavior of all our projects that rely on this code. I'd say it's very unusual for |
Yeah. This is a valid concern.
That's subjective I guess, but I don't find it uncommon, and use that functionality a lot. It's especially common when dealing with systemd units, long running daemons, etc. Thought I guess the fact that I'm the first to report it, might suggest that you're right. But by the same coin - not a lot of people will be affected by switching it. As far as I know Rust standard library: https://doc.rust-lang.org/std/env/fn.home_dir.html Python:
Node.js:
Ruby:
Julia:
Haskell:
|
Hmm, interesting. Thanks for the additional info and the test cases. Maybe we should instead do what the shell does and check if we can actually access the directory returned by |
I understand the worry about not breaking lots of downstream projects, but I don't think making things more complicated is going to be helpful. How about phasing it in, so:
The end goal is using After some time old behavior and |
Sounds good to me. |
$HOME
should be preferred source of home directory information.It's a common pattern to run software with a custom HOME to temporarily isolate it from the system-set home directory. All Unix software I ever worked with would respect HOME.
Preferring
user.Current()
(which I guess reads from/etc/passwd
) make it impossible to temporarily change effective home, as changing/etc/passwd
is not an option.I'm currently migrating things to a self-hosted github-actions runner, where one system user is used for all github actions runners, but each instance gets a different
HOME
, and is otherwise sandboxed to not even have permissions to touch the original home. Everything works exceptlncli
which fails with:and I traced it to this code.