Skip to content

Commit

Permalink
Dep rules selection bug (pantsbuild#17623)
Browse files Browse the repository at this point in the history
  • Loading branch information
@kaos
kaos authored Nov 23, 2022
1 parent 8b27c08 commit c3d5b03
Show file tree
Hide file tree
Showing 4 changed files with 91 additions and 38 deletions.
8 changes: 6 additions & 2 deletions src/python/pants/backend/visibility/glob.py
View file
Original file line number Diff line number Diff line change
Expand Up @@ -28,17 +28,21 @@ class PathGlob:
raw: str
anchor_mode: PathGlobAnchorMode = field(compare=False)
glob: Pattern = field(compare=False)
uplvl: int

@classmethod
def parse(cls, pattern: str, base: str) -> PathGlob:
anchor_mode = PathGlobAnchorMode.parse(pattern)
glob = os.path.normpath(pattern).lstrip("./")
glob = os.path.normpath(pattern)
uplvl = glob.count("../")
glob = glob.lstrip("./")
if anchor_mode is PathGlobAnchorMode.DECLARED_PATH:
glob = os.path.join(base, glob)
return cls(
raw=pattern,
anchor_mode=anchor_mode,
glob=cls._parse_pattern(glob),
uplvl=uplvl,
)

@staticmethod
Expand All @@ -57,7 +61,7 @@ def _parse_pattern(pattern: str) -> Pattern:

def _match_path(self, path: str, base: str) -> str | None:
if self.anchor_mode is PathGlobAnchorMode.INVOKED_PATH:
path = os.path.relpath(path, base)
path = os.path.relpath(path, base + "/.." * self.uplvl)
if path.startswith(".."):
# The `path` is not in the sub tree of `base`.
return None
Expand Down
24 changes: 17 additions & 7 deletions src/python/pants/backend/visibility/glob_test.py
View file
Original file line number Diff line number Diff line change
Expand Up @@ -8,20 +8,22 @@


@pytest.mark.parametrize(
"pattern, base, anchor_mode, glob",
"pattern, base, anchor_mode, glob, uplvl",
[
("foo", "base", "", "foo$"),
(".", "base", ".", "$"),
("./foo", "base", ".", "foo$"),
("/foo", "base", "/", "base/foo$"),
("//foo", "base", "//", "foo$"),
("foo", "base", "", "foo$", 0),
(".", "base", ".", "$", 0),
("./foo", "base", ".", "foo$", 0),
("../foo/../bar", "base", ".", "bar$", 1),
("/foo/../baz", "base", "/", "base/baz$", 0),
("//foo", "base", "//", "foo$", 0),
],
)
def test_parse_pattern(pattern: str, base: str, anchor_mode: str, glob: str) -> None:
def test_parse_pattern(pattern: str, base: str, anchor_mode: str, glob: str, uplvl: int) -> None:
parsed = PathGlob.parse(pattern, base)
assert pattern == parsed.raw
assert anchor_mode == parsed.anchor_mode.value
assert glob == parsed.glob.pattern
assert uplvl == parsed.uplvl


@pytest.mark.parametrize(
Expand All @@ -30,11 +32,19 @@ def test_parse_pattern(pattern: str, base: str, anchor_mode: str, glob: str) ->
(
PathGlob.parse("./foo/bar", "base"),
(
# path, base, expected
("tests/foo", "src", None),
("src/foo", "src", "foo"),
("src/foo", "src/a", None),
),
),
(
PathGlob.parse("../foo/bar", "base"),
(
# path, base, expected
("src/foo/bar", "src/qux", "foo/bar"),
),
),
],
)
def test_match_path(glob: PathGlob, tests: tuple[tuple[str, str, str | None], ...]) -> None:
Expand Down
38 changes: 19 additions & 19 deletions src/python/pants/backend/visibility/rule_types.py
View file
Original file line number Diff line number Diff line change
Expand Up @@ -167,8 +167,9 @@ def create_parser_state(
) -> BuildFileDependencyRulesParserState:
return BuildFileVisibilityRulesParserState(path, cast(BuildFileVisibilityRules, parent))

