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

Avoid crashing when the user has no homedir #2814

Merged
merged 1 commit into from Feb 8, 2022
Merged

Conversation

ewjoachim
Copy link
Contributor

@ewjoachim ewjoachim commented Jan 27, 2022

Description

See #2813
Avoid crashing when the user has no homedir

Checklist - did you ...

  • Add a CHANGELOG entry if necessary?
  • Add / update tests if necessary?
  • Add new / update outdated documentation? < not applicable

Things left to discuss

Regarding the tests, I'm unsure what the best strategy is here. As far as I can tell, the current PermissionError path isn't tested, and the find_pyproject_toml function is not tested directly in the tests.

Should I:

  • Add new tests for find_pyproject_toml, covering all function paths ?
  • Add new tests for find_pyproject_toml, covering just my new path ?
  • Add tests on the caller function read_pyproject_toml ?
  • Or smething else ?

@ewjoachim
Copy link
Contributor Author

@ewjoachim ewjoachim commented Jan 27, 2022

Yep, I confirm that the code I modified is among the 5% of code that is not currently tested:

https://coveralls.io/builds/45988623/source?filename=src%2Fblack%2Ffiles.py#L90

Copy link
Collaborator

@felix-hilden felix-hilden left a comment

Thanks for submitting!

If we wanted to be really careful, we could catch the error deeper and raise a custom one, since RuntimeError could result from other things as well. But because we print the message anyway it isn't a big deal.

Not sure what would be the best way to write the test though. Let me get back to you!

CHANGES.md Outdated Show resolved Hide resolved
@ichard26 ichard26 self-requested a review Jan 27, 2022
@github-actions
Copy link

@github-actions github-actions bot commented Jan 27, 2022

diff-shades reports zero changes comparing this PR (667d021) to main (fb9fe6b).


What is this? | Workflow run | diff-shades documentation

Copy link
Collaborator

@cooperlees cooperlees left a comment

Nice. Love simple fixes and thanks for adding the explanations of the exceptions and what they normally mean.

We could test by writing a unit test and setting the os.eviron['HOME] = /not_exists and limit the test to only run when platform.system() == "Linux".

If that does not work mocking find_user_pyproject_toml() to throw a RuntimeError would also work ...

@ewjoachim
Copy link
Contributor Author

@ewjoachim ewjoachim commented Jan 27, 2022

I believe that just unsetting $HOME will not be enough, by default Python uses both $HOME and passwd, so a real test would really need a non-existant user. I'd say mocking will be much easier (but it depends on this exception being stable in Python... Which is probably a safe bet)

@ewjoachim
Copy link
Contributor Author

@ewjoachim ewjoachim commented Jan 27, 2022

So, I'll try the path "Add new tests for find_pyproject_toml, covering all function paths" right ? @felix-hilden should I wait for you to get back to me as you mentionned or was @cooperlees 's answer the maintainer consensus ?

@JelleZijlstra
Copy link
Collaborator

@JelleZijlstra JelleZijlstra commented Jan 27, 2022

I haven't tried this, but what happens if the home directory points to a non-existent directory? I would expect that to raise FileNotFoundError, which isn't currently caught.

@felix-hilden
Copy link
Collaborator

@felix-hilden felix-hilden commented Jan 27, 2022

Both of what Cooper suggested sound good!

@ewjoachim
Copy link
Contributor Author

@ewjoachim ewjoachim commented Jan 28, 2022

@JelleZijlstra strangely enough, when HOME is defined to a non-existing folder, black seems ok.

I have no name!@fde3ae008de9:/$ export HOME=/foo
I have no name!@fde3ae008de9:/$ /tmp/venv/bin/black /tmp
No Python files are present to be formatted. Nothing to do 😴

(same with unset HOME crashes black)

@ewjoachim ewjoachim force-pushed the no-homedir branch 3 times, most recently from 189cf55 to c3fcaee Compare Jan 29, 2022
@ewjoachim
Copy link
Contributor Author

@ewjoachim ewjoachim commented Jan 29, 2022

Ok I've added a test that mocks. Took me longer to write than what I had expected, but here it is. A few notes:

  • I wanted to parametrize the error class, but I didn't figure out how to use pytest.mark.parametrize in this context (it's been years since I worked with unittest-based pytest tests). It didn't add my parameter to the test function parameters, and I don't know why. Maybe it doesn't play well with patch ?
  • The fact that it was unit-test based meant I couldn't use the capsys standard fixture, so I resorted to contextlib.capture_stderr. When looking at the rest of the code, it seems we never check stderr messages expect in integration tests (calling the whole CLI with invoke, rather than a single function). I could try to do that too, but I'm afraid it would slow down the test suite and I thought a scoped unit test would be more appropriate, though I'm not sure. Let me know I this needs to be changed.

