Support and require disambiguated file addresses#10535
Merged
stuhood merged 11 commits intopantsbuild:masterfrom Aug 4, 2020
Merged
Support and require disambiguated file addresses#10535stuhood merged 11 commits intopantsbuild:masterfrom
stuhood merged 11 commits intopantsbuild:masterfrom
Conversation
7a799e9 to
a633560
Compare
Eric-Arellano
pushed a commit
that referenced
this pull request
Aug 3, 2020
### Problem #10535 normalizes specs to not include the trailing name when it will default. In order to fix tests, many instances of `src/example/test:test` in _output_ (the form is still supported and normalized in input) will be converted to `src/example/test`. To avoid needing to differentiate input from output though, and allow for a cleaner find-replace, I'm going to change all instances. While doing so, I noticed that the `ArgSplitter` operates relative to the current directory rather than the buildroot, which meant that `GoalRuleTestBase` was not recognizing file args or defaulted directory specs correctly. ### Solution Adjust `ArgSplitter` to operate relative to the buildroot. [ci skip-rust] [ci skip-build-wheels]
[ci skip-rust] [ci skip-build-wheels]
…o an AddressInput which can be resolved into an Address by the engine. [ci skip-rust] [ci skip-build-wheels]
[ci skip-rust] [ci skip-build-wheels]
[ci skip-build-wheels] # Rust tests and lints will be skipped. Delete if not intended. [ci skip-rust]
[ci skip-build-wheels] [ci skip-rust]
# Rust tests and lints will be skipped. Delete if not intended. [ci skip-rust] # Building wheels and fs_util will be skipped. Delete if not intended. [ci skip-build-wheels]
a633560 to
a7cbd1c
Compare
stuhood
commented
Aug 3, 2020
* Add back parsing of relative file dependencies via `./` * Fix validation of `../` in the `target_component`. Thanks Stu! * Enforce that top-level file deps must have a target attached to them * Fix ! ignores not working with file deps This also adds many new tests. # Rust tests and lints will be skipped. Delete if not intended. [ci skip-rust] # Building wheels and fs_util will be skipped. Delete if not intended. [ci skip-build-wheels]
stuhood
added a commit
to stuhood/pants
that referenced
this pull request
Aug 4, 2020
# Rust tests and lints will be skipped. Delete if not intended. [ci skip-rust] # Building wheels and fs_util will be skipped. Delete if not intended. [ci skip-build-wheels]
Also simplify the graph_test.py test for parsing dependencies. The parsing in graph.py is now extremely simple and there is no need for redundancy with address_test.py and build_files_test.py. # Rust tests and lints will be skipped. Delete if not intended. [ci skip-rust] # Building wheels and fs_util will be skipped. Delete if not intended. [ci skip-build-wheels]
# Rust tests and lints will be skipped. Delete if not intended. [ci skip-rust] # Building wheels and fs_util will be skipped. Delete if not intended. [ci skip-build-wheels]
Contributor
Eric-Arellano
left a comment
There was a problem hiding this comment.
I added a PR description explaining the changes.
I recommend starting with address.py and address_test.py to see the model changes, then perhaps go to build_files.py and build_files_test.py to see how we now resolve the ambiguity between a file vs. a directory via the engine. The last major change is in graph.py for the dependencies rule.
| @@ -2,109 +2,209 @@ | |||
| # Licensed under the Apache License, Version 2.0 (see LICENSE). | |||
Contributor
There was a problem hiding this comment.
The diff is probably not useful. I recommend looking at it cleanly.
| @@ -1,213 +1,294 @@ | |||
| # Copyright 2014 Pants project contributors (see CONTRIBUTORS.md). | |||
Contributor
There was a problem hiding this comment.
Ditto on the diff not being very helpful.
stuhood
commented
Aug 4, 2020
| def test_address_input_from_file() -> None: | ||
| assert AddressInput("a/b/c.txt", target_component=None).file_to_address() == Address( | ||
| "a/b", relative_file_path="c.txt" | ||
| ) |
Member
Author
There was a problem hiding this comment.
Thanks a lot for adding these, and for the fixes!
stuhood
added a commit
to stuhood/pants
that referenced
this pull request
Aug 4, 2020
[ci skip-rust] [ci skip-build-wheels]
# Rust tests and lints will be skipped. Delete if not intended. [ci skip-rust] # Building wheels and fs_util will be skipped. Delete if not intended. [ci skip-build-wheels]
…malize-file-addresses [ci skip-rust] [ci skip-build-wheels]
Eric-Arellano
approved these changes
Aug 4, 2020
Member
Author
|
@Eric-Arellano : Thanks again! |
stuhood
added a commit
to stuhood/pants
that referenced
this pull request
Aug 4, 2020
[ci skip-rust] [ci skip-build-wheels]
stuhood
added a commit
to stuhood/pants
that referenced
this pull request
Aug 4, 2020
[ci skip-rust] [ci skip-build-wheels]
stuhood
added a commit
to stuhood/pants
that referenced
this pull request
Aug 5, 2020
[ci skip-rust] [ci skip-build-wheels]
Eric-Arellano
pushed a commit
to Eric-Arellano/pants
that referenced
this pull request
Aug 5, 2020
[ci skip-rust] [ci skip-build-wheels]
Eric-Arellano
added a commit
that referenced
this pull request
Aug 5, 2020
The old validation for `BUILD` in the spec path would incorrectly error for a generated subtarget with `BUILD` as its file. We only want to error if `BUILD` is in the `spec_path`, but it's okay if it's in the `relative_file_path` or `target_name`. Our `spec` logic was also incorrect for generated subtargets which live in a directory below the original target, where the original target is the default name. [ci skip-rust] [ci skip-build-wheels]
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
This adds a new syntax for generated subtargets / file deps to specify which original base target they come from:
a/b/c.txt:original, with the following characteristics:namefield is left off, then there is no need for the disambiguation.a/b/c.txt:../../original.Thanks to this new syntax, we no longer look for the Owners of explicit file dependencies. We require the values to be unambiguous, which improves performance and removes an edge case where we would error if there were multiple owners of the file.
We use the engine now to parse an
AddressInputinto anAddress. This is necessary so that we can properly determine if a path is a file or a directory, asa/b/ccould technically be a file or a directory.In followups, we can extend this syntax to file specs. It also will allow us to consistently use generated subtargets when using file specs, which will simplify lots of that code and resolve #10455.