Speaking of ^, we (<@U02Q3KHC59B> and I) have been...
# general
l
Speaking of ^, we (@sparse-lifeguard-95737 and I) have been trying out the new visibility feature that has landed recently and have some questions/feedback. 🧵
👀 1
1. Not ideal that its a single error at a time. We regularly write rules which initially trigger dozens of violations. But pants would ony show us one error per run (when they are treated as errors, warnings would log all of them). It would be great to treat the output more like a linter, which wrote all all the violations.
2. It does not appear play nicely with python automatically depending on all `__init__py`'s above it in the tree. This may be solved with documentation. But you can't easily deny everything, since the dependencies on
__init__.py
in the current and parent directory are inferred.
3. Maybe we just couldn't figure this out, but it seems you cannot use a non-file target in any of the rules. Colons in the rules seem to be interpreted literally rather than as part of an identifier for a non-file target, like a docker image or pex.
4. We figured this out eventually, but it was a bit annoying to figure out that we needed to install some default rules at the root of our repo. This may be solved with documentation.
s
putting 3 another way: we couldn’t figure out how to write a dependency/dependent rule matching a single target within a directory
c
1. Easily addressed. Thanks for that feedback.
2. Oh? I’ll have to run some labs on that. Thanks for raising it.
3. Yes, the current design is path based, with the option to filter down on target type but not by target address.
4. Could you please elaborate a bit on this one? Parts of the subtree not using these rules should have an implicit “allow all” rule applied.
Also, are these experiences pre or post https://github.com/pantsbuild/pants/pull/17588 ?
s
post
👍 1
on 4 - the rules we were writing were likely covering more things than we expected. I think the error message could be a bit more verbose to say “you may need to add a “*” catch-all…” etc. we were stumped for a few minutes until we re-read the PR description and noticed the bit about needing a catch-all rule
👍 1
on 3 - at least for us, filtering down by target address is critical to compete with
import-linter
(https://import-linter.readthedocs.io/en/stable/contract_types.html). we’ll typically introduce new rules before our codebase is completely in compliance to “stop the bleeding”, and set all the
ignore_imports
for existing rule violations. if we aren’t able to specify per-target exceptions then we’d need to keep
import-linter
around until we’ve either broken all the bad imports or majorly reorganized our codebase (both of which will take a long while)
👀 1
b
Thank you for sharing feedback on a new feature! We really benefit from early feedback like this. Much appreciated.
1
c
totally agree on error messages needing improvements, they’re very crude at the moment (I have to read them over two or three times myself too to make ends of them)
there are more details logged when running with
-ldebug
though, in case you’ve not seen those yet… 🙂
h
Re 3 - it seems reasonable to interpret
:
as indicating an address, it’s pretty unlikely to have a literal
:
in a path
Most systems don’t support that AFAIK
c
However, note that we’ll only be able to work with addresses for target declared in the BUILD files, i.e. the generators and actual targets, not generated targets as we’re working with
TargetAdaptors
in this implementation…
hmm… I’ll have a think through if maybe I can glean some stuff out any way….
oh yea, I do have the address in question… nvm me, fixing! 😄
(and thanks a million for trying this out so thoroughly early on!!)
OK, a couple of upcoming improvements regarding error messages first:
Copy code
pants.engine.internals.dep_rules.DependencyRuleActionDeniedError: src/python/pants/backend/visibility/rules.py has 2 dependency violations:

  * src/python/pants/backend/explorer/BUILD: '!*' : DENY python_sources src/python/pants/backend/visibility/rules.py -> python_sources src/python/pants/backend/explorer/graphql/setup.py
  * src/python/pants/backend/explorer/BUILD: '!*' : DENY python_sources src/python/pants/backend/visibility/rules.py -> python_sources src/python/pants/backend/explorer/server/uvicorn.py
and
Copy code
pants.backend.visibility.rule_types.BuildFileVisibilityRulesError: There is no matching rule from the `__dependencies_rules__` defined in src/python/pants/engine/internals/BUILD for the `python_tests` target src/python/pants/engine/internals/dep_rules_test.py:tests for the dependency on the `python_test_utils` target //conftest.py:test_utils

Consider adding the required catch-all rule at the end of the rules spec. Example adding a "deny all" at the end:

  (('*',), 'not/matching', '!this/either', '!*')
where the BUILD file has:
Copy code
__dependencies_rules__(("*", "not/matching", "!this/either"),)
PR will likely be coming sometime tomorrow… any suggestions for improving the phrasing of the above, or information to include most welcome 🙂
regarding the
BuildFileVisibilityRulesError
so will it pick out the matching rule spec for the target in question based on the target globs for the rules in the BUILD file for the example in the message 😉
https://pantsbuild.slack.com/archives/C046T6T9U/p1669067240497999?thread_ts=1669067231.802249&amp;cid=C046T6T9U @loud-laptop-17949 I’m guessing these init files are non-empty? And yes, I think having an early
"__init__.py"
rule before any deny rules to allow them could go a long way..
l
most of our init files are empty, the ones we were testing almost certainly were
This might be a case where the docs need to be a big clearer, exp when providing a clean-slate deny-all policy from which you can grant back permitted imports.
c
ok, let’s keep this current. I’m not entirely sure when/why this is a thing (I don’t see it reliably myself) but it certainly shouldn’t be difficult to handle with the rules, and having a section in the upcoming docs for it makes sense as it will be a common thing to tackle I’m sure.