-
-
Notifications
You must be signed in to change notification settings - Fork 636
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 new visibility
field to targets to limit target dependees.
#15803
Conversation
Also introduces a new `defaults` target which allows to dynamically provide "scoped" default field values. Closes pantsbuild#13393. [ci skip-rust] [ci skip-build-wheels]
Did you just implement #13767? 😐 |
Oh, yeah this would need some more work, but yes pretty much ;) |
However I feel the defaults may be a more involved topic better suited in a dedicated PR, so am considering limiting this one to just the visibility field and implementation. That leaves it at a sub optimal use, but could serve as a smoother introduction. |
Cool!! I agree it would be better to have this only focus on |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks!
class ValidateVisibilityRulesRequest(ValidateDependenciesRequest): | ||
field_set_type = ValidateVisibilityRulesFieldSet |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Technically ValidateDependenciesRequest
operates on the consuming target, rather than on the producing target. That means that if someone defined a consuming target which didn't have the visibility field, the producer's visibility wouldn't be validated. I'm not really sure how ValidateDependenciesRequest
could be adapted to solve that.
I don't think that that is worth worrying too much about, but maybe worth making a note of here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We could have a check that enforces all Target types to have a visibility field (and all that include the COMMON_TARGET_FIELDS get it).
I think this would be a potential gotcha for plugin authors, which a note here would be of moot help.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is addressed by #15876 so that we can register the visibility field as a plugin field directly on Target
and it will apply to all targets.
src/python/pants/engine/target.py
Outdated
return True | ||
if rule == self.PRIVATE_VISIBILITY: | ||
return to_path.startswith(target_path) | ||
if to_path.startswith(rule): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think that this would need to validate that rule
ends in a slash as well?
I'd suggest switching both of these to
pants/src/python/pants/util/dirutil.py
Lines 44 to 48 in b98649e
def fast_relpath_optional(path: str, start: str) -> str | None: | |
"""A prefix-based relpath, with no normalization or support for returning `..`. | |
Returns None if `start` is not a directory-aware prefix of `path`. | |
""" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Switching the entire logic to instead use Specs
for determining valid targets.
def rules(): | ||
return ( | ||
*collect_rules(), | ||
UnionRule(ValidateDependenciesRequest, ValidateVisibilityRulesRequest), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Given that this is already using a pluggable mechanism, is it worth putting it in its own backend, to avoid the overhead of checking visibility if a repo isn't using it at all?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, I like that.
Awesome! My main open question is what the DSL for users should look like. For example, will there be a need to only allow certain targets to consume another target? Or this should all be filesystem based? If you're willing, I think it would be helpful to either use something like a Google Doc or GitHub discussion so that we can fully scope out the ideal user experience. What do you think? |
Yeah, I considered the possibility to use specs rather than just paths, so you can single out specific targets, or just the targets at one level rather than recursive etc. Seeing the fruitful use of a design doc for the defaults feature, I can see the benefit to use that here as well. |
Start of fleshing out a design doc here: https://docs.google.com/document/d/1DdVZ4DqX7uwiYuCL_ntb17yPxyez1fbiu8K0whZ78Lg/edit# |
Could I use visibility to say "this module should be a leaf" (ie it can be imported by anything but must not import any of the other local modules)? |
Could I use a glob in the path? like |
Could we rename this PR? I think - Add new `visibility` field to targets to limit target dependencies.
+ Add new `visibility` field to targets to limit target dependees. |
visibility
field to targets to limit target dependencies.visibility
field to targets to limit target dependees.
Still todo: move into dedicated backend plugin.
@@ -0,0 +1,85 @@ | |||
# Copyright 2020 Pants project contributors (see CONTRIBUTORS.md). |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
2022? 😂
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Whopsie 😂
Not necessary for this PR, we can file a feature request ticket later. But another interesting visibility use-case is setting visibility to "test". E.g. "only test utilities or tests can depend on this". It helps me make sure no prod code depends on a test utility or a test |
Since the visibility rules now simply are Perhaps something along the lines of:
this would mean that you could run goals on a select subset of target types, like:
|
I swapped out the bespoke visibility check in the explorer backend for this new feature to try it out. Could drop that switch before landing if desired. |
…ckend. # 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]
Also, I think that we'd maybe want to run some sort of benchmark using this to see if the current approach is feasible from a performance perspective. My time will become rather limited soon to pursue this further for some time (month-ish or more) |
That would be -- Sorry I have not reviewed this! Is this still ready for review? |
No worries.
I believe so. Although I'm not setup for dev work on my new machine just yet. |
I have a new angle for this feature. I'll also sync some with @benjyw who've expressed an interest in tackling this feature, and update the design doc going forward. |
A GitHub Discussion linking to the design doc would be great! |
Closes #13393.
Example
Example error message: