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

handle failures to find a user home directory #278

Merged
merged 5 commits into from
Aug 26, 2024

Conversation

cj81499
Copy link
Contributor

@cj81499 cj81499 commented Aug 11, 2024

In certain environments (notably, the bazel sandbox on windows), it is possible for pathlib.Path('~').expanduser() to fail to find the user home directory and raise a RuntimeError. This causes distutils (and ultimately, setuptools) to fail.

With this patch, we catch and handle the exception by logging a warning and continuing without a user's config file.

motivated by bazelbuild/rules_python#1067

In certain environments (notably, the [bazel sandbox](https://bazel.build/docs/sandboxing) on windows), it is possible for `pathlib.Path('~').expanduser()` to fail to find the user home directory and [raise a `RuntimeError`](https://docs.python.org/3/library/pathlib.html#pathlib.Path.expanduser). This causes `distutils` (and ultimately, `setuptools`) to fail.

With this patch, we catch and handle the exception by logging a warning and continuing without a user's config file.

motivated by bazelbuild/rules_python#1067
distutils/dist.py Outdated Show resolved Hide resolved
@jaraco
Copy link
Member

jaraco commented Aug 22, 2024

In certain environments (notably, the bazel sandbox on windows), it is possible for pathlib.Path('~').expanduser() to fail

In other scenarios, I've taken measures to explicitly set the home dir to a known location. That approach has the benefit of allowing downstream packages (like distutils) to continue to operate normally and only alters the behavior in the environments that are relevant (e.g. bazel sandbox).

Wouldn't it be better for bazel sandbox to provide a suitable (empty) home directory and thus address this concern for any project running in the sandbox?

cj81499 and others added 2 commits August 25, 2024 23:18
Co-authored-by: Anderson Bravalheri <andersonbravalheri+github@gmail.com>
@cj81499
Copy link
Contributor Author

cj81499 commented Aug 26, 2024

In other scenarios, I've taken measures to explicitly set the home dir to a known location.

That's pretty much the workaround I implemented in https://github.com/bazelbuild/rules_python/pull/1067/files#diff-a92cc6468f47191b8e22d4bbe2564d3968d3dd4bbc383b9f3df3c4c520e1583eR157-R165.

Wouldn't it be better for bazel sandbox to provide a suitable (empty) home directory and thus address this concern for any project running in the sandbox?

Personally, I can't think of a reason for setuptools/distutils to put the burden of ensuring a home directory exists on its users and/or other projects (rules_python/bazel).
Failing to find a home directory that might contain a user config file and failing to find a user config file within a located home directory are both ways that setuptools/distutils might fail to find a user config file, so I would expect it to behave the same (continue the build without using user configuration) in both cases.

For what it's worth, other Python build backends (eg: flit, hatchling) do not fail in the event that you attempt to build without a valid home directory.

@jaraco
Copy link
Member

jaraco commented Aug 26, 2024

Personally, I can't think of a reason for setuptools/distutils to put the burden of ensuring a home directory exists on its users and/or other projects (rules_python/bazel).

I think I agree with you here. However, there's another factor to consider. Do you want rules_python to be sensitive to settings in the user's home directory? The reason I created those exclusions was not because the project failed when there was no home directory, but because the user's configuration changed the default behaviors, affecting tests that were meant to run in isolation. My guess is that rules_python would want the same isolation, which you'll lose if we roll out this change and then you remove the workaround.

That said, you make a compelling argument. There's not a very good reason for distutils not to be resilient under such conditions.

distutils/dist.py Outdated Show resolved Hide resolved
distutils/dist.py Outdated Show resolved Hide resolved
@jaraco jaraco merged commit 6c224d3 into pypa:main Aug 26, 2024
19 checks passed
@cj81499
Copy link
Contributor Author

cj81499 commented Aug 26, 2024

Do you want rules_python to be sensitive to settings in the user's home directory?

Personally, no.

My understanding is that bazel's sandbox restricts I/O outside of the sandbox, but not in a way that'd prevent reading a user's config file1. bazel builds that are performed remotely shouldn't be impacted though (since the home directory of the user invoking bazel build <TARGET> won't be present).
In either case, this shouldn't crash anymore, which I see as an improvement!

Footnotes

  1. The sandbox "makes the entire filesystem read-only except for the sandbox directory", so I guess the workaround should potentially stay in place, since it would be sensitive to user config without it. Perhaps we actually ought to change the workaround to fake the home directory on other platforms (not just windows) too. I'll share this comment with the maintainers of rules_python for their consideration.
    EDIT: see https://github.com/bazelbuild/rules_python/issues/2121#issuecomment-2311198431 and feel free to chime in with any advice you have there!

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

Successfully merging this pull request may close these issues.

3 participants