Skip to content

refactor(core): get_zip_runfiles_path should call startswith less #3442

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

Merged
rickeylev merged 1 commit into from
Dec 7, 2025
+23 −17
Merged

refactor(core): get_zip_runfiles_path should call startswith less #3442

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
40 changes: 23 additions & 17 deletions python/private/py_executable.bzl
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -70,6 +70,7 @@ load(":venv_runfiles.bzl", "create_venv_app_files")
_py_builtins = py_internal
_EXTERNAL_PATH_PREFIX = "external"
_ZIP_RUNFILES_DIRECTORY_NAME = "runfiles"
_INIT_PY = "__init__.py"
_LAUNCHER_MAKER_TOOLCHAIN_TYPE = "@bazel_tools//tools/launcher:launcher_maker_toolchain_type"

# Non-Google-specific attributes for executables
Expand Down Expand Up @@ -834,14 +835,14 @@ def _create_zip_file(ctx, *, output, zip_main, runfiles):
manifest.add("__main__.py={}".format(zip_main.path))
manifest.add("__init__.py=")
manifest.add(
"{}=".format(
_get_zip_runfiles_path("__init__.py", workspace_name, legacy_external_runfiles),
),
"{}=".format(_get_zip_runfiles_path(_INIT_PY, workspace_name)),
)

def map_zip_empty_filenames(list_paths_cb):
return [
_get_zip_runfiles_path(path, workspace_name, legacy_external_runfiles) + "="
# FIXME @aignas 2025-12-06: what kind of paths do we expect here? Will they
Copy link
Collaborator

@rickeylev rickeylev Dec 7, 2025

Choose a reason for hiding this comment

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

I would expect the paths to match what you'd see for a file in runfiles.files.

e.g., if a file in the runfiles comes from an external repo (and would have a short path of external/repo/foo.py or ../repo/foo.py), then there would be an empty file with a similar short path.

# ever start with `../` or `external`?
_get_zip_runfiles_path_legacy(path, workspace_name, legacy_external_runfiles) + "="
for path in list_paths_cb().to_list()
]

Expand All @@ -856,7 +857,7 @@ def _create_zip_file(ctx, *, output, zip_main, runfiles):
def map_zip_runfiles(file):
return (
# NOTE: Use "+" for performance
_get_zip_runfiles_path(file.short_path, workspace_name, legacy_external_runfiles) +
_get_zip_runfiles_path_legacy(file.short_path, workspace_name, legacy_external_runfiles) +
"=" + file.path
)

Expand Down Expand Up @@ -893,23 +894,28 @@ def _create_zip_file(ctx, *, output, zip_main, runfiles):
progress_message = "Building Python zip: %{label}",
)

def _get_zip_runfiles_path(path, workspace_name, legacy_external_runfiles):
maybe_workspace = ""
if legacy_external_runfiles and path.startswith(_EXTERNAL_PATH_PREFIX):
zip_runfiles_path = path.removeprefix(_EXTERNAL_PATH_PREFIX)
def _get_zip_runfiles_path(path, workspace_name = ""):
# NOTE @aignas 2025-12-06: This is to avoid the prefix checking in the very
# trivial case that is always happening once per this function call

# NOTE: Use "+" for performance
if workspace_name:
# NOTE: Use "+" for performance
return _ZIP_RUNFILES_DIRECTORY_NAME + "/" + workspace_name + "/" + path
else:
return _ZIP_RUNFILES_DIRECTORY_NAME + "/" + path
Comment on lines +901 to +906
Copy link
Contributor

@gemini-code-assist gemini-code-assist bot Dec 6, 2025

Choose a reason for hiding this comment

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

medium

This function can be slightly simplified to reduce code duplication and improve readability by building the path prefix incrementally. This change maintains the use of + for string concatenation for performance.

    # NOTE: Use "+" for performance
    prefix = _ZIP_RUNFILES_DIRECTORY_NAME + "/"
    if workspace_name:
        prefix += workspace_name + "/"
    return prefix + path


def _get_zip_runfiles_path_legacy(path, workspace_name, legacy_external_runfiles):
if legacy_external_runfiles and path.startswith(_EXTERNAL_PATH_PREFIX):
return _get_zip_runfiles_path(path.removeprefix(_EXTERNAL_PATH_PREFIX))
elif path.startswith("../"):
# NOTE: External runfiles (artifacts in other repos) will have a leading
# path component of "../" so that they refer outside the main workspace
# directory and into the runfiles root. So we simplify it, e.g.
# "workspace/../foo/bar" to simply "foo/bar".
if path.startswith("../"):
zip_runfiles_path = path[3:]
else:
zip_runfiles_path = path
maybe_workspace = workspace_name + "/"

# NOTE: Use "+" for performance
return _ZIP_RUNFILES_DIRECTORY_NAME + "/" + maybe_workspace + zip_runfiles_path
return _get_zip_runfiles_path(path[3:])
else:
return _get_zip_runfiles_path(path, workspace_name)

def _create_executable_zip_file(
ctx,
Expand Down