Skip to content
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

Add __defaults__ BUILD file symbol #15836

Merged
merged 23 commits into from
Jun 23, 2022
Merged

Add __defaults__ BUILD file symbol #15836

merged 23 commits into from
Jun 23, 2022

Conversation

kaos
Copy link
Member

@kaos kaos commented Jun 15, 2022

Provide default field values for a set of targets in a particular subtree of a project using the __defaults__ symbol, using the same syntax as for the overrides field.

Example:

# BUILD file
__defaults__(
  {
    pyhon_sources: dict(sources=["*.py"]),
    (python_tests, python_testutils): dict(tags=["some-tag"]),
  },
  all=dict(description="Copyright (c) 2022, Company Ltd."),
)

...

The __defaults__ declaration should go at the top of the BUILD file, in order to apply to the targets in the same file. All BUILD files in subdirectories inherit the same defaults by default, but can be overridden/extended with their own __defaults__.

Closes #13767.
Discussion: #15809.

kaos added 4 commits June 15, 2022 08:23
[ci skip-rust]

[ci skip-build-wheels]
[ci skip-rust]

[ci skip-build-wheels]
[ci skip-rust]

[ci skip-build-wheels]
[ci skip-rust]

[ci skip-build-wheels]
@kaos
Copy link
Member Author

kaos commented Jun 15, 2022

This seems to work!

I've put this up here for some early cursory remarks/feedback before jumping on writing a lot of tests.

I'm currently holding my fingers tightly (very tightly) crossed, that we always parse all parent BUILD files, and not jump straight into the project subtree for some operations.. (seems to be the case but haven't verified)

kaos added 3 commits June 15, 2022 17:55
[ci skip-rust]

[ci skip-build-wheels]
 * Add `__str__` to parser `Registrar` rather than interhit from `str`.
 * Allow either `Registrar` or `str` as keys for the `__defaults__` dictionary.
@kaos
Copy link
Member Author

kaos commented Jun 17, 2022

OK, this is ready for review. I'm still slightly hazy on how to best ensure that the BUILD files are always parsed in an unbroken sequence. But from my code review, it seems that it might only be a TargetAdaptorRequest that has the potential to do that, and currently the rules requesting it does so in a proper way to ensure a consistent line of BUILD files being parsed properly.

There are a few open TODO questions in defaults.py, I'm planning on addressing the one regarding valid target field aliases to avoid recalculating them so frequently.

[ci skip-build-wheels]
Copy link
Member

@stuhood stuhood left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks! One bug, but other than that it looks good.

src/python/pants/engine/internals/build_files.py Outdated Show resolved Hide resolved
src/python/pants/engine/internals/build_files.py Outdated Show resolved Hide resolved
src/python/pants/engine/internals/defaults.py Outdated Show resolved Hide resolved
src/python/pants/engine/internals/defaults.py Show resolved Hide resolved
src/python/pants/engine/internals/defaults.py Outdated Show resolved Hide resolved
src/python/pants/engine/internals/parser.py Show resolved Hide resolved
kaos added 5 commits June 22, 2022 12:26
[ci skip-build-wheels]
[ci skip-build-wheels]
[ci skip-build-wheels]
Copy link
Member

@stuhood stuhood left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks! This is really great.

src/python/pants/engine/internals/defaults.py Show resolved Hide resolved
src/python/pants/engine/internals/defaults.py Outdated Show resolved Hide resolved
src/python/pants/engine/internals/defaults.py Outdated Show resolved Hide resolved
src/python/pants/engine/internals/defaults.py Outdated Show resolved Hide resolved
src/python/pants/engine/internals/defaults.py Outdated Show resolved Hide resolved
f"Valid fields are: {', '.join(sorted(valid_field_aliases))}.",
)
# TODO: moved fields for TargetGenerators ? See: `Target._calculate_field_values()`.
# TODO: support parametrization ?
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think that parametrization should "just work", because the parametrize object is preserved, and not expanded until the target is actually constructed.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think the issue here would be that we do eager validation of the default values when freezing them.. so we would be passing a Parametrize object to the field.compute_value.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Even so, we can avoid this validation, but get stuck on sticking the Parametrize instance in a FrozenDict does not work well:

E           TypeError: Even though you are using a `FrozenDict`, the underlying values are not hashable. Please use hashable (and preferably immutable) types for the underlying values, e.g. use tuples instead of lists and use FrozenOrderedSet instead of set().
E           
E           Original error message: unhashable type: 'dict'
E           
E           Value: FrozenDict({'description': parametrize(a=desc A, b=desc B})

src/python/pants/engine/internals/specs_rules.py Outdated Show resolved Hide resolved
Copy link
Member

@cognifloyd cognifloyd left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A couple of notes about the docs. This is exciting!

docs/markdown/Using Pants/concepts/targets.md Outdated Show resolved Hide resolved
docs/markdown/Using Pants/concepts/targets.md Show resolved Hide resolved
[ci skip-rust]

[ci skip-build-wheels]
@kaos kaos enabled auto-merge (squash) June 23, 2022 16:38
@kaos kaos merged commit bc3661f into pantsbuild:main Jun 23, 2022
@kaos kaos deleted the defaults branch June 23, 2022 18:59
kaos added a commit that referenced this pull request Jul 10, 2022
…f BUILD files. (#16089)

Fix bug introduced in #15836 

As reported by @ced-f on Slack:
> I have a top level `django/BUILD` file which contains:
> ```python
> __defaults__({
>     (python_tests): dict(dependencies=["django/django_core/settings.py"])
> })
> ```
> And a BUILD file under `django/sub/tests/classes/BUILD` which just defines:
> ```python
> python_tests(
>     name="tests",
> )
> ```
> The dependencies goal in the subdirectory does not reference `django/django_core/settings.py`
> If I extend the contents of `django/sub/tests/classes/BUILD` the dependency `django/django_core/settings.py` gets shown correctly:
> ```python
> __defaults__({
>     (python_tests): dict(dependencies=["django/django_core/settings.py"])
> })
> python_tests(
>     name="tests",
> )
> ```
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Consider adding explicit support for applying build metadata to multiple targets at once
3 participants