-
Notifications
You must be signed in to change notification settings - Fork 1k
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
Allow state directory based in $HOME
.
#1236
base: main
Are you sure you want to change the base?
Conversation
The reasoning for this is already outlined well in facebook#1092. This would be useful for us in Homebrew, and would probably limit the number of bug reports you get from Homebrew users that get tripped up by our hard-coded `WATCHMAN_STATE_DIR`. (See, e.g., facebook#963.) It should also reduce the number of users who end up with `brew` building Watchman from source because they're using a non-default prefix, and hopefully issues from them too (e.g. facebook#1132). Closes facebook#1092.
@facebook-github-bot has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator. |
We already look up the user's `HOME` directory in `main.cpp` using `getpwuid`. Let's use that as the fallback for an unset `HOME` environment variable (no matter how unlikely) and deduplicate that code with the same one we've added to `UserDir.cpp`.
@carlocab has updated the pull request. You must reimport the pull request before landing. |
@facebook-github-bot has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator. |
CC @andrewhamon for thoughts, since you might be interested. |
hi @carlocab - this looks like a nice change to have, thanks for the PR! Would you mind sharing some manual test output/examples to verify the behavior and that it works? Also, instead of gating this at build time in the CMake file, could we put this behind a watchmanconfig (and default it to off)? https://facebook.github.io/watchman/docs/config |
Thanks for having a look!
Sure, here you go: Test command output
Let me know if there are any other commands you'd like me to try.
Sure, this makes sense to me. Does an option like |
The reasoning for this is already outlined well in #1092.
This would be useful for us in Homebrew, and would probably limit the
number of bug reports you get from Homebrew users that get tripped up by
our hard-coded
WATCHMAN_STATE_DIR
. (See, e.g., #963.)It should also reduce the number of users who end up with
brew
building Watchman from source because they're using a non-default
prefix, and hopefully issues from them too (e.g. #1132).
Closes #1092.