Ensure that changing platforms invalidates pex binary creation #6202
Conversation
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.
| numpy_macos_dep, | ||
| namelist) | ||
|
|
||
| def _open_zip(self, path): |
There was a problem hiding this comment.
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(): | |||
There was a problem hiding this comment.
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) |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
That makes sense to me. I don't think it'd be too much work to update this PR with that.
|
Thanks @baroquebobcat : it would be good to have this in with John's feedback. There is one actual failure in CI. |
| @@ -47,6 +48,7 @@ def subsystem_dependencies(cls): | |||
| return super(PythonNativeCode, cls).subsystem_dependencies() + ( | |||
| NativeToolchain.scoped(cls), | |||
| PythonSetup.scoped(cls), | |||
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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()) | |||
There was a problem hiding this comment.
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): |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Hm. I think I'd want to have a bit more definition around what it's responsibilities are first.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Makes sense. I guess my issue is figuring out what to call it. PexBuildResolution maybe?
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.
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.