Skip to content

Commit

Permalink
Two enhancements to tailor. (#11625)
Browse files Browse the repository at this point in the history
1. We require root-level targets to be
   explicitly named, so this change makes tailor
   take care of that.

2. It's possible for directory named BUILD to collide
   with a file we want to create under the same name.
   This is not as unlikely as you may think on systems
   where paths are case-insensitive, such as MacOS.
   This change detects this case and renames the BUILD
   file.

[ci skip-rust]

[ci skip-build-wheels]
  • Loading branch information
benjyw committed Mar 4, 2021
1 parent 361cd0b commit 26103f4
Show file tree
Hide file tree
Showing 2 changed files with 39 additions and 2 deletions.
14 changes: 14 additions & 0 deletions src/python/pants/core/goals/tailor.py
Expand Up @@ -268,6 +268,10 @@ async def rename_conflicting_targets(ptgts: PutativeTargets) -> UniquelyNamedPut
for ptgt in ptgts:
idx = 0
possibly_renamed_ptgt = ptgt
# Targets in root-level BUILD files must be named explicitly.
if possibly_renamed_ptgt.path == "" and possibly_renamed_ptgt.kwargs.get("name") is None:
possibly_renamed_ptgt = possibly_renamed_ptgt.rename("root")
# Eliminate any address collisions.
while possibly_renamed_ptgt.address.spec in existing_addrs:
possibly_renamed_ptgt = ptgt.rename(f"{ptgt.name}{idx}")
idx += 1
Expand Down Expand Up @@ -342,6 +346,16 @@ def make_content_str(
@rule(desc="Edit BUILD files with new targets")
async def edit_build_files(req: EditBuildFilesRequest) -> EditedBuildFiles:
ptgts_by_build_file = group_by_build_file(req.putative_targets)
# There may be an existing *directory* whose name collides with that of a BUILD file
# we want to create. This is more likely on a system with case-insensitive paths,
# such as MacOS. We detect such cases and use an alt BUILD file name to fix.
existing_paths = await Get(Paths, PathGlobs(ptgts_by_build_file.keys()))
existing_dirs = set(existing_paths.dirs)
# Technically there could be a dir named "BUILD.pants" as well, but that's pretty unlikely.
ptgts_by_build_file = {
(f"{bf}.pants" if bf in existing_dirs else bf): pts
for bf, pts in ptgts_by_build_file.items()
}
existing_build_files_contents = await Get(DigestContents, PathGlobs(ptgts_by_build_file.keys()))
existing_build_files_contents_by_path = {
ebfc.path: ebfc.content for ebfc in existing_build_files_contents
Expand Down
27 changes: 25 additions & 2 deletions src/python/pants/core/goals/tailor_test.py
Expand Up @@ -141,6 +141,28 @@ def test_rename_conflicting_targets(rule_runner: RuleRunner) -> None:
)


def test_root_targets_are_explicitly_named(rule_runner: RuleRunner) -> None:
rule_runner.create_file("foo.f90", "")
ptgt = PutativeTarget("", "", "fortran_library", ["foo.f90"], FortranLibrarySources.default)
unpts = rule_runner.request(UniquelyNamedPutativeTargets, [PutativeTargets([ptgt])])
ptgts = unpts.putative_targets
assert (
PutativeTargets(
[
PutativeTarget(
"",
"root",
"fortran_library",
["foo.f90"],
FortranLibrarySources.default,
kwargs={"name": "root"},
)
]
)
== ptgts
)


def test_restrict_conflicting_sources(rule_runner: RuleRunner) -> None:
dir_structure = {
"src/fortran/foo/BUILD": "fortran_library(sources=['bar/baz1.f90'])",
Expand Down Expand Up @@ -173,6 +195,7 @@ def test_restrict_conflicting_sources(rule_runner: RuleRunner) -> None:

def test_edit_build_files(rule_runner: RuleRunner) -> None:
rule_runner.create_file("src/fortran/foo/BUILD", 'fortran_library(sources=["bar1.f90"])')
rule_runner.create_dir("src/fortran/baz/BUILD") # NB: A directory, not a file.
req = EditBuildFilesRequest(
PutativeTargets(
[
Expand Down Expand Up @@ -200,12 +223,12 @@ def test_edit_build_files(rule_runner: RuleRunner) -> None:
)
edited_build_files = rule_runner.request(EditedBuildFiles, [req])

assert edited_build_files.created_paths == ("src/fortran/baz/BUILD",)
assert edited_build_files.created_paths == ("src/fortran/baz/BUILD.pants",)
assert edited_build_files.updated_paths == ("src/fortran/foo/BUILD",)

contents = rule_runner.request(DigestContents, [edited_build_files.digest])
expected = [
FileContent("src/fortran/baz/BUILD", "fortran_library()\n".encode()),
FileContent("src/fortran/baz/BUILD.pants", "fortran_library()\n".encode()),
FileContent(
"src/fortran/foo/BUILD",
textwrap.dedent(
Expand Down

0 comments on commit 26103f4

Please sign in to comment.