Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Two enhancements to tailor. #11625

Merged
merged 2 commits into from Mar 4, 2021
Merged

Two enhancements to tailor. #11625

merged 2 commits into from Mar 4, 2021

Conversation

benjyw
Copy link
Sponsor Contributor

@benjyw benjyw commented Mar 3, 2021

  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]

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]
@benjyw
Copy link
Sponsor Contributor Author

benjyw commented Mar 3, 2021

PS I encountered both these cases in the real world while running tailor on the mypy repo...

@stuhood stuhood added this to the 2.3.x milestone Mar 3, 2021
Copy link
Contributor

@Eric-Arellano Eric-Arellano left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Excellent! Maybe BUILD.pants instead of .alt? I'm trying to think of a name that would make sense to a first-time user who doesn't realize why this is happening.

Or, maybe log detected directory named build, using the BUILD file name BUILD.pants instead? It would break the comprehension you have there, but the clarity could be helpful.

@@ -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", "")
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No need to waste CI time fixing this, but FYI the content takes a default arg and you can leave that off.

Copy link
Sponsor Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I prefer to be explicit anyway.

[ci skip-rust]

[ci skip-build-wheels]
@benjyw
Copy link
Sponsor Contributor Author

benjyw commented Mar 3, 2021

Changed the alt extension to ".pants", but I didn't add logging since it will be outside the flow of the normal goal output. It's not a super-common case and I think it'll be reasonably obvious why. Certainly if anyone tries to rename the file...

@benjyw benjyw merged commit 26103f4 into pantsbuild:master Mar 4, 2021
@benjyw benjyw deleted the tailor_tweaks branch March 4, 2021 00:36
benjyw added a commit to benjyw/pants that referenced this pull request Mar 4, 2021
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]
benjyw added a commit that referenced this pull request Mar 4, 2021
* Two enhancements to tailor. (#11625)

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.

* Ensure that ancestor files at the buildroot are found. (#11632)

Previously we would find ancestor files correctly all the
way to the source root, and even above, but would stop short
at the buildroot.

However there are real-world cases in which the buildroot is
the source root, and can contain, e.g., conftest.py files which
we fail to discover.

This change fixes this oversight.

[ci skip-rust]

[ci skip-build-wheels]
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants