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

Ensure that changing platforms invalidates pex binary creation #6202

Merged
merged 8 commits into from Oct 26, 2018

Conversation

Projects
None yet
3 participants
@baroquebobcat
Contributor

baroquebobcat commented Jul 19, 2018

Currently, the platforms used to build pexes are not fingerprinted. This means that running ./pants binary some/python:bin twice with different platform settings will not recreate the pex on the second run. Instead it will retain the platform settings of the first run. This forces clean-alls when changing platform flags.

This patch causes the platforms flag to be fingerprinted and adds its owning subsystem to python binary create's subsystem dependencies.

Ensure that changing platforms invalidates pex binary creation
Currently, the platforms used to build pexes are not fingerprinted. This means that running ./pants binary some/python:bin twice with different platform settings will not recreate the pex on the second run. Instead it will retain the platform settings of the first run. This forces clean-alls when changing platform flags.

This patch causes the platforms flag to be fingerprinted and adds its owning subsystem to python binary create's subsystem dependencies.
@jsirois

It also looks like --interpreter-constraints in PythonSetup is improperly not fingerprinted. Lots broken here and I realize you may want to limit scope in the PR. I noted the broken I spotted though anyway.

numpy_macos_dep,
namelist)
def _open_zip(self, path):

This comment has been minimized.

@jsirois

jsirois Jul 19, 2018

Member

You can use pants.util.contextutil.open_zip.

@@ -127,6 +127,15 @@ def dump_requirements(builder, interpreter, reqs, log, platforms=None):
locations.add(dist.location)
def subsystems_used():

This comment has been minimized.

@jsirois

jsirois Jul 19, 2018

Member

This hits 4 tasks and 1 subsystem in addition to python_binary_create.py. I'd be happy with all of these being fixed in one PR but would also be happy with an issue to track the wider fix.

This function is provided so that consuming tasks can call it in their
subsystem_dependencies implementations.
"""
return (PythonRepos, PythonSetup)

This comment has been minimized.

@jsirois

jsirois Jul 19, 2018

Member

This also points wrapping the utlity functions that need these into a class whose constructor requires PythonRepos & PythonSetup instances be passed in with no defaulting. That would prevent the problem going forward.

This comment has been minimized.

@baroquebobcat

baroquebobcat Jul 20, 2018

Contributor

That makes sense to me. I don't think it'd be too much work to update this PR with that.

@jsirois jsirois referenced this pull request Jul 30, 2018

Merged

Upgrade to pex 1.4.5. #6267

@stuhood

This comment has been minimized.

Member

stuhood commented Jul 31, 2018

Thanks @baroquebobcat : it would be good to have this in with John's feedback.

There is one actual failure in CI.

@jsirois

LGTM mod red CI

@@ -47,6 +48,7 @@ def subsystem_dependencies(cls):
return super(PythonNativeCode, cls).subsystem_dependencies() + (
NativeToolchain.scoped(cls),
PythonSetup.scoped(cls),

This comment has been minimized.

@jsirois

jsirois Aug 22, 2018

Member

This highlight what looks like an already broken scoped here vs global in dump requirements. Not your issue but @cosmicexplorer and @CMLivingston may want to sort why scoped and why not consistent use.

This comment has been minimized.

@baroquebobcat

baroquebobcat Aug 22, 2018

Contributor

Hm. I was wondering about that too.

@@ -55,7 +55,8 @@ def create_requirements(cls, context, workdir):
def build_isort_pex(cls, context, interpreter, pex_path, requirements_lib):
with safe_concurrent_creation(pex_path) as chroot:
builder = PEXBuilder(path=chroot, interpreter=interpreter)
dump_requirement_libs(builder=builder,
pex_build_util = PexBuildUtil(PythonRepos.global_instance(), PythonSetup.global_instance())

This comment has been minimized.

@jsirois

jsirois Aug 22, 2018

Member

The IsortPrep.subsystem_dependencies override should be moved up here into IsortPrep.Isort.Factory.

return pex_build_util.resolve_multi(interpreter, requirements, platforms, find_links)
class PexBuildUtil(object):

This comment has been minimized.

@jsirois

jsirois Aug 22, 2018

Member

This may have better SOC as a subsystem that declares its own dependencies on PythonSetup and PythonRepos, but I'm happy enough with this PR's step forward. As you see fit.

This comment has been minimized.

@baroquebobcat

baroquebobcat Aug 22, 2018

Contributor

Hm. I think I'd want to have a bit more definition around what it's responsibilities are first.

This comment has been minimized.

@jsirois

jsirois Aug 22, 2018

Member

The responsibilities don't change, being a subsystem just opts in to ~dependency injection and keeps users from having to know the util's details.

This comment has been minimized.

@baroquebobcat

baroquebobcat Aug 22, 2018

Contributor

Makes sense. I guess my issue is figuring out what to call it. PexBuildResolution maybe?

@stuhood stuhood merged commit a368267 into pantsbuild:master Oct 26, 2018

1 check passed

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

@stuhood stuhood deleted the twitter:nhoward/fingerprint_platform_for_pexes branch Oct 26, 2018

stuhood added a commit that referenced this pull request Nov 1, 2018

Ensure that changing platforms invalidates pex binary creation (#6202)
Currently, the platforms used to build pexes are not fingerprinted. This means that running ./pants binary some/python:bin twice with different platform settings will not recreate the pex on the second run. Instead it will retain the platform settings of the first run. This forces clean-alls when changing platform flags.

This patch causes the platforms flag to be fingerprinted and adds its owning subsystem to python binary create's subsystem dependencies.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment