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

move wheel extraction for UnpackWheels into PexBuilderWrapper and resolve for a single platform only #7289

Conversation

Projects
None yet
3 participants
@cosmicexplorer
Copy link
Contributor

commented Feb 26, 2019

Problem

Resolves #7245. We are seeing wheels getting unpacked for the wrong platform nondeterministically in our internal repo, where we have multiple platforms in python-setup.platforms.

Solution

  • Resolve wheels for UnpackWheels for the current platform only.
  • Ensure that there is exactly a single wheel resolved for the desired distribution name.

Result

The complex and unnecessary dist resolution / location process in UnpackWheels is a lot smoother, we resolve for only a single platform, and we check to ensure only a single dist can be located from the resolved requirements.

@cosmicexplorer cosmicexplorer requested review from stuhood and jsirois Feb 26, 2019

@stuhood
Copy link
Member

left a comment

Looks good, thanks.

It's probably worth clarifying somewhere (either on the unpacked wheels task or target) that it always resolves a wheel for the current platform (currently).

cosmicexplorer added some commits Feb 26, 2019

@cosmicexplorer cosmicexplorer force-pushed the cosmicexplorer:simplify-unpack-wheels-dist-selection branch from 6e1e4c3 to 299fe66 Feb 28, 2019

except (StopIteration, ValueError) as e:
raise self.SingleDistExtractionError(
"Exactly one dist was expected to match name {} in requirements {}: {}"
.format(dist_key, reqs, e))

This comment has been minimized.

Copy link
@baroquebobcat

baroquebobcat Feb 28, 2019

Contributor

It'd be nice to have a test that exercises this case, if there isn't already.

This comment has been minimized.

Copy link
@cosmicexplorer

cosmicexplorer Mar 1, 2019

Author Contributor

Currently there is no testing except the testprojects integration test :) Will look into that now.

This comment has been minimized.

Copy link
@cosmicexplorer

cosmicexplorer Mar 1, 2019

Author Contributor

Done! This testing exposed the fact that we don't always want to descend into the <name>-<version>.data/ directory (although we probably do most of the time) -- so also specialized that to occur if the new within_data_subdir= kwarg is set. So this testing was extremely useful although there were multiple TODOs left over.

cosmicexplorer added some commits Mar 1, 2019

@cosmicexplorer cosmicexplorer force-pushed the cosmicexplorer:simplify-unpack-wheels-dist-selection branch from f09b5fe to baec9a6 Mar 1, 2019

@baroquebobcat
Copy link
Contributor

left a comment

Thanks for the tests!

@cosmicexplorer cosmicexplorer merged commit 2c9c338 into pantsbuild:master Mar 1, 2019

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details

stuhood added a commit that referenced this pull request Mar 1, 2019

move wheel extraction for UnpackWheels into PexBuilderWrapper and res…
…olve for a single platform only (#7289)

Resolves #7245. We are seeing wheels getting unpacked for the wrong platform nondeterministically in our internal repo, where we have multiple platforms in `python-setup.platforms`.

- Resolve wheels for `UnpackWheels` for the current platform only.
- Ensure that there is exactly a single wheel resolved for the desired distribution name.

The complex and unnecessary dist resolution / location process in `UnpackWheels` is a lot smoother, we resolve for only a single platform, and we check to ensure only a single dist can be located from the resolved requirements.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.