Skip to content

Fix extract_single_wheel for Windows#498

Merged
thundergolfer merged 1 commit intobazel-contrib:mainfrom
easybe:windows_fix
Jul 16, 2021
Merged

Fix extract_single_wheel for Windows#498
thundergolfer merged 1 commit intobazel-contrib:mainfrom
easybe:windows_fix

Conversation

@easybe
Copy link
Copy Markdown
Contributor

@easybe easybe commented Jun 28, 2021

On Windows, when using pip_parse, pip would fail with following error:

Could not open requirements file: [Errno 13] Permission denied: ...

This is due to Python holding a handle to the temporary file preventing
pip, which is run as a sub-process, from reading it. For more
information, see: https://bugs.python.org/issue14243

Closing the requirements file before running pip solves the problem.


PR Checklist

Please check if your PR fulfills the following requirements:

  • Does not include precompiled binaries, eg. .par files. See CONTRIBUTING.md for info
  • Tests for the changes have been added (for bug fixes / features)
  • Docs have been added / updated (for bug fixes / features)

PR Type

What kind of change does this PR introduce?

  • Bugfix
  • Feature (please, look at the "Scope of the project" section in the README.md file)
  • Code style update (formatting, local variables)
  • Refactoring (no functional changes, no api changes)
  • Build related changes
  • CI related changes
  • Documentation content changes
  • Other... Please describe:

What is the current behavior?

Issue Number: N/A

What is the new behavior?

pip_parse works on Windows.

Does this PR introduce a breaking change?

  • Yes
  • No

Other information

@easybe easybe requested review from brandjon and lberki as code owners June 28, 2021 20:30
@google-cla
Copy link
Copy Markdown

google-cla bot commented Jun 28, 2021

Thanks for your pull request. It looks like this may be your first contribution to a Google open source project (if not, look below for help). Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

📝 Please visit https://cla.developers.google.com/ to sign.

Once you've signed (or fixed any issues), please reply here with @googlebot I signed it! and we'll verify it.


What to do if you already signed the CLA

Individual signers
Corporate signers

ℹ️ Googlers: Go here for more info.

@google-cla google-cla bot added the cla: no label Jun 28, 2021
@easybe
Copy link
Copy Markdown
Contributor Author

easybe commented Jun 28, 2021

@googlebot I signed it!

try:
os.unlink(requirement_file.name)
except OSError:
pass
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ignoring this exception seems unsafe.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is in case, for whatever reason, the file is not present. Seems very unlikely though, I can remove it if you prefer.

Copy link
Copy Markdown
Contributor Author

@easybe easybe Jul 1, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It was inspired by: https://bugs.python.org/msg157925

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I doesn't seem too unsafe to me, as the pip subprocess would already have been run. As long as any errors from the subprocess aren't swallowed here it should be okay.

Copy link
Copy Markdown
Contributor

@hrfuller hrfuller Jul 1, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
pass
except OSError as e:
if e.errno != errno.ENOENT:
raise

Is probably better than catching any error.

pip_args += json.loads(args.extra_pip_args)["args"]

with NamedTemporaryFile(mode='wb') as requirement_file:
requirement_file = NamedTemporaryFile(mode='wb', delete=False)
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is due to Python holding a handle to the temporary file preventing pip, which is run as a sub-process, from reading it.

Interesting. This looks to contain elaboration of the issue https://bugs.python.org/issue14243

@thundergolfer thundergolfer requested review from hrfuller and removed request for brandjon and lberki July 1, 2021 05:01
@easybe easybe requested a review from thundergolfer July 1, 2021 18:10
Copy link
Copy Markdown
Contributor

@hrfuller hrfuller left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@thundergolfer
Copy link
Copy Markdown

@easybe are you able to refresh against main?

On Windows, when using pip_parse, pip would fail with following error:

    Could not open requirements file: [Errno 13] Permission denied: ...

This is due to Python holding a handle to the temporary file preventing
pip, which is run as a sub-process, from reading it. For more
information, see: https://bugs.python.org/issue14243

Closing the requirements file before running pip solves the problem.
@easybe
Copy link
Copy Markdown
Contributor Author

easybe commented Jul 6, 2021

@easybe are you able to refresh against main?

Done

@thundergolfer thundergolfer merged commit cd64466 into bazel-contrib:main Jul 16, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants