From 732c19cd4418c23de0bac95ad20d2fea4e222dec Mon Sep 17 00:00:00 2001 From: Benjy Weinberger Date: Tue, 2 Mar 2021 23:59:50 -0800 Subject: [PATCH 1/2] Two enhancements to tailor. 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] --- src/python/pants/core/goals/tailor.py | 13 +++++++++++ src/python/pants/core/goals/tailor_test.py | 27 ++++++++++++++++++++-- 2 files changed, 38 insertions(+), 2 deletions(-) diff --git a/src/python/pants/core/goals/tailor.py b/src/python/pants/core/goals/tailor.py index 7c3af9e760a..05d0b060f35 100644 --- a/src/python/pants/core/goals/tailor.py +++ b/src/python/pants/core/goals/tailor.py @@ -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 @@ -342,6 +346,15 @@ 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.alt" as well, but that's pretty unlikely. + ptgts_by_build_file = { + (f"{bf}.alt" 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 diff --git a/src/python/pants/core/goals/tailor_test.py b/src/python/pants/core/goals/tailor_test.py index 96137fe0dab..213d3f71651 100644 --- a/src/python/pants/core/goals/tailor_test.py +++ b/src/python/pants/core/goals/tailor_test.py @@ -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'])", @@ -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( [ @@ -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.alt",) 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.alt", "fortran_library()\n".encode()), FileContent( "src/fortran/foo/BUILD", textwrap.dedent( From bba5eb05bd6cbd55b3357802aa12891850d507c2 Mon Sep 17 00:00:00 2001 From: Benjy Weinberger Date: Wed, 3 Mar 2021 12:29:51 -0800 Subject: [PATCH 2/2] Address code review feedback. [ci skip-rust] [ci skip-build-wheels] --- src/python/pants/core/goals/tailor.py | 5 +++-- src/python/pants/core/goals/tailor_test.py | 4 ++-- 2 files changed, 5 insertions(+), 4 deletions(-) diff --git a/src/python/pants/core/goals/tailor.py b/src/python/pants/core/goals/tailor.py index 05d0b060f35..8c58efcbf8f 100644 --- a/src/python/pants/core/goals/tailor.py +++ b/src/python/pants/core/goals/tailor.py @@ -351,9 +351,10 @@ async def edit_build_files(req: EditBuildFilesRequest) -> EditedBuildFiles: # 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.alt" as well, but that's pretty unlikely. + # Technically there could be a dir named "BUILD.pants" as well, but that's pretty unlikely. ptgts_by_build_file = { - (f"{bf}.alt" if bf in existing_dirs else bf): pts for bf, pts in ptgts_by_build_file.items() + (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 = { diff --git a/src/python/pants/core/goals/tailor_test.py b/src/python/pants/core/goals/tailor_test.py index 213d3f71651..f8dd2d4622a 100644 --- a/src/python/pants/core/goals/tailor_test.py +++ b/src/python/pants/core/goals/tailor_test.py @@ -223,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.alt",) + 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.alt", "fortran_library()\n".encode()), + FileContent("src/fortran/baz/BUILD.pants", "fortran_library()\n".encode()), FileContent( "src/fortran/foo/BUILD", textwrap.dedent(