Overall, I want to thank you all for your reactivity on the PR ! Lots of reactions, advice, etc. Great first contribution experience :) That said... I think some changes in the testing strategy may make the experience even better. I'll be glad to provide some feedback in a dedicated issue if you're interested :)

@felix-hilden
Copy link
Collaborator

@felix-hilden felix-hilden commented Jan 29, 2022

Thanks! We're in the process of cutting a release, so this will likely be worked on afterwards. We do have a feedback issue: #2238 Any feedback is much appreciated!

@JelleZijlstra
Copy link
Collaborator

@JelleZijlstra JelleZijlstra commented Jan 31, 2022

The changelog entry is now in the wrong place, we need to merge main in to fix it.

@ewjoachim
Copy link
Contributor Author

@ewjoachim ewjoachim commented Feb 1, 2022

Rebased and fixed the changelog :) 👍

@ichard26 ichard26 removed their request for review Feb 1, 2022
@ichard26
Copy link
Collaborator

@ichard26 ichard26 commented Feb 1, 2022

I can't actually reproduce #2813 for some reason, I suspect this is something on my end 🤔

Copy link
Collaborator

@felix-hilden felix-hilden left a comment

This seems like a pain to try to reproduce indeed, but the fix is simple enough so I trust the stack trace of the bug report. I can't imagine where it would hurt to catch RuntimeError to gracefully inform that home was not found, unless we really want to print the full trace to the user.


I can't even guess what hitting this would take on Windows 😄

@ewjoachim
Copy link
Contributor Author

@ewjoachim ewjoachim commented Feb 2, 2022

Here's me reproducing it in the same setup as the original bug:

$ docker run -u=1234 --rm -it python bash
Unable to find image 'python:latest' locally
latest: Pulling from library/python
0c6b8ff8c37e: Pull complete
412caad352a3: Pull complete
e6d3e61f7a50: Pull complete
461bb1d8c517: Pull complete
808edda3c2e8: Pull complete
724cfd2dc19b: Pull complete
8bd4965a24ab: Pull complete
fccd5fa208a8: Pull complete
af1ca64a0eec: Pull complete
Digest: sha256:a7a73f894e756267b2bac3b068e51ad50aa06f16855a9c6b208630d48937796f
Status: Downloaded newer image for python:latest
$ I have no name!@3da66efb3db4:/$ python -m venv /tmp/venv
$ I have no name!@3da66efb3db4:/$ /tmp/venv/bin/pip install black
WARNING: The directory '/.cache/pip' or its parent directory is not owned or is not writable by the current user. The cache has been disabled. Check the permissions and owner of that directory. If executing pip with sudo, you should use sudo's -H flag.
Requirement already satisfied: black in /tmp/venv/lib/python3.10/site-packages (21.12b1.dev88+g667d021)
Collecting black
  Downloading black-22.1.0-cp310-cp310-manylinux_2_17_x86_64.manylinux2014_x86_64.whl (1.5 MB)
     |████████████████████████████████| 1.5 MB 4.2 MB/s
Requirement already satisfied: pathspec>=0.9.0 in /tmp/venv/lib/python3.10/site-packages (from black) (0.9.0)
Requirement already satisfied: platformdirs>=2 in /tmp/venv/lib/python3.10/site-packages (from black) (2.4.1)
Requirement already satisfied: mypy-extensions>=0.4.3 in /tmp/venv/lib/python3.10/site-packages (from black) (0.4.3)
Requirement already satisfied: click>=8.0.0 in /tmp/venv/lib/python3.10/site-packages (from black) (8.0.3)
Requirement already satisfied: tomli>=1.1.0 in /tmp/venv/lib/python3.10/site-packages (from black) (2.0.0)
Installing collected packages: black
  Attempting uninstall: black
    Found existing installation: black 21.12b1.dev88+g667d021
    Uninstalling black-21.12b1.dev88+g667d021:
      Successfully uninstalled black-21.12b1.dev88+g667d021
