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

PythonTests force default platform resolves #7618

Merged

Conversation

Projects
None yet
4 participants
@illicitonion
Copy link
Contributor

commented Apr 24, 2019

Fixes #7616.

Without this PR, if a test and a binary are both on the same command
line, and the binary specifies an explicit platform not in the default
platforms, we won't resolve for the default platform, and the test will
fail to run because it will be missing dependencies for its platform.

This forces any PythonTests target in the graph to contribute to which
platforms will be resolved.

PythonTests force default platform resolves
Fixes #7616.

Without this PR, if a test and a binary are both on the same command
line, and the binary specifies an explicit platform not in the default
platforms, we won't resolve for the default platform, and the test will
fail to run because it will be missing dependencies for its platform.

This forces any PythonTests target in the graph to contribute to which
platforms will be resolved.

@illicitonion illicitonion requested review from ity, blorente and Eric-Arellano Apr 24, 2019

@Eric-Arellano
Copy link
Contributor

left a comment

Thank you!

# the target doesn't have a platforms property.
#
# If we don't do this, the default configured platform may not be resolved (specifically if a
# PythonBinary is also built at the same time, which explicitly defines platforms not

This comment has been minimized.

Copy link
@Eric-Arellano

Eric-Arellano Apr 24, 2019

Contributor

What about PythonDistribution? I would expect the same effect.

This comment has been minimized.

Copy link
@illicitonion

illicitonion Apr 24, 2019

Author Contributor

PythonDistribution does explicitly have a platforms property, and so is already covered here (because it's in the list of types checked in may_add_python_platform_to_resolve)

This comment has been minimized.

Copy link
@illicitonion

illicitonion Apr 25, 2019

Author Contributor

Removed comment :)

@ity

ity approved these changes Apr 24, 2019

Copy link
Contributor

left a comment

thank you!

@blorente
Copy link
Contributor

left a comment

Thanks!

if can_have_python_platform(target):
for platform in target.platforms if target.platforms else python_setup.platforms:
if may_add_python_platform_to_resolve(target):
# PythonTests doesn't have a platforms property, and should (currently) always run on the

This comment has been minimized.

Copy link
@blorente

blorente Apr 25, 2019

Contributor

Is this better than saying explicitly that PythonTests should always fall back to default platforms? Something like:

if has_to_run_under_default_platform(target): # PythonTests case
  for platform in python_setup.platforms:
    # do something
elif may_add_python_platform_to_resolve(target):
  # Do something

Then, this comment could move to the docstring of has_to_run_under_default_platform, and we avoid some possibly stale code.

That said, this is probably bike-sheddy :)

This comment has been minimized.

Copy link
@illicitonion

illicitonion Apr 25, 2019

Author Contributor

Yeah, I was 50:50 on this... I prefer splitting out the separate case, as it means I can delete a lot of comments. Done.

@illicitonion illicitonion merged commit 3b71834 into pantsbuild:master Apr 26, 2019

1 check passed

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

@illicitonion illicitonion deleted the twitter:dwagnerhall/python_tests/platforms branch Apr 26, 2019

illicitonion added a commit that referenced this pull request May 15, 2019

Fix 'current' platform check for python_dist() targets with C/C++ sou…
…rces (#7687)

### Problem

Currently, in BuildLocalPythonDistributions, any `python_dist()` targets which depend on shared libs we compile and link from `ctypes_compatible_cpp_library()` targets are converted into platform-specific wheels, with the platform matching the current host (this is then injected into the build graph as a PythonRequirementLibrary). In ResolveRequirementsTaskBase, we attempt to check for whether any such `python_dist()` targets with native code exist, and if so, ensure we only resolve for the platform `['current']`. This check is currently broken in such a way that we never find any native code, and we always try to resolve for all the platforms in `--python-setup-platforms` (or whatever platforms may be set on individual `python_binary()` targets). If `--python-setup-platforms` has anything besides `['current']`, this resolve will fail, because we've only built our distributions with native code locally, i.e. for the current platform.

This wasn't noticed until #7618, which ended up essentially assigning `--python-setup-platforms` to `python_tests()` targets, causing the resolve to fail in our repo when running tests. We also then realized that we don't actually ever need to resolve for anything but the `'current'` platform *except* explicitly in `PythonBinaryCreate`, so we were able to remove all of this logic from `ResolveRequirementsTaskBase` and only trigger the check when it actually needs to fail.

### Solution

- Don't check the platforms and always resolve for `'current'` in `ResolveRequirementsTaskBase#resolve_requirements()`.
- Work around the issue catalyzed by #7618 by not assigning any platforms to `python_tests()` when platform checking unless the issue in #7616 is encountered specifically.
- Fix the broken platform check.
- Do a very small refactor of some of the `python_dist()` testing.

### Result

The platform checking in ResolveRequirementsTaskBase is removed entirely, only occurring during python binary creation instead. When that platform checking does occur, `python_tests()` will only be assigned platforms if another target has an explicit `platforms` setting, and `python_dist()` targets with native code will be successfully resolved by the `ResolveRequirements` task.

illicitonion added a commit that referenced this pull request May 15, 2019

Fix 'current' platform check for python_dist() targets with C/C++ sou…
…rces (#7687)

### Problem

Currently, in BuildLocalPythonDistributions, any `python_dist()` targets which depend on shared libs we compile and link from `ctypes_compatible_cpp_library()` targets are converted into platform-specific wheels, with the platform matching the current host (this is then injected into the build graph as a PythonRequirementLibrary). In ResolveRequirementsTaskBase, we attempt to check for whether any such `python_dist()` targets with native code exist, and if so, ensure we only resolve for the platform `['current']`. This check is currently broken in such a way that we never find any native code, and we always try to resolve for all the platforms in `--python-setup-platforms` (or whatever platforms may be set on individual `python_binary()` targets). If `--python-setup-platforms` has anything besides `['current']`, this resolve will fail, because we've only built our distributions with native code locally, i.e. for the current platform.

This wasn't noticed until #7618, which ended up essentially assigning `--python-setup-platforms` to `python_tests()` targets, causing the resolve to fail in our repo when running tests. We also then realized that we don't actually ever need to resolve for anything but the `'current'` platform *except* explicitly in `PythonBinaryCreate`, so we were able to remove all of this logic from `ResolveRequirementsTaskBase` and only trigger the check when it actually needs to fail.

### Solution

- Don't check the platforms and always resolve for `'current'` in `ResolveRequirementsTaskBase#resolve_requirements()`.
- Work around the issue catalyzed by #7618 by not assigning any platforms to `python_tests()` when platform checking unless the issue in #7616 is encountered specifically.
- Fix the broken platform check.
- Do a very small refactor of some of the `python_dist()` testing.

### Result

The platform checking in ResolveRequirementsTaskBase is removed entirely, only occurring during python binary creation instead. When that platform checking does occur, `python_tests()` will only be assigned platforms if another target has an explicit `platforms` setting, and `python_dist()` targets with native code will be successfully resolved by the `ResolveRequirements` task.
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.