Skip to content

gh-104522: Change message returned by subprocess when cwd option is not the cause #104851

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

Closed
wants to merge 7 commits into from

Conversation

PurityLake
Copy link
Contributor

@PurityLake PurityLake commented May 24, 2023

Refers to #104522

Summary of change:
subprocess._execute_child would raise an exception referencing cwd as the cause of the issue even if this not the case.

So in the sample code:

import subprocess
subprocess.check_call(["pwd"], user="root", cwd="/tmp") 
# or
subprocess.check_call(["pwd"], pass_fds=[30,], cwd="/tmp")

subprocess will put the blame on cwd but raise the correct exception. This can be misleading to the user as to what the correct cause is.

Current state of this PR,:
The exception type is checked by instantiating an error object store in child_exception_type which is OSError, but when supplied the errno and any string can be tested with isinstance to check for FileNotFoundError and NotADirectoryError, these exceptions imply that cwd is most certainly the error as at this point as the subprocess was in preexec. Although if other errors are raise in this context the errno and message are still raised but contents of cwd will not be passed the exception.

@serhiy-storchaka
Copy link
Member

Good idea, but not very reliable. Other errno can be set by failing chdir(cwd) (such as EACCES or ELOOP), and we cannot guarantee that ENOENT and ENOTDIR can only be set by this call. I implemented more reliable way in #114195.

@gpshead
Copy link
Member

gpshead commented Jan 18, 2024

thanks, Serhiy's other PR has been merged! :)

@gpshead gpshead closed this Jan 18, 2024
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.

4 participants