Successfully installed black-22.1.0
WARNING: You are using pip version 21.2.4; however, version 22.0.2 is available.
You should consider upgrading via the '/tmp/venv/bin/python -m pip install --upgrade pip' command.
$ I have no name!@3da66efb3db4:/$ unset HOME
$ I have no name!@3da66efb3db4:/$ /tmp/venv/bin/black /tmp/
Traceback (most recent call last):
  File "/tmp/venv/bin/black", line 8, in <module>
    sys.exit(patched_main())
  File "src/black/__init__.py", line 1424, in patched_main
  File "/tmp/venv/lib/python3.10/site-packages/click/core.py", line 1128, in __call__
    return self.main(*args, **kwargs)
  File "/tmp/venv/lib/python3.10/site-packages/click/core.py", line 1052, in main
    with self.make_context(prog_name, args, **extra) as ctx:
  File "/tmp/venv/lib/python3.10/site-packages/click/core.py", line 914, in make_context
    self.parse_args(ctx, args)
  File "/tmp/venv/lib/python3.10/site-packages/click/core.py", line 1370, in parse_args
    value, args = param.handle_parse_result(ctx, opts, args)
  File "/tmp/venv/lib/python3.10/site-packages/click/core.py", line 2347, in handle_parse_result
    value = self.process_value(ctx, value)
  File "/tmp/venv/lib/python3.10/site-packages/click/core.py", line 2309, in process_value
    value = self.callback(ctx, self, value)
  File "src/black/__init__.py", line 117, in read_pyproject_toml
  File "/tmp/venv/lib/python3.10/site-packages/black/files.py", line 84, in find_pyproject_toml
    path_user_pyproject_toml = find_user_pyproject_toml()
  File "/tmp/venv/lib/python3.10/site-packages/black/files.py", line 120, in find_user_pyproject_toml
    user_config_path = Path(config_root).expanduser() / "black"
  File "/usr/local/lib/python3.10/pathlib.py", line 1438, in expanduser
    raise RuntimeError("Could not determine home directory.")
RuntimeError: Could not determine home directory.
$ I have no name!@3da66efb3db4:/$ /tmp/venv/bin/python -m pip install -U git+https://github.com/ewjoachim/black.git@no-homedir
WARNING: The directory '/~/.cache/pip' or its parent directory is not owned or is not writable by the current user. The cache has been disabled. Check the permissions and owner of that directory. If executing pip with sudo, you should use sudo's -H flag.
Collecting git+https://github.com/ewjoachim/black.git@no-homedir
  Cloning https://github.com/ewjoachim/black.git (to revision no-homedir) to /tmp/pip-req-build-h3bils9b
  Running command git clone -q https://github.com/ewjoachim/black.git /tmp/pip-req-build-h3bils9b
  Running command git checkout -b no-homedir --track origin/no-homedir
  Switched to a new branch 'no-homedir'
  Branch 'no-homedir' set up to track remote branch 'no-homedir' from 'origin'.
  Resolved https://github.com/ewjoachim/black.git to commit 667d02184ca536cb9478cd744403a2780080fcf3
  Installing build dependencies ... done
  Getting requirements to build wheel ... done
    Preparing wheel metadata ... done
Requirement already satisfied: pip in /tmp/venv/lib/python3.10/site-packages (21.2.4)
Requirement already satisfied: install in /tmp/venv/lib/python3.10/site-packages (1.3.5)
Requirement already satisfied: platformdirs>=2 in /tmp/venv/lib/python3.10/site-packages (from black==21.12b1.dev88+g667d021) (2.4.1)
Requirement already satisfied: tomli>=1.1.0 in /tmp/venv/lib/python3.10/site-packages (from black==21.12b1.dev88+g667d021) (2.0.0)
Requirement already satisfied: click>=8.0.0 in /tmp/venv/lib/python3.10/site-packages (from black==21.12b1.dev88+g667d021) (8.0.3)
Requirement already satisfied: pathspec>=0.9.0 in /tmp/venv/lib/python3.10/site-packages (from black==21.12b1.dev88+g667d021) (0.9.0)
Requirement already satisfied: mypy-extensions>=0.4.3 in /tmp/venv/lib/python3.10/site-packages (from black==21.12b1.dev88+g667d021) (0.4.3)
Building wheels for collected packages: black
  Building wheel for black (PEP 517) ... done
  Created wheel for black: filename=black-21.12b1.dev88+g667d021-py3-none-any.whl size=161166 sha256=fa1dab611c071521d2545f839b6cf05cdff0ddb0fbfeb1dda58e8dfc66bd6c95
  Stored in directory: /tmp/pip-ephem-wheel-cache-18xvmgi3/wheels/a3/a6/4e/e9b0869112fb3d8d9cd0df34509e8f15f66d8023696d486b03
Successfully built black
Installing collected packages: black
  Attempting uninstall: black
    Found existing installation: black 22.1.0
    Uninstalling black-22.1.0:
      Successfully uninstalled black-22.1.0
Successfully installed black-21.12b1.dev88+g667d021
WARNING: You are using pip version 21.2.4; however, version 22.0.2 is available.
You should consider upgrading via the '/tmp/venv/bin/python -m pip install --upgrade pip' command.
$ I have no name!@3da66efb3db4:/$ /tmp/venv/bin/black /tmp/
Ignoring user configuration directory due to RuntimeError('Could not determine home directory.')
No Python files are present to be formatted. Nothing to do 😴

In particular, note that it installed black-21.12b1.dev88+g667d021 which matches the last commit of this branch, 667d021

@ewjoachim
Copy link
Contributor Author

@ewjoachim ewjoachim commented Feb 2, 2022

If I have to take a guess, I'd say that if you can't reproduce, it's either because your current user has an entry in /etc/passwd specifying your home folder, or that you have a HOME envvar. For the bug to hit, we need to have neither.

Copy link
Contributor

@Shivansh-007 Shivansh-007 left a comment

Thanks! Looks Good To Me! :shipit:

Screenshot from 2022-02-02 23-05-25

@cooperlees
Copy link
Collaborator

@cooperlees cooperlees commented Feb 2, 2022

I am going to play devils advocate here and UNIX BOFH. I feel, if you don't have a passwd entry (or equivalent) AND don't have a home directory, why are you running black as that user?

I feel maybe we should error here and the administrators should ideally fix the environment. Are we potentially hiding a problem here with this "fix"?

@jack1142
Copy link
Contributor

@jack1142 jack1142 commented Feb 2, 2022

System users do not have a home directory by default.

@cooperlees
Copy link
Collaborator

@cooperlees cooperlees commented Feb 2, 2022

If I have to take a guess, I'd say that if you can't reproduce, it's either because your current user has an entry in /etc/passwd specifying your home folder, or that you have a HOME envvar. For the bug to hit, we need to have neither.

^^ I was more replying to ensure we don't expect to support "your current user has an entry in /etc/passwd"

Agree with system users. But it's best practice to make dedicated users in an ideal world. I get that's not always practical tho.

@ewjoachim
Copy link
Contributor Author

@ewjoachim ewjoachim commented Feb 8, 2022

Yeah, it's not a common setup... until it is :D

When using Jenkins, and the docker agent, Jenkins will launch your script in the image of your choice. It will use a volume share to have the jenkins workspace exposed inside the docker, and it will request that the docker daemon uses the UID of the host Jenkins user (the owner of the workspace files) inside the docker, so that if you create files from within the docker, you're not likely to create a horrible permission issue that would prevent the host from even removing the jenkins workspace. Unless you build the docker image in the pipeline, you'll likely not have an existing user inside the Docker image with a UID matching the host jenkins user, so you'll most likely end up with a non-existing user. And unless you define a HOME envvar, none is defined for you.

While we could "just define a HOME envvar", it felt strange to write a comment saying "Add a fake HOME envvar because Black crashes otherwise", which is why I thought it was more natural to fix it here.

(note: you can't adduser in this case, unless your image has sudo and lets absolutely anyone including non-existing user sudo, which is not usual. The only way to fix this is to build your own image, passing UID and GID as build-args. Which would probably make Jenkins runs longer, and force all projects to have a Dockerfile, just to solve this problem.)

@JelleZijlstra JelleZijlstra merged commit b4a6bb0 into psf:main Feb 8, 2022
38 checks passed
@JelleZijlstra
Copy link
Collaborator

@JelleZijlstra JelleZijlstra commented Feb 8, 2022

Thanks. This seems like a low-risk fix and it addresses a real problem, so let's just land it.

@ewjoachim ewjoachim deleted the no-homedir branch Feb 8, 2022
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.

None yet

7 participants