Skip to content
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

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

carlocab
Copy link
Contributor

@carlocab carlocab commented Aug 2, 2024

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.

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
Copy link
Contributor

@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`.
@facebook-github-bot
Copy link
Contributor

@carlocab has updated the pull request. You must reimport the pull request before landing.

@facebook-github-bot
Copy link
Contributor

@facebook-github-bot has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator.

@carlocab
Copy link
Contributor Author

carlocab commented Aug 6, 2024

CC @andrewhamon for thoughts, since you might be interested.

@genevievehelsel
Copy link
Contributor

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

@carlocab
Copy link
Contributor Author

carlocab commented Sep 6, 2024

Thanks for having a look!

Would you mind sharing some manual test output/examples to verify the behavior and that it works?

Sure, here you go:

Test command output
❯ _build/bin/watchman; echo $?
0

❯ _build/bin/watchman get-sockname
{
    "sockname": "/Users/carlocab/.local/state/watchman/carlocab-state/sock",
    "version": "20240803.002435.0",
    "unix_domain": "/Users/carlocab/.local/state/watchman/carlocab-state/sock"
}

❯ _build/bin/watchman watch .
{
    "version": "20240803.002435.0",
    "watcher": "fsevents",
    "watch": "/Users/carlocab/github/watchman"
}

❯ _build/bin/watchman watch-list
{
    "version": "20240803.002435.0",
    "roots": [
        "/Users/carlocab/github/watchman"
    ]
}

❯ _build/bin/watchman shutdown-server
{
    "version": "20240803.002435.0",
    "shutdown-server": true
}

Let me know if there are any other commands you'd like me to try.

Also, instead of gating this at build time in the CMake file, could we put this behind a watchmanconfig (and default it to off)? facebook.github.io/watchman/docs/config

Sure, this makes sense to me. Does an option like use_xdg_state_dir or similar sound ok?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Allow stateDir that is based on $HOME
3 participants