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

Consider how to relativize target Snapshots to source roots #7697

Closed
stuhood opened this issue May 10, 2019 · 2 comments

Comments

Projects
None yet
2 participants
@stuhood
Copy link
Member

commented May 10, 2019

In #7696, @Eric-Arellano noticed that the sources inputted to the pytest environment have not be relativized to their source roots (which is what allows for absolute imports of python modules).

This leads to a question: should target's source Snapshots be relativized to their source roots by default? If not, what API should we expose to accomplish this?

In v1, a consumer of a Target's sources uses

def sources_relative_to_source_root(self):
"""
:API: public
"""
if self.has_sources():
abs_source_root = os.path.join(get_buildroot(), self.target_base)
for source in self.sources_relative_to_buildroot():
abs_source = os.path.join(get_buildroot(), source)
yield os.path.relpath(abs_source, abs_source_root)
to get sources relative to the Target's source root. In v2, a target's Snapshot would need to be cloned into a new relativized version.

@stuhood

This comment has been minimized.

Copy link
Member Author

commented May 10, 2019

I expect that this choice boils down to: "across various languages, is it more common to need to have target sources be relative to their source root, or to the build root?". After answering that question, we could add either an API to prefix paths to Snapshots, or an API to do the equivalent of a batch relpath.

Eric-Arellano added a commit that referenced this issue May 14, 2019

Add builtin Rust rule to strip prefixes from directories for source r…
…oot support (#7699)

### Problem
We need to relativize files to support source roots (https://www.pantsbuild.org/setup_repo.html#source-roots). Otherwise, tools like Pytest will fail to understand imports.

This relates to #7697.

### Solution
Add a builtin Rust rule (i.e. an intrinsic rule) that strips a given prefix from each file in the directory `Digest`.

#### Alternatives considered
* Exposing more implementation details of Snapshot up to the Python side and allowing this manipulation to happen client-side.
* Having Target snapshots be source-root relative (and keep track of that source-root) rather than build-root relative. I suspect that would just lead to us needing the inverse API (nesting), which we'll probably still need at some point. It would save round-tripping through the engine (good), but would make it more likely that people will end up handing the engine unknown snapshots (bad).
@stuhood

This comment has been minimized.

Copy link
Member Author

commented May 16, 2019

Implemented in #7699! Thank you @illicitonion.

@stuhood stuhood closed this May 16, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.