Documentation for the new visibility rules

[kaos]

Initial visibility docs draft chapter.

[kaos]

Any way to push this to readme for preview?

Or are there alternatives to get a peek at what this looks like?

[kaos]

I'm in favour of dropping the remaining use of fnmatch to limit the initial glob syntax supported. (see the implementation note in the previous paragraph for primary motivation for this)

(implementation detail: it relies on the fnmatch python library so there is a bit more syntax available, but if you find yourself using more than * please let us know in case we switch to something else).

[kaos]

I tagged "everyone" as reviewer on this PR as I think everyone has potential to contribute. If it's not your area of interest/expertise you may very well be the expert at finding passages that are difficult to understand etc. ;) If you're so inclined 🙏🏽

[AlexTereshenkov]

Excellent work @kaos! Here are the notes from our experimentations:

---

PANTS_SHA=d47aecab6764c6c4ca3f80f3bc98d0985af399e7 ./pants peek ::

It is only when you declare rules, for either direction at either side of the relationship link, that it needs to be comprehensive. So within a single __dep*_rules__ , you aren’t required to have both. If there are __dependencies_rules__, but there are no __dependents_rules__ rules, it’s an implicit "allow all".

The dependencies are validated from the origin targets only: if a module appA/run.py imports a module appB/utils.py (that it's not supposed to import), you have to include the source of the dependency (appA/run.py) during the validation. For example, running ./pants peek appB:: won't show an error because it's not fault of appB/utils.py that it's imported by a module that is not supposed to do that.

To make it easier to distinguish between dependencies and dependents, you can think in this way:

• Dependencies rules: “this may only import …”
• Dependents rules: “this may only be imported from ...”

In this section:

. - Anchor the glob to the invocation path. This is the file path of the target for which the rule will apply for. Relative paths are supported, so ../../cousin/path is valid. Example: there is a rule ./lib/* when applied for the file src/python/proj/main.py it would match src/python/proj/lib/*.

“When applied for x” means, when we look up what rules we should consult for x. So, say we have a src/python/proj/BUILD with some rules in it, then when the time comes to see if main.py may import baz.py there will be a set of rules that “applies to” main.py which will dictate whether to deny or allow the dependency on baz.py.

The ./lib/* rule is about anchoring. The . is anchored to the directory where the rule “is applied”. When the rule is “used for” (or applied for, to stick with the term) /src/python/proj/main.py, the path where it is being applied is src/python/proj. Join those two pieces together and you get: src/python/proj + ./lib/* -> src/pyhon/proj/./lib/* = src/python/proj/lib/*. So if src/python/proj/main.py depends on anything in src/python/proj/lib/, then the rule ./lib/* would allow it.

Keep in mind that currently having multiple __dependents_rules__ in the same BUILD file is allowed. However, the last one declared will be used and all the previously declared ones will be ignored. This is different to the behavior you'd see having multiple __defaults__ in your BUILD files. To avoid confusion, make sure to include all of your rulesets under a single declaration:

___dependents_rules__(
  (# who can import `python_test_utils` in this directory
    python_test_utils,
    "...",
    "!*",
  ),

  # who can import `python_sources` in this directory
  (
    python_tests,
    "more rules",
    ...
  ),


  # More rule sets
  (
    (files, resources),
    "res/**",
  ),
  ...
)

It is under consideration whether multiple __dep*__ rules should be merged in case having multiple declarations is justified (e.g. splitting source dependencies and tests dependencies) from the readability perspective.

To check for any violations of your visibility rules, you can run ./pants peek :: >/dev/null command which would evaluate all BUILD files in your repository and report any violations.

[jake-sigtech]

Excellent work @kaos !
I have been following this feature with interest and have tried it out today for the first time - looks great.

Would it be possible to add an example to the docs about how to pattern match third-party dependencies.
For example: //:requirements#pandas. This has confused me and I can't figure it out!

[AlexTereshenkov]

Excellent work @kaos ! I have been following this feature with interest and have tried it out today for the first time - looks great.

Would it be possible to add an example to the docs about how to pattern match third-party dependencies. For example: //:requirements#pandas. This has confused me and I can't figure it out!

Thanks for this @jake-sigtech. FWIW If I am not mistaken, you can also use a wildcard to permit all requirements, that's the syntax:

__dependencies_rules__(
  (
    "*",  # applies to everything in this BUILD file
    "/**",  # allow all dependencies in this subtree
    "//requirements#*",
    "//cheeseshop/cli/**",  # may not depend on cheeseshop/cli
    "*"
  )
)

where the requirements is the name of your python_requirements target (default one, IIRC).

See an example here.

[jake-sigtech]

See an example here.

It only works for me if I use a glob. Raised bug here: #17759

[Eric-Arellano]

(didn't finish review)

[Eric-Arellano]

I think this repeats the paragraph above - wdyt?

[kaos]

It does. I think of it like a news paper excerpt, something we want to draw a little extra attention too. And if it tickles your fancy, read the surrounding text for more details and in depth content.

[kaos]

Bar spelling/grammar/major issues I think we may want to merge this to get some base line for further updates, as it's becoming a rather large body of text it's not so pleasant to review.

I've tried to incorporate as much as possible of the feedback provided here thus far. Thank you all for those!