-
-
Notifications
You must be signed in to change notification settings - Fork 636
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
Fix 'current' platform handling. #6104
Fix 'current' platform handling. #6104
Conversation
5 Legit looking errors in
|
A similar issue in
And a slightly different looking error as well:
|
@jsirois I've been getting that first error ever since I started working on this project two weeks ago when running |
Thanks for the independent datapoint @Eric-Arellano. I run linux so this is especially useful to me. That says we definitely have more problems than just in |
@jsirois I have a good recollection of that platform-based python_distribution test and the case it is covering. I can look into that. |
Thanks @CMLivingston |
I cannot repro the osx shard failure locally (https://travis-ci.org/pantsbuild/pants/jobs/403189175). Would anyone else be able to take a crack at the tests and report any failures? From what I can see in the output log, it looks like the wheel is named properly and the os version aligns with mine, so I can't see why this would fail. |
OK, This hacky fix debug output (see:
|
This is more work related to workarounds for pex-tool/pex#511. Most of the logic introduced here - or all of it - should move into |
OK @CMLivingston all linux test failures are now fixed so I'll dig into |
Noting that in the OSX failures, The fix above is designed to handle that case in the resolve_multi path. I suspect the python distribution code represents another - unfixed path. |
@@ -227,7 +227,8 @@ def _create_dist(self, dist_tgt, dist_target_dir, interpreter, | |||
|
|||
setup_runner = SetupPyRunner.for_bdist_wheel( | |||
dist_target_dir, | |||
is_platform_specific=is_platform_specific) | |||
is_platform_specific=is_platform_specific, | |||
interpreter=interpreter) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
OK @CMLivingston and @cosmicexplorer - other issues elsewhere aside - this code never stood a fair chance of working since the selected interpreter was not passed.
The interpreter selection bit you just described seems utterly fantastic because
is what i spent an entire migraine on trying to understand in the osx shard in #6022 last week (which you and Kris were there for, which I appreciate immensely) — not trying to whine, just this seems like it would directly address that maybe, so that’s great. |
9bd378a
to
ab976db
Compare
Noting current issue on OSX: With the correct interpreter being used for run/test/repl/... pexes, which is the system 2.7.10 in this case (as opposed to brew 2.7.14), the accepted platforms calculation looks wrong - no
|
6196ae2
to
503b193
Compare
Awesome! I'm running |
6 tests fail now instead of I think it was 11 earlier, that's exciting! https://hastebin.com/oxinuwepol.rb One of the failures is the same as CI's unit test failure, |
@Eric-Arellano note well that the OSX fix for Travis was ultimately getting off an OSX VM with Apple's supplied 2.7.10 which returns a broken -intel build platform. If you too have that, this may not provide you relief locally yet. |
Previously the platform passed for 'current' was partial, leading to ambiguous resolves. Further, partial platforms in user's BUILD files also would lead to ambiguous resolves. For example, a multi-platform `python_binary` with something like `platforms=['current', 'linux-x86_64', 'macosx_10.11_x86_64']`. Expand and and then fixup platforms as required by replacing `get_local_platform` with `expand_and_maybe_adjust_platform`.
+ Fix an unused `interpreter` parameter - forward to `SetupPyRunner`. + Kill declaring `--universal` on behalf of the package author. There is not enough info to make the decision if the code being dist'd, if pure python, is 2/3 compatible. To compensate for the removal of `--universal`, give an example of how to declare your python_dist as `--universal` when you know it is as the BUILD `python_dist` target author.
833d40b
to
fe2907e
Compare
Thanks @Eric-Arellano - I was able to repro that failure on linux and it should be fixed this round. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
lgtm!
abi_tag=ident.abi_tag)) | ||
|
||
|
||
def expand_and_maybe_adjust_platform(interpreter, platform): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
a TODO to upstream this into pex might be good?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yup - this old one pex-tool/pex#511, I'll add a TODO similar to the method above.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed.
env = copy(kwargs.pop('env', {})) | ||
|
||
# Hack around bug in PEX where custom interpreters are not forwarded to PEXEnvironments. | ||
# TODO(John Sirois): Remove when XXX is fixed. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
s/XXX
/a ticket?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yup - just finished filing on the pex side. I'll send up a final comment-only diff with issue inks here as soon as this goes green then merge: pex-tool/pex#522
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed.
.travis.yml
Outdated
@@ -476,6 +476,10 @@ matrix: | |||
language: generic | |||
env: | |||
- SHARD="Rust + Platform-specific Tests OSX" | |||
# Specifically avoid the OSX provided 2.7.10 under xcode8.3 since it returns a platform | |||
# of `macosx-*-intel` where the `intel` suffix is bogus but pex has not yet been taught to | |||
# deal with this. Can be removed when this issue is resolved: YYY |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
YYY -> pex-tool/pex#523
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed.
Previously the platform passed for 'current' was partial, leading to
ambiguous resolves. Further, partial platforms in user's BUILD files
also would lead to ambiguous resolves. For example, a multi-platform
python_binary
with something likeplatforms=['current', 'linux-x86_64', 'macosx_10.11_x86_64']
.Expand and and then fixup platforms as required by replacing
get_local_platform
withexpand_and_maybe_adjust_platform
.