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.

Sign up for GitHub

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

Jump to bottom

[internal] Use fast_relpath_optional for source root calculation in local_dists. #13091

Merged
merged 1 commit into from
Oct 3, 2021
Merged

[internal] Use fast_relpath_optional for source root calculation in local_dists. #13091

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
8 changes: 3 additions & 5 deletions src/python/pants/backend/python/util_rules/local_dists.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,6 @@
from __future__ import annotations

import logging
import os
import zipfile
from dataclasses import dataclass
from io import BytesIO
Expand All @@ -30,6 +29,7 @@
)
from pants.engine.rules import Get, MultiGet, collect_rules, rule
from pants.engine.target import TransitiveTargets, TransitiveTargetsRequest
from pants.util.dirutil import fast_relpath_optional
from pants.util.docutil import doc_url
from pants.util.meta import frozen_after_init

Expand Down Expand Up @@ -150,10 +150,8 @@ async def build_local_dists(
for source in request.sources.source_files.files:
if source not in unrooted_files_set:
for source_root in source_roots:
if (
source.startswith(source_root)
and os.path.relpath(source, source_root) in provided_files
):
source_relpath = fast_relpath_optional(source, source_root)
Copy link
Contributor

@jsirois jsirois Oct 3, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

It might be good if source_roots were non-str / non-PathLike objects so attempts to use os APIs failed, forcing you to convert them to a PathLike 1st. That might be enough pause to see new, perf appropriate API's hung off the source root type.

Copy link
Sponsor Member Author

Choose a reason for hiding this comment

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

Mm... yea. I think that for a bit there we thought that Path/PurePath were going to be this better API, but it unfortunately seems that that isn't the case.

The challenge is always how widespread these APIs are: there is a huge surface area in the stdlib that takes either str/Path, so any custom path type has a high bar to be worth the extra boilerplate of conversions at the boundary. And we don't have Deref like on the Rust side.

But yea, maybe if it were isolated to a SourceRoot type... although that limits the upside of the custom type as well.

if source_relpath is not None and source_relpath in provided_files:
remaining_sources.remove(source)
remaining_sources_snapshot = await Get(
Snapshot,
Expand Down