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

Simplify source root stripping. #10543

Merged
merged 4 commits into from Aug 4, 2020
Merged

Conversation

benjyw
Copy link
Sponsor Contributor

@benjyw benjyw commented Aug 4, 2020

Previously we had two parallel rule paths for stripped and unstripped source files.

This added a lot of complication: various types could contain either stripped or
unstripped sources, and there wasn't always an easy way to know which. This also
required a lot of near-duplication of code, to handle each of the two paths.

This change simplifies the types and rules, as follows:

  • Unstripped generic sources are represented by the SourceFiles type.
  • Stripped generic sources are represented by the StrippedSourceFiles type.
  • Unstripped Python sources are represented by the PythonSourceFiles type,
    which wraps a SourceFiles.
  • Stripped Python sources are represented by the StrippedPythonSourceFiles type,
    which wraps a StrippedSourceFiles.

The only way to get a StrippedSourceFiles is to request via a SourceFiles subject.
The only way to get a StrippedPythonSourceFiles is to request it via a PythonSourceFiles subject.

The SourceFiles type carries just enough extra information to allow source-root stripping
even in a merged snapshot, where the originating target information is lost.

As a result, other rules operate primarily on unstripped sources, and strip them just as needed.
For example, finding ancestor files operates only on unstripped sources, and no longer needs
to implement logic to handle the stripped case.

[ci skip-rust]

[ci skip-build-wheels]

Previously we had two parallel rule paths for stripped and unstripped source files.

This added a lot of complication: various types could contain either stripped or
unstripped sources, and there wasn't always an easy way to know which. This also
required a lot of near-duplication of code, to handle each of the two paths.

This change simplifies the types and rules, as follows:

- Unstripped generic sources are represented by the SourceFiles type.
- Stripped generic sources are represented by the StrippedSourceFiles type.
- Unstripped Python sources are represented by the PythonSourceFiles type,
  which wraps a SourceFiles.
- Stripped Python sources are represented by the StrippedPythonSourceFiles type,
  which wraps a StrippedSourceFiles.

The only way to get a StrippedSourceFiles is to request via a SourceFiles subject.
The only way to get a StrippedPythonSourceFiles is to request it via a PythonSourceFiles subject.

The SourceFiles type carries just enough extra information to allow source-root stripping
even in a merged snapshot, where the originating target information is lost.

As a result, other rules operate primarily on unstripped sources, and strip them just as needed.
For example, finding ancestor files operates only on unstripped sources, and no longer needs
to implement logic to handle the stripped case.

[ci skip-rust]

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

benjyw commented Aug 4, 2020

It's a non-trivial diff, but the most important parts to focus on are determine_source_files.py, python_sources.py and strip_source_roots.py. Most of the rest follows as a necessity from those changes.

# 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]
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! Thank you.

[ci skip-rust]

[ci skip-build-wheels]
And fix test.

[ci skip-rust]

[ci skip-build-wheels]
@coveralls
Copy link

Coverage Status

Coverage remained the same at 0.0% when pulling 7a400dd on benjyw:stripping_refactor into 492426d on pantsbuild:master.

@benjyw benjyw merged commit 52c7c26 into pantsbuild:master Aug 4, 2020
@benjyw benjyw deleted the stripping_refactor branch August 4, 2020 17:17
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