@staticmethod
@classmethod
def check_dependency_rules(
cls,
*,
origin_address: Address,
origin_adaptor: TargetAdaptor,
Expand All @@ -193,8 +194,7 @@ def check_dependency_rules(
cast(BuildFileVisibilityRules, dependencies_rules).get_action(
target=origin_adaptor,
address=dependency_address,
relpath=origin_address.spec_path,
outgoing_dependency=True,
relpath=cls._get_address_relpath(origin_address),
)
if dependencies_rules is not None
else (None, DependencyRuleAction.ALLOW, None)
Expand All @@ -215,8 +215,7 @@ def check_dependency_rules(
cast(BuildFileVisibilityRules, dependents_rules).get_action(
target=dependency_adaptor,
address=origin_address,
relpath=dependency_address.spec_path,
outgoing_dependency=False,
relpath=cls._get_address_relpath(dependency_address),
)
if dependents_rules is not None
else (None, DependencyRuleAction.ALLOW, None)
Expand Down Expand Up @@ -246,6 +245,12 @@ def check_dependency_rules(
dependency_type=dependency_adaptor.type_alias,
)

@staticmethod
def _get_address_relpath(address: Address) -> str:
if address.is_file_target:
return os.path.dirname(address.filename)
return address.spec_path

@staticmethod
def _get_address_path(address: Address) -> str:
if address.is_file_target:
Expand All @@ -259,7 +264,6 @@ def get_action(
target: TargetAdaptor,
address: Address,
relpath: str,
outgoing_dependency: bool,
) -> tuple[VisibilityRuleSet | None, DependencyRuleAction | None, str | None]:
"""Get applicable rule for target type from `path`.
Expand All @@ -272,20 +276,16 @@ def get_action(
for visibility_rule in ruleset.rules:
if visibility_rule.match(path, relpath):
if visibility_rule.action != DependencyRuleAction.ALLOW:
target_addr = f"{relpath}:{target.name or os.path.basename(relpath)}"
target_type = target.type_alias
if outgoing_dependency:
logger.debug(
f"{visibility_rule.action.name}: the `{target_type}` target "
f"{target_addr} dependency to a target at {path} by dependency rule "
f"{str(visibility_rule)!r} declared in {self.path}"
)
else:
logger.debug(
f"{visibility_rule.action.name}: dependency on the `{target_type}` "
f"target {target_addr} from a target at {path} by dependent rule "
f"{str(visibility_rule)!r} declared in {self.path}"
logger.debug(
softwrap(
f"""
{visibility_rule.action.name}: type:{target.type_alias}
name:{target.name} path:{path!r} relpath:{relpath!r} address:{address}
rule:{str(visibility_rule)!r} {self.path}:
{', '.join(map(str, ruleset.rules))}
"""
)
)
return ruleset, visibility_rule.action, str(visibility_rule)
return ruleset, None, None

Expand Down
59 changes: 49 additions & 10 deletions src/python/pants/backend/visibility/rule_types_test.py
View file
Original file line number Diff line number Diff line change
Expand Up @@ -473,8 +473,17 @@ def assert_dependency_rules(
],
)

expected = {address: action for address, (_, action) in zip(addresses, dependencies)}
assert expected == {addr: rule.action for addr, rule in rsp.dependencies_rule.items()}
for address, (_, action) in zip(addresses, dependencies):
application = rsp.dependencies_rule[address]
print(
"-",
application.rule_description,
"\n ",
application.origin_address.spec,
application.action.name,
application.dependency_address.spec,
)
assert action == application.action


def test_dependency_rules(rule_runner: RuleRunner, caplog) -> None:
Expand Down Expand Up @@ -576,8 +585,8 @@ def BUILD(dependencies: tuple = (), dependents: tuple = (), extra: str = "") ->
expect_logged=[
(
logging.DEBUG,
"WARN: dependency on the `target` target src/a/a2:joker from a target at src/a by "
"dependent rule '?*' declared in src/a/a2/BUILD",
"WARN: type:target name:joker path:'src/a' relpath:'src/a/a2' address:src/a:a "
"rule:'?*' src/a/a2/BUILD: ?*",
),
],
exclusively=False,
Expand All @@ -598,8 +607,8 @@ def BUILD(dependencies: tuple = (), dependents: tuple = (), extra: str = "") ->
expect_logged=[
(
logging.DEBUG,
"DENY: dependency on the `resources` target src/a:internal from a target at src/b "
"by dependent rule '!*' declared in src/a/BUILD",
"DENY: type:resources name:internal path:'src/b' relpath:'src/a' "
"address:src/b:b rule:'!*' src/a/BUILD: ., !*",
),
],
exclusively=False,
Expand All @@ -617,10 +626,8 @@ def BUILD(dependencies: tuple = (), dependents: tuple = (), extra: str = "") ->
expect_logged=[
(
logging.DEBUG,
# This message is different, as it fired from a dependency rule, rather than a
# dependent rule as in the other cases.
"DENY: the `resources` target src/a:internal dependency to a target at src/b by "
"dependency rule '!*' declared in src/a/BUILD",
"DENY: type:resources name:internal path:'src/b' relpath:'src/a' address:src/b:b "
"rule:'!*' src/a/BUILD: ., !*",
),
],
exclusively=False,
Expand Down Expand Up @@ -823,3 +830,35 @@ def test_file_specific_rules(rule_runner: RuleRunner) -> None:
("src/lib/pub/exception.txt:../lib", DependencyRuleAction.DENY),
("src/lib/priv/secret.txt:../lib", DependencyRuleAction.WARN),
)


def test_relpath_for_file_targets(rule_runner: RuleRunner) -> None:
# Testing purpose:
#
# When a file is owned by a target declared in a parent directory, make sure the correct BUILD
# file is consulted for the rules set to apply, and with the correct relpath for the matching.
rule_runner.write_files(
{
"anchor-mode/invoked/BUILD": dedent(
"""
files(name="inv", sources=["**/*.inv"])
__dependencies_rules__(
("*", "../dependency/*", "!*"),
)
__dependents_rules__(
("*", "../origin/*", "!*"),
)
"""
),
"anchor-mode/invoked/origin/file1.inv:../inv": "",
"anchor-mode/invoked/dependency/file2.inv:../inv": "",
},
)

assert_dependency_rules(
rule_runner,
"anchor-mode/invoked/origin/file1.inv:../inv",
("anchor-mode/invoked/dependency/file2.inv:../inv", DependencyRuleAction.ALLOW),
)

0 comments on commit c3d5b03

Please sign in to comment.