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

Resolve for current platform only if resolving a local python dist with native extensions #5618

Merged
merged 11 commits into from Mar 28, 2018

Conversation

Projects
None yet
5 participants
@CMLivingston
Copy link
Contributor

CMLivingston commented Mar 21, 2018

Problem

If a pants.ini file specifies default platforms that include more than just the current platform, local python dist targets will fail to resolve during the resolve requirements task because they are built locally for the current platform. Furthermore, the resolve requirements task does not plumb binary target platform arguments through to the pex resolver at resolve time, so we need a mechanism to check if there are native sources (from a python dist) in play, and pass only the "current" platform to the pex resolver if this is the case.

Solution

Check if a python_dist target is in play during the requirements resolution task and whether any of these targets contain native (c or cpp) sources. If they do, set the platforms parameter of the pex resolver to contain only "current".

Result

Pants projects that have pants.ini files which specify multiple default platforms can now build local python dist targets for the current platform only.

Chris Livingston

@property
def has_native_sources(self):
return any(src.endswith(('.c', '.cpp')) for src in self.sources_relative_to_target_base())

This comment has been minimized.

@UnrememberMe

UnrememberMe Mar 21, 2018

Contributor

.cc as well

@@ -51,13 +58,17 @@ def resolve_requirements(self, interpreter, req_libs):
else:
target_set_id = 'no_targets'

# We need to ensure that we are resolving for only the current platform if we are
# including local python dist targets that have native extensions.
maybe_platforms = ['current'] if self.tgt_closure_has_native_sources else None

This comment has been minimized.

@UnrememberMe

UnrememberMe Mar 21, 2018

Contributor

It's probably better to only set maybe_platforms to current if the default configuration has more than one platform and current is in the platform list.

@staticmethod
def tgt_closure_has_native_sources():
local_dist_tgts = self.context.targets(is_local_python_dist)
if local_dist_tgts:

This comment has been minimized.

@stuhood

stuhood Mar 21, 2018

Member

Should be able to remove the if here: def targets should always return a collection. But more generally: this method should always result in an explicit return, and dropping the if solves that.

@@ -34,6 +35,12 @@ def prepare(cls, options, round_manager):
round_manager.require_data(PythonInterpreter)
round_manager.optional_product(PythonRequirementLibrary) # For local dists.

@staticmethod

This comment has been minimized.

@stuhood

stuhood Mar 21, 2018

Member

This method is not static because it uses self.

This code needs a unit or integration test.

dump_requirement_libs(builder, interpreter, req_tgts, self.context.log, binary_tgt.platforms)
# We need to ensure that we are resolving for only the current platform if we are
# including local python dist targets that have native extensions.
platforms = ['current'] if self.tgt_closure_has_native_sources() else binary_tgt.platforms

This comment has been minimized.

@kwlzn

kwlzn Mar 21, 2018

Member

this seems like implicit behavior that we'd want to at a minimum warn the user about if the platforms they've defined don't match exactly the current platform?

it would probably appear like a bug to a user who wasn't expecting this if the binary they built ended up with only current platform deps if they've defined e.g. osx + linux as target platforms.

@@ -51,13 +51,17 @@ def resolve_requirements(self, interpreter, req_libs):
else:
target_set_id = 'no_targets'

# We need to ensure that we are resolving for only the current platform if we are
# including local python dist targets that have native extensions.
maybe_platforms = ['current'] if self.tgt_closure_has_native_sources() else None

This comment has been minimized.

@kwlzn

kwlzn Mar 21, 2018

Member

ditto. tho this code path is slightly less important than the one above (because presumably this is the path called for things like run or test which execute locally), a user would probably expect e.g. an unsatisfiable transitive dep for e.g. linux-x86_64 to manifest as a failure in test locally on an OSX machine - which won't happen in this case.

output = subprocess.check_output(pex)
self.assertIn('Super hello', output)
# Cleanup
os.remove(pex)

This comment has been minimized.

@kwlzn

kwlzn Mar 21, 2018

Member

cleanups like this should typically go in a try/finally block to ensure they happen even on test failure.

@@ -19,5 +19,6 @@ python_binary(
source='main.py',
dependencies=[
':fasthello',
]

This comment has been minimized.

@UnrememberMe

UnrememberMe Mar 22, 2018

Contributor

is it possible to specify platform= on python_dist as well?

This comment has been minimized.

@CMLivingston

CMLivingston Mar 22, 2018

Contributor

It is currently not supported. I cannot recall why we did not but I remember there being a reason for not supporting it, at least initially.

if not platforms:
# If no targets specify platforms, inherit the default platforms.
platforms = PythonSetup.global_instance().platforms
return platforms

This comment has been minimized.

@UnrememberMe

UnrememberMe Mar 22, 2018

Contributor

should we do a list(set(platforms)) here?

This comment has been minimized.

@CMLivingston
@CMLivingston

This comment has been minimized.

Copy link
Contributor

CMLivingston commented Mar 23, 2018

Ready for another look when possible. 👍

@stuhood
Copy link
Member

stuhood left a comment

Thanks for improving the tests: a few issues with reuse and then I think this is good to go.


@property
def has_native_sources(self):
return any(src.endswith(('.c', '.cpp', '.cc')) for src in self.sources_relative_to_target_base())

This comment has been minimized.

@stuhood

stuhood Mar 26, 2018

Member

This can be accomplished with self.has_sources(extension=('.c', '.cpp', '.cc')).

if self.tgt_closure_has_native_sources():
platforms = self.tgt_closure_platforms()
if platforms != ['current']:
raise IncompatiblePlatformsError('The target set contains one or more targets that depend on '

This comment has been minimized.

@stuhood

stuhood Mar 26, 2018

Member

This would be much more useful if it listed (the first two?) incompatible targets: ... you could maybe make self.tgt_closure_platforms() return a dict(str->list) of targets by platform, and then render the first two incompatible targets.

if self.tgt_closure_has_native_sources():
platforms = self.tgt_closure_platforms()
if platforms != ['current']:
raise IncompatiblePlatformsError('The target set contains one or more targets that depend on '

This comment has been minimized.

@stuhood

stuhood Mar 26, 2018

Member

This message is duplicated. Should move this logic to a shared location.

@@ -651,6 +653,20 @@ def determine_target_roots(self, goal_name):
# of e.g. `./pants --changed-parent=HEAD list` (w/ no changes) returning an empty result.
return self.context.target_roots

def tgt_closure_has_native_sources(self):

This comment has been minimized.

@stuhood

stuhood Mar 26, 2018

Member

This should probably not go on Task: the only use of self is to call self.context.targets(), but if a caller were to call that method directly, they could then call your utility function with the list of targets to check.

This comment has been minimized.

@stuhood

stuhood Mar 26, 2018

Member

(to be clear, all self.context.targets(predicate) is doing is doing filter(predicate, all_targets))

Chris Livingston
Performs a check of whether the current target closure has native sources and if so, ensures that
Pants is only targeting the current platform.
:context_tgts_func function: The target filtering function of the current task context.

This comment has been minimized.

@stuhood

stuhood Mar 28, 2018

Member

This parameter can just be a list of targets, and then you can call filter. There is nothing magical about self.context.targets(): if you pass it a predicate, it calls filter for you... if you don't, you get everything.

'targets are compatible with the current platform. Found platforms: {}'
.format(str(platforms)))
maybe_platforms = ['current']
maybe_platforms = ['current'] if build_for_current_platform_only_check else None

This comment has been minimized.

@stuhood

stuhood Mar 28, 2018

Member

The function is not being called here.

Chris Livingston added some commits Mar 28, 2018

Chris Livingston
Chris Livingston
Chris Livingston
@stuhood
Copy link
Member

stuhood left a comment

Thanks. Will merge on green ci.

if len(platforms.keys()) > 1 or not 'current' in platforms.keys():
raise IncompatiblePlatformsError('The target set contains one or more targets that depend on '
'native code. Please ensure that the platform arguments in all relevant targets and build '
'options are compatible with the current platform. Found targets for platforms: {}'

This comment has been minimized.

@stuhood

stuhood Mar 28, 2018

Member

The idea here was that we could actually show one or more of the incompatible targets, but let's just save that for a future change.

This comment has been minimized.

@CMLivingston

CMLivingston Mar 28, 2018

Contributor

platforms isn't a great name for this var. Since it is a dict, the output will contain a string repr of keys-> list of targets w/ addresses when this exception is raised. That way you can see what targets do not align w/ current platform. Is this what you were thinking?

This comment has been minimized.

@stuhood

stuhood Mar 28, 2018

Member

Ooh, right. 👍

(although the formatting will be unfortunate)

@stuhood stuhood merged commit 2674072 into pantsbuild:master Mar 28, 2018

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
# Test that pants will override pants.ini platforms config when building
# or running a target that depends on native (c or cpp) sources.
pex = os.path.join(get_buildroot(), 'dist', 'main.pex')
pants_ini_config = {'python-setup': {'platforms': ['current', 'linux-x86_64']}}

This comment has been minimized.

@cosmicexplorer

cosmicexplorer Mar 30, 2018

Contributor

It's not clear to me what this is testing. Is this testing that pants ignores the python-setup platforms (it seems like it), or is Pants supposed to check against these platforms for compatibility with the host platform? Why specifically 'linux-x86_64' and not also 'macosx-10.8-intel'?

wisechengyi added a commit that referenced this pull request Apr 18, 2018

Ensure test goal implicitly targets current platform when using pytho…
…n_dist targets (#5720)

### Problem

#5618 introduced safety checks to ensure that targets depending on `python_dist` targets build for the current platform only. However, test targets do not have a platform parameter, and we need to extend this check to ensure that `./pants test` runs do not inherit incompatible default platform constraints and only build for `'current'` only.

### Solution

Give the `python_dist` target a hard-coded platform constraint so that `python_tests` implicitly targets the current platform when testing APIs from a `python_dist` target.

### Result

Users can now depend on the `python_dist` target in `python_tests` targets when pants.ini specifies multiple platforms.

wisechengyi added a commit that referenced this pull request Apr 18, 2018

Ensure test goal implicitly targets current platform when using pytho…
…n_dist targets (#5720)

### Problem

#5618 introduced safety checks to ensure that targets depending on `python_dist` targets build for the current platform only. However, test targets do not have a platform parameter, and we need to extend this check to ensure that `./pants test` runs do not inherit incompatible default platform constraints and only build for `'current'` only.

### Solution

Give the `python_dist` target a hard-coded platform constraint so that `python_tests` implicitly targets the current platform when testing APIs from a `python_dist` target.

### Result

Users can now depend on the `python_dist` target in `python_tests` targets when pants.ini specifies multiple platforms.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment