Ooof a coworker of mine just hit a huge landmine t...
# development
g
Ooof a coworker of mine just hit a huge landmine that was introduced in 2.25... I'm bisecting to figure out now where this happened and why; but if someone knows that'd save me some time.
Copy code
echo 'python_sources(name="foo")' > BUILD.first
echo 'python_sources(name="foo")' > BUILD.second
pants list :
Errors before 2.25, runs incorrectly after that, assuming your targets differ. In the exact case we had
relocated_files
and
shell_command
duplicated in two files, and this poor person spent 3-4 hours just not understanding why their edits in one file had no effect at all.
w
And let's all hope it's not related to call-by-name
What's the error you get?
f
w
There were a couple notes in the release notes that are like... potential causes
g
It's between 2.25.0.dev0 and 2.25.0.dev1:
w
https://github.com/pantsbuild/pants/pull/21725 My first guess was this.- which I think is what Tom said too
f
and that PR/commit landed in that version range
w
Literally.
g
Aight, that's a strong lead indeed šŸ˜„ Thanks to both of you
w
Do the targets specifically need to be in different files in the same folder?
Confirmed the regression started in 333c2392f8d3d2f1b894737baf817b3fe4e9115b https://github.com/pantsbuild/pants/commit/333c2392f8d3d2f1b894737baf817b3fe4e9115b
Copy code
āŗ scratch/tompants % pants --no-pantsd list src/python/pants/build_graph: 
//src/python/pants/build_graph/__init__.py:../../../../all-__init__.py-files
src/python/pants/build_graph:build_graph
src/python/pants/build_graph:foo
src/python/pants/build_graph:tests
src/python/pants/build_graph/__init__.py
src/python/pants/build_graph/__init__.py:foo
src/python/pants/build_graph/address.py
src/python/pants/build_graph/address.py:foo
src/python/pants/build_graph/address_test.py:tests
src/python/pants/build_graph/build_configuration.py
src/python/pants/build_graph/build_configuration.py:foo
src/python/pants/build_graph/build_configuration_test.py:tests
src/python/pants/build_graph/build_file_aliases.py
src/python/pants/build_graph/build_file_aliases.py:foo
src/python/pants/build_graph/build_file_aliases_test.py:tests
src/python/pants/build_graph/subproject_integration_test.py:tests


āŗ scratch/tompants % git checkout e518a82b9a087dc71f977b733d6c073c7e84321c &&  pants --no-pantsd list src/python/pants/build_graph:

M       pants.toml
M       src/python/pants/build_graph/BUILD
Previous HEAD position was 333c2392f8 Fix bad `__hash__` implementations. (#21725)
HEAD is now at e518a82b9a resolve Python 3.11 importlib deprecation WARNings (#21712)
...
pants.engine.internals.mapper.DuplicateNameError: A target with name 'foo' is already defined in 'src/python/pants/build_graph/BUILD', but is also defined in 'src/python/pants/build_graph/BUILD.pants'. Because both targets share the same namespace of 'src/python/pants/build_graph', this is not allowed.
g
Yeah I've found the bug too.
Copy code
name_to_path_and_declared_target: dict[str, tuple[str, TargetAdaptor]] = {}
    for declared_address_map in declared_address_maps:
        for name, target in declared_address_map.name_to_target_adaptor.items():
            name_to_path_and_declared_target[name] = (declared_address_map.path, target)
That name clearly isn't unique in this case... There's a validation step in the AddressFamily create function, but the data is already wrong at that point.
f
g
Yeah I was looking at the same solution; the comment at line 468 suggests it too.
f
pushed some more, just need to fix the exception raising and test to be correct, but the logic detects the case
g
Hmm, what about just raising if
name_to_path_and_declared_target
already contains the name? Maybe not as nice as a separate validation step but a bunch less work.
It wouldn't list all three files if you use the name in three files; but it seems like a very far out case
w
Three BUILD files in the same directory, correct?
f
g
Yepp
w
@gorgeous-winter-99296 Bit of a sidebar, but what's your workflow for multiple build files in the same directory? I had always assumed multiple build files would get merged into one and then processed, but that's clearly not happening. How do overrides and defaults work?
f
I'd rather identify all the BUILD files. Who knows if there is a user out there with 50
BUILD
files in the same directory?
Outlandish but technically permissible.
(And this code could actually identify all of the duplicate names at once, although the exception raise does not do that currently.)
g
Fair point. Though wouldn't the implication be, since this was an error not too long ago, that they'd have added 50 conflicting targets in a very short amount of time, with no adverse effects? Otherwise it'd explode as soon as you added the first duplicate...
f
<joke>Maybe they don't iterate at all and invoke Pants just once a day</joke>
but yeah good point
I'm fine with adjusting the logic however we decide.
let's discuss on the PR
How do overrides and defaults work?
I wonder if we need to check the logic for other "conflicts between
BULD
files" issues.
g
Ack. Re the sidebar; we only do this for containers. We used to have those with the apps in a single BUILD file. However some apps have a bunch of variants, so the files got quite long. So we split them into one build-file-per-container, and eventually we gathered them into
src/container
. So
src/container/BUILD.gcssync
builds our GCS synch sidecar, etc. And this directory contains no defaults and no overrides, since there's no actual things to override on. šŸ™‚
šŸ‘ 1
In this case my coworker was adding a tool to several container, so once they got it working for one they duplicated the rule, and then got review feedback...
w
I approved the PR with the note that we should probably look at BUILD files more holistically, as this feels like an underserved edge case (I'm calling it an edge case, as I'd never seen multiple BUILD files in a directory before). I don't know what I'd expect to happen with defaults and overrides semantically.
To that end, Pants
2.14
introduces `__defaults__`: a mechanism to set the default arguments used for the targets defined in a directory.
https://www.pantsbuild.org/blog/2022/10/26/pants-2-14#less-boilerplate-via-hierarchical-defaults-for-target-field-values I'm curious if that happens on a BUILD file, or directory basis. Never thought about it before
g
Yeah... There's quirks in general with defaults, some are very hard to undo in subdirs... I think the
**parametrize
variant is especially guilty of this. Can't imagine what kind of shenanigans would happen if you had that in two files in one directory, especially affecting the same target types...
šŸ’Æ 1
f
@gorgeous-winter-99296: I ended up going with the simpler version you had suggested (just doing the check in the existing loop).
less invasive, the better for a bug fix
any way, I have to step out so will merge the fix later tonight if no one else does
fix landed and backport PRs are doing their stuff
p
I use 3 BUILD files in 2 directories to keep them cleaner (there is a lot of BUILD metadata in these dirs): https://github.com/StackStorm/st2/tree/master https://github.com/StackStorm/st2/tree/master/packaging I don't use
__defaults__
in those dirs though.