Deduplicate Platform and PlatformConstraint
#11157
Conversation
# Building wheels and fs_util will be skipped. Delete if not intended. [ci skip-build-wheels]
# Rust tests and lints will be skipped. Delete if not intended. [ci skip-rust] # Building wheels and fs_util will be skipped. Delete if not intended. [ci skip-build-wheels]
| } | ||
| #[derive(PartialOrd, Ord, Clone, Copy, Debug, Eq, PartialEq, Hash)] | ||
| pub struct PlatformConstraint { | ||
| platform: Option<Platform>, |
There was a problem hiding this comment.
This was baked into PlatformConstraint previously, but it does seem odd to have a single field with optionality baked in here. At use sites this reads "I have a required field ... but that (wrapped) field is optional." It would seem the same could be achieved with no loss of clarity by having the use site just say its an Option doing away with this type altogether. Maybe more knobs were planned for a PlatformConstraint but never materialized?
There was a problem hiding this comment.
I ended up putting the context on the main PR thread rather than here... so yea, can refer to that. IIRC, Platform was a concrete single platform, while PlatformConstraint was meant to select multiple Platforms.
I think that pushing further the PEP425 parallel while working on https://docs.google.com/document/d/1fFjMlQ7fLKsq3bomaoDoNgMxL2zMZPiqk0wjqZX8AFM/edit#heading=h.pquqi3lvc4go might be useful... @jsirois would know better, but my understanding of the PEP425 tags is that when they are partially specified they represent a set of platforms that share the specified portion of the tag. So not a range, per-se... more like a prefix match maybe? So a partially specified tag is like a PlatformConstraint, and a fully specified tag is like a Platform.
There was a problem hiding this comment.
@stuhood that's how things work in spirit, but you cannot actually partially specify a tag, it must be a complete tag. That tag can contain elements that make it looser though like any and none in certain slots. For the python ecosystem, the flecibility is provided by platform sets. The match within a set must be exact, but the set itself contains both exact platform matches and looser ones too. Unfortunately the logic for generating the sets is not codified in any PEP I know and the defacto standard only exists in https://github.com/pypa/packaging/blob/master/packaging/tags.py#L805.
There was a problem hiding this comment.
The idea was to have a concrete type to wrap a value of None to indicate that no platform constraint was applied. The ergonomics of that were supposed to be that None could be confusing by itself as the platform constraint, but in retrospect the class doesn't offer much beside a nicer name around None. I'm fine with the simplification as the intended purpose doesn't seem to have been clear anyway.
|
Off the cuff comment: I have always figured that the intent with these was for But they may not currently be used that way, and it might make sense to simplify them before angling back into to adapt them to how we need them to act. |
There was a problem hiding this comment.
I'm fine with shipping simplifications in service of further improvement, but I would suggest sketching out a longer series of edits that you'd like https://docs.google.com/document/d/1fFjMlQ7fLKsq3bomaoDoNgMxL2zMZPiqk0wjqZX8AFM/edit#heading=h.pquqi3lvc4go to result in (a rough "PR TODO list") before going too much further.
# Building wheels and fs_util will be skipped. Delete if not intended. [ci skip-build-wheels]
Internal only changes: * Rewrite changelog helper to Python and use markdown for changelog ([#11224](#11224)) * Update to nails `0.8.0`. ([#11226](#11226)) * upgrade to Rust v1.48.0 ([#11210](#11210)) * Move Session to its own file. ([#11222](#11222)) * Port InteractiveProcess to Tokio to unblock async interruption ([#11219](#11219)) * Add `./cargo` script to facilitate Rust development ([#11218](#11218)) * Add partition metadata ([#11212](#11212)) * Hotfix unbound `$PY` variable breaking cargo script ([#11209](#11209)) * Remove dataclasses backport ([#11208](#11208)) * Stop using Python 3.6 internally and in CI ([#11175](#11175)) * Update to tokio 0.2.23. ([#11200](#11200)) * Add test address metadata ([#11193](#11193)) * Port more tests from `TestBase` to `RuleRunner` ([#11183](#11183)) * acquire GIL during work unit conversion ([#11186](#11186)) * Add counter metrics for remote execution ([#11155](#11155)) * add metrics to local cache code paths ([#11146](#11146)) * Deprecate the `sources` field for `python_awslambda` ([#11176](#11176)) * Make S3 bucket expiry deletion handling robust. ([#11156](#11156)) * Detect delete markers in our s3 cache of native_engine.so. ([#11140](#11140)) * Fix target globs for deleted file `pants_run_integration_test.py`. ([#11127](#11127)) * Deduplicate `Platform` and `PlatformConstraint` ([#11157](#11157)) * move counter increment into check_action_cache ([#11154](#11154)) * Replace the "base target" concept with "BUILD targets" ([#11136](#11136)) * Re-land the port of Pants' nailgun client to Rust ([#11147](#11147)) * Flatten execution and target PlatformConstraints in MultiPlatformProcess ([#11145](#11145)) * Prepare 2.1.0rc2 ([#11180](#11180)) * Prepare 2.1.0rc1 ([#11144](#11144)) * Prepare 2.0.1rc0 ([#11141](#11141)) * Prepare 2.0.1rc2 ([#11220](#11220)) * Prepare 2.1.1rc0 ([#11221](#11221)) * Prepare 2.1.0 ([#11198](#11198)) * Prepare 2.0.1rc1 ([#11192](#11192)) * Prepare 2.1.0rc3 ([#11191](#11191)) [ci skip-rust]
We were duplicating the valid platform names between these two enums. As we plan to add more Platforms to Pants, this duplication will become more expensive.
We simply delete
PlatformConstraintand replace it withOption<Platform>.[ci skip-build-wheels]