From 46b2c9c8a58f39eeae393eaa95c6616b85d72c5b Mon Sep 17 00:00:00 2001 From: Eric Arellano <14852634+Eric-Arellano@users.noreply.github.com> Date: Tue, 28 Apr 2020 10:30:24 -0700 Subject: [PATCH] Allow `HydratedSourcesRequest` to indicate which Sources types are expected (#9641) Soon, we will add codegen. With this, we need a way to signal which language should be generated, if any. @stuhood proposed in https://github.com/pantsbuild/pants/pull/9634#discussion_r416121567 that we can extend this setting to indicate more generally which Sources fields are valid, e.g. that we expect to work with `PythonSources` and `FilesSources` (or subclasses), but nothing else. All invalid fields would return an empty snapshot and indicate via a new `HydratedSources.output_type` field that it was an invalid sources field. This means that call sites can still pre-filter sources fields like they typically do via `tgt.has_field()` (and configurations), but they can also use this new sugar. If they want to use codegen in the upcoming PR, they must use this new mechanism. Further, when getting back the `HydratedSources`, call sites can switch on the type. Previously, they could do this by zipping the original `Sources` with the resulting `HydratedSources`, but this won't work once we have codegen, as the original `Sources` will be, for example, `ThriftSources`. ```python if hydrated_sources.output_type == PythonSources: ... elif hydrated_sources.output_type == FilesSources: ... ``` [ci skip-rust-tests] [ci skip-jvm-tests] --- .../python/rules/importable_python_sources.py | 11 +- .../backend/python/rules/run_setup_py.py | 10 +- .../core/util_rules/determine_source_files.py | 37 +++- .../core/util_rules/strip_source_roots.py | 24 ++- src/python/pants/engine/target.py | 196 +++++++++++------- src/python/pants/engine/target_test.py | 25 ++- 6 files changed, 204 insertions(+), 99 deletions(-) diff --git a/src/python/pants/backend/python/rules/importable_python_sources.py b/src/python/pants/backend/python/rules/importable_python_sources.py index cf221e7623b..bed7665bf6c 100644 --- a/src/python/pants/backend/python/rules/importable_python_sources.py +++ b/src/python/pants/backend/python/rules/importable_python_sources.py @@ -12,7 +12,7 @@ from pants.engine.fs import Snapshot from pants.engine.rules import RootRule, rule from pants.engine.selectors import Get -from pants.engine.target import Sources, Target, Targets +from pants.engine.target import Sources, Targets @dataclass(frozen=True) @@ -33,14 +33,11 @@ class ImportablePythonSources: @rule async def prepare_python_sources(targets: Targets) -> ImportablePythonSources: - def is_relevant(tgt: Target) -> bool: - return any( - tgt.has_field(field) for field in (PythonSources, ResourcesSources, FilesSources) - ) - stripped_sources = await Get[SourceFiles]( AllSourceFilesRequest( - (tgt.get(Sources) for tgt in targets if is_relevant(tgt)), strip_source_roots=True + (tgt.get(Sources) for tgt in targets), + for_sources_types=(PythonSources, ResourcesSources, FilesSources), + strip_source_roots=True, ) ) init_injected = await Get[InitInjectedSnapshot](InjectInitRequest(stripped_sources.snapshot)) diff --git a/src/python/pants/backend/python/rules/run_setup_py.py b/src/python/pants/backend/python/rules/run_setup_py.py index d810152b31a..f9d16446993 100644 --- a/src/python/pants/backend/python/rules/run_setup_py.py +++ b/src/python/pants/backend/python/rules/run_setup_py.py @@ -472,7 +472,11 @@ async def get_sources( ) -> SetupPySources: targets = request.targets stripped_srcs_list = await MultiGet( - Get[SourceRootStrippedSources](StripSourcesFieldRequest(target.get(Sources))) + Get[SourceRootStrippedSources]( + StripSourcesFieldRequest( + target.get(Sources), for_sources_types=(PythonSources, ResourcesSources) + ) + ) for target in targets ) @@ -518,7 +522,9 @@ async def get_ancestor_init_py( """ source_roots = source_root_config.get_source_roots() sources = await Get[SourceFiles]( - AllSourceFilesRequest(tgt[PythonSources] for tgt in targets if tgt.has_field(PythonSources)) + AllSourceFilesRequest( + (tgt.get(Sources) for tgt in targets), for_sources_types=(PythonSources,) + ) ) # Find the ancestors of all dirs containing .py files, including those dirs themselves. source_dir_ancestors: Set[Tuple[str, str]] = set() # Items are (src_root, path incl. src_root). diff --git a/src/python/pants/core/util_rules/determine_source_files.py b/src/python/pants/core/util_rules/determine_source_files.py index a78985c3b1b..403d1bcef7a 100644 --- a/src/python/pants/core/util_rules/determine_source_files.py +++ b/src/python/pants/core/util_rules/determine_source_files.py @@ -2,7 +2,7 @@ # Licensed under the Apache License, Version 2.0 (see LICENSE). from dataclasses import dataclass -from typing import Iterable, Tuple, Union +from typing import Iterable, Tuple, Type, Union from pants.base.specs import AddressSpec, OriginSpec from pants.core.util_rules import strip_source_roots @@ -35,12 +35,18 @@ def files(self) -> Tuple[str, ...]: @dataclass(unsafe_hash=True) class AllSourceFilesRequest: sources_fields: Tuple[SourcesField, ...] - strip_source_roots: bool = False + for_sources_types: Tuple[Type[SourcesField], ...] + strip_source_roots: bool def __init__( - self, sources_fields: Iterable[SourcesField], *, strip_source_roots: bool = False + self, + sources_fields: Iterable[SourcesField], + *, + for_sources_types: Iterable[Type[SourcesField]] = (SourcesField,), + strip_source_roots: bool = False ) -> None: self.sources_fields = tuple(sources_fields) + self.for_sources_types = tuple(for_sources_types) self.strip_source_roots = strip_source_roots @@ -48,15 +54,18 @@ def __init__( @dataclass(unsafe_hash=True) class SpecifiedSourceFilesRequest: sources_fields_with_origins: Tuple[Tuple[SourcesField, OriginSpec], ...] - strip_source_roots: bool = False + for_sources_types: Tuple[Type[SourcesField], ...] + strip_source_roots: bool def __init__( self, sources_fields_with_origins: Iterable[Tuple[SourcesField, OriginSpec]], *, + for_sources_types: Iterable[Type[SourcesField]] = (SourcesField,), strip_source_roots: bool = False ) -> None: self.sources_fields_with_origins = tuple(sources_fields_with_origins) + self.for_sources_types = tuple(for_sources_types) self.strip_source_roots = strip_source_roots @@ -81,7 +90,9 @@ async def determine_all_source_files(request: AllSourceFilesRequest) -> SourceFi """Merge all `Sources` fields into one Snapshot.""" if request.strip_source_roots: stripped_snapshots = await MultiGet( - Get[SourceRootStrippedSources](StripSourcesFieldRequest(sources_field)) + Get[SourceRootStrippedSources]( + StripSourcesFieldRequest(sources_field, for_sources_types=request.for_sources_types) + ) for sources_field in request.sources_fields ) digests_to_merge = tuple( @@ -89,7 +100,9 @@ async def determine_all_source_files(request: AllSourceFilesRequest) -> SourceFi ) else: all_hydrated_sources = await MultiGet( - Get[HydratedSources](HydrateSourcesRequest(sources_field)) + Get[HydratedSources]( + HydrateSourcesRequest(sources_field, for_sources_types=request.for_sources_types) + ) for sources_field in request.sources_fields ) digests_to_merge = tuple( @@ -104,7 +117,11 @@ async def determine_specified_source_files(request: SpecifiedSourceFilesRequest) """Determine the specified `sources` for targets, possibly finding a subset of the original `sources` fields if the user supplied file arguments.""" all_hydrated_sources = await MultiGet( - Get[HydratedSources](HydrateSourcesRequest(sources_field_with_origin[0])) + Get[HydratedSources]( + HydrateSourcesRequest( + sources_field_with_origin[0], for_sources_types=request.for_sources_types + ) + ) for sources_field_with_origin in request.sources_fields_with_origins ) @@ -133,7 +150,11 @@ async def determine_specified_source_files(request: SpecifiedSourceFilesRequest) all_sources_fields = (*full_snapshots.keys(), *snapshot_subset_requests.keys()) stripped_snapshots = await MultiGet( Get[SourceRootStrippedSources]( - StripSourcesFieldRequest(sources_field, specified_files_snapshot=snapshot) + StripSourcesFieldRequest( + sources_field, + specified_files_snapshot=snapshot, + for_sources_types=request.for_sources_types, + ) ) for sources_field, snapshot in zip(all_sources_fields, all_snapshots) ) diff --git a/src/python/pants/core/util_rules/strip_source_roots.py b/src/python/pants/core/util_rules/strip_source_roots.py index 2661079d7b4..a490efd6c20 100644 --- a/src/python/pants/core/util_rules/strip_source_roots.py +++ b/src/python/pants/core/util_rules/strip_source_roots.py @@ -4,7 +4,7 @@ import itertools from dataclasses import dataclass from pathlib import PurePath -from typing import Optional, cast +from typing import Iterable, Optional, Tuple, Type, cast from pants.core.target_types import FilesSources from pants.engine.addresses import Address @@ -23,6 +23,7 @@ from pants.engine.target import Sources as SourcesField from pants.engine.target import rules as target_rules from pants.source.source_root import NoSourceRootError, SourceRootConfig +from pants.util.meta import frozen_after_init @dataclass(frozen=True) @@ -46,7 +47,8 @@ class StripSnapshotRequest: representative_path: Optional[str] = None -@dataclass(frozen=True) +@frozen_after_init +@dataclass(unsafe_hash=True) class StripSourcesFieldRequest: """A request to strip source roots for every file in a `Sources` field. @@ -56,8 +58,20 @@ class StripSourcesFieldRequest: """ sources_field: SourcesField + for_sources_types: Tuple[Type[SourcesField], ...] = (SourcesField,) specified_files_snapshot: Optional[Snapshot] = None + def __init__( + self, + sources_field: SourcesField, + *, + for_sources_types: Iterable[Type[SourcesField]] = (SourcesField,), + specified_files_snapshot: Optional[Snapshot] = None, + ) -> None: + self.sources_field = sources_field + self.for_sources_types = tuple(for_sources_types) + self.specified_files_snapshot = specified_files_snapshot + @rule async def strip_source_roots_from_snapshot( @@ -129,7 +143,11 @@ async def strip_source_roots_from_sources_field( if request.specified_files_snapshot is not None: sources_snapshot = request.specified_files_snapshot else: - hydrated_sources = await Get[HydratedSources](HydrateSourcesRequest(request.sources_field)) + hydrated_sources = await Get[HydratedSources]( + HydrateSourcesRequest( + request.sources_field, for_sources_types=request.for_sources_types + ) + ) sources_snapshot = hydrated_sources.snapshot if not sources_snapshot.files: diff --git a/src/python/pants/engine/target.py b/src/python/pants/engine/target.py index 2fe26e12029..6fec652edb8 100644 --- a/src/python/pants/engine/target.py +++ b/src/python/pants/engine/target.py @@ -1162,83 +1162,10 @@ def compute_value( # ----------------------------------------------------------------------------------------------- -# Common Fields used across most targets +# Sources # ----------------------------------------------------------------------------------------------- -class Tags(StringSequenceField): - """Arbitrary strings that you can use to describe a target. - - For example, you may tag some test targets with 'integration_test' so that you could run - `./pants --tags='integration_test' test ::` to only run on targets with that tag. - """ - - alias = "tags" - - -class DescriptionField(StringField): - """A human-readable description of the target. - - Use `./pants list --documented ::` to see all targets with descriptions. - """ - - alias = "description" - - -# TODO(#9388): remove? We don't want this in V2, but maybe keep it for V1. -class NoCacheField(BoolField): - """If True, don't store results for this target in the V1 cache.""" - - alias = "no_cache" - default = False - v1_only = True - - -# TODO(#9388): remove? -class ScopeField(StringField): - """A V1-only field for the scope of the target, which is used by the JVM to determine the - target's inclusion in the class path. - - See `pants.build_graph.target_scopes.Scopes`. - """ - - alias = "scope" - v1_only = True - - -# TODO(#9388): Remove. -class IntransitiveField(BoolField): - alias = "_transitive" - default = False - v1_only = True - - -COMMON_TARGET_FIELDS = (Tags, DescriptionField, NoCacheField, ScopeField, IntransitiveField) - - -# NB: To hydrate the dependencies into Targets, use -# `await Get[Targets](Addresses(tgt[Dependencies].value)`. -class Dependencies(PrimitiveField): - """Addresses to other targets that this target depends on, e.g. `['src/python/project:lib']`.""" - - alias = "dependencies" - value: Optional[Tuple[Address, ...]] - default = None - - # NB: The type hint for `raw_value` is a lie. While we do expect end-users to use - # Iterable[str], the Struct and Addressable code will have already converted those strings - # into a List[Address]. But, that's an implementation detail and we don't want our - # documentation, which is auto-generated from these type hints, to leak that. - @classmethod - def compute_value( - cls, raw_value: Optional[Iterable[str]], *, address: Address - ) -> Optional[Tuple[Address, ...]]: - value_or_default = super().compute_value(raw_value, address=address) - if value_or_default is None: - return None - return tuple(sorted(value_or_default)) - - class Sources(AsyncField): """A list of files and globs that belong to this target. @@ -1343,15 +1270,39 @@ def filespec(self) -> Filespec: ) -@dataclass(frozen=True) +@frozen_after_init +@dataclass(unsafe_hash=True) class HydrateSourcesRequest: field: Sources + for_sources_types: Tuple[Type[Sources], ...] + + def __init__( + self, field: Sources, *, for_sources_types: Iterable[Type[Sources]] = (Sources,) + ) -> None: + """Convert raw sources globs into an instance of HydratedSources. + + If you only want to handle certain Sources fields, such as only PythonSources, set + `for_sources_types`. Any invalid sources will return a `HydratedSources` instance with an + empty snapshot and `output_type = None`. + """ + self.field = field + self.for_sources_types = tuple(for_sources_types) @dataclass(frozen=True) class HydratedSources: + """The result of hydrating a SourcesField. + + The `output_type` will indicate which of the `HydrateSourcesRequest.valid_sources_type` the + result corresponds to, e.g. if the result comes from `FilesSources` vs. `PythonSources`. If this + value is None, then the input `Sources` field was not one of the expected types. This property + allows for switching on the result, e.g. handling hydrated files() sources differently than + hydrated Python sources. + """ + snapshot: Snapshot filespec: Filespec + output_type: Optional[Type[Sources]] def eager_fileset_with_spec(self, *, address: Address) -> EagerFilesetWithSpec: return EagerFilesetWithSpec(address.spec_path, self.filespec, self.snapshot) @@ -1362,10 +1313,21 @@ async def hydrate_sources( request: HydrateSourcesRequest, glob_match_error_behavior: GlobMatchErrorBehavior ) -> HydratedSources: sources_field = request.field - globs = sources_field.sanitized_raw_value + output_type = next( + ( + valid_type + for valid_type in request.for_sources_types + if isinstance(sources_field, valid_type) + ), + None, + ) + if output_type is None: + return HydratedSources(EMPTY_SNAPSHOT, sources_field.filespec, output_type=None) + + globs = sources_field.sanitized_raw_value if globs is None: - return HydratedSources(EMPTY_SNAPSHOT, sources_field.filespec) + return HydratedSources(EMPTY_SNAPSHOT, sources_field.filespec, output_type=output_type) conjunction = ( GlobExpansionConjunction.all_match @@ -1387,7 +1349,85 @@ async def hydrate_sources( ) ) sources_field.validate_snapshot(snapshot) - return HydratedSources(snapshot, sources_field.filespec) + return HydratedSources(snapshot, sources_field.filespec, output_type=output_type) + + +# ----------------------------------------------------------------------------------------------- +# Other common Fields used across most targets +# ----------------------------------------------------------------------------------------------- + + +class Tags(StringSequenceField): + """Arbitrary strings that you can use to describe a target. + + For example, you may tag some test targets with 'integration_test' so that you could run + `./pants --tags='integration_test' test ::` to only run on targets with that tag. + """ + + alias = "tags" + + +class DescriptionField(StringField): + """A human-readable description of the target. + + Use `./pants list --documented ::` to see all targets with descriptions. + """ + + alias = "description" + + +# TODO(#9388): remove? We don't want this in V2, but maybe keep it for V1. +class NoCacheField(BoolField): + """If True, don't store results for this target in the V1 cache.""" + + alias = "no_cache" + default = False + v1_only = True + + +# TODO(#9388): remove? +class ScopeField(StringField): + """A V1-only field for the scope of the target, which is used by the JVM to determine the + target's inclusion in the class path. + + See `pants.build_graph.target_scopes.Scopes`. + """ + + alias = "scope" + v1_only = True + + +# TODO(#9388): Remove. +class IntransitiveField(BoolField): + alias = "_transitive" + default = False + v1_only = True + + +COMMON_TARGET_FIELDS = (Tags, DescriptionField, NoCacheField, ScopeField, IntransitiveField) + + +# NB: To hydrate the dependencies into Targets, use +# `await Get[Targets](Addresses(tgt[Dependencies].value)`. +class Dependencies(PrimitiveField): + """Addresses to other targets that this target depends on, e.g. `['src/python/project:lib']`.""" + + alias = "dependencies" + value: Optional[Tuple[Address, ...]] + default = None + + # NB: The type hint for `raw_value` is a lie. While we do expect end-users to use + # Iterable[str], the Struct and Addressable code will have already converted those strings + # into a List[Address]. But, that's an implementation detail and we don't want our + # documentation, which is auto-generated from these type hints, to leak that. + @classmethod + def compute_value( + cls, raw_value: Optional[Iterable[str]], *, address: Address + ) -> Optional[Tuple[Address, ...]]: + value_or_default = super().compute_value(raw_value, address=address) + if value_or_default is None: + return None + return tuple(sorted(value_or_default)) # TODO: figure out what support looks like for this with the Target API. The expected value is an diff --git a/src/python/pants/engine/target_test.py b/src/python/pants/engine/target_test.py index c2607a798dc..a7af86753cb 100644 --- a/src/python/pants/engine/target_test.py +++ b/src/python/pants/engine/target_test.py @@ -751,7 +751,7 @@ def assert_invalid_type(raw_value: Any) -> None: # ----------------------------------------------------------------------------------------------- -# Test common fields +# Test Sources # ----------------------------------------------------------------------------------------------- @@ -791,6 +791,29 @@ def test_normal_hydration(self) -> None: "exclude": [{"globs": ["src/fortran/**/ignore*", "src/fortran/ignored.f03"]}], } + def test_output_type(self) -> None: + class SourcesSubclass(Sources): + pass + + addr = Address.parse(":lib") + self.create_files("", files=["f1.f95"]) + + valid_sources = SourcesSubclass(["*"], address=addr) + hydrated_valid_sources = self.request_single_product( + HydratedSources, + HydrateSourcesRequest(valid_sources, for_sources_types=[SourcesSubclass]), + ) + assert hydrated_valid_sources.snapshot.files == ("f1.f95",) + assert hydrated_valid_sources.output_type == SourcesSubclass + + invalid_sources = Sources(["*"], address=addr) + hydrated_invalid_sources = self.request_single_product( + HydratedSources, + HydrateSourcesRequest(invalid_sources, for_sources_types=[SourcesSubclass]), + ) + assert hydrated_invalid_sources.snapshot.files == () + assert hydrated_invalid_sources.output_type is None + def test_unmatched_globs(self) -> None: self.create_files("", files=["f1.f95"]) sources = Sources(["non_existent.f95"], address=Address.parse(":lib"))