Skip to content

Commit

Permalink
Actually run deprecated targets fixer (pantsbuild#18860)
Browse files Browse the repository at this point in the history
This fixes pantsbuild#18509 by updating `update-build-files`'s
`maybe_rename_deprecated_targets` rule to run the deprecated target
fixers: previously, that rule was accidentally rerunning the deprecated
_fields_ fixers and deprecated targets weren't being fixed. This fix
ensures `experimental_shell_command` is switched to `shell_command` and
`experimental_run_shell_command` is switched to `run_shell_command`.

As part of this, I rewrote `renamed_targets_rules.py` to match the
current phrasing of `renamed_fields_rules.py`: without doing that, I was
getting graph errors. I suspect there was smaller rewrites that would've
worked, but it didn't seem unreasonable to have both the
`renamed_..._rules.py` files basically match from top to bottom, with
only the type names and transformation details differing.

This _doesn't_ do the update to `workdir="/"` for `run_shell_command`
that I flagged in
pantsbuild#18509 (comment).
With the current token-based rewrites, doing that seems like a lot of
work, although, it would be possible (do something similar to
`renamed_fields_rules.py` matching parens/brackets and iterate looking
for a `workdir=...` field, and updating/inserting it, all packaged up to
be vaguely reusable).
  • Loading branch information
huonw authored May 3, 2023
1 parent b82509f commit 2417cfe
Show file tree
Hide file tree
Showing 3 changed files with 32 additions and 23 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@
from __future__ import annotations

import tokenize
from dataclasses import dataclass

from pants.backend.build_files.fix.base import FixBuildFilesRequest
from pants.backend.build_files.fix.deprecations.base import FixBUILDFileRequest, FixedBUILDFile
Expand All @@ -22,38 +23,45 @@ class RenameTargetsInFilesRequest(FixBuildFilesRequest):
tool_subsystem = BUILDDeprecationsFixer


class RenameTargetsInFileRequest(FrozenDict[str, str]):
class RenameTargetsInFileRequest(FixBUILDFileRequest):
"""Deprecated target type names to new names."""


@dataclass(frozen=True)
class RenamedTargetTypes:
"""Map deprecated field names to their new name, per target."""

target_renames: FrozenDict[str, str]


@rule
def determine_renamed_target_types(
target_types: RegisteredTargetTypes,
) -> RenameTargetsInFileRequest:
return RenameTargetsInFileRequest(
{
tgt.deprecated_alias: tgt.alias
for tgt in target_types.types
if tgt.deprecated_alias is not None
}
) -> RenamedTargetTypes:
return RenamedTargetTypes(
FrozenDict(
{
tgt.deprecated_alias: tgt.alias
for tgt in target_types.types
if tgt.deprecated_alias is not None
}
)
)


class RenameRequest(FixBUILDFileRequest):
pass


@rule
def fix_single(
request: RenameRequest,
renamed_target_types: RenameTargetsInFileRequest,
request: RenameTargetsInFileRequest,
renamed_target_types: RenamedTargetTypes,
) -> FixedBUILDFile:
tokens = request.tokenize()

def should_be_renamed(token: tokenize.TokenInfo) -> bool:
no_indentation = token.start[1] == 0
if not (
token.type is tokenize.NAME and token.string in renamed_target_types and no_indentation
token.type is tokenize.NAME
and token.string in renamed_target_types.target_renames
and no_indentation
):
return False
# Ensure that the next token is `(`
Expand All @@ -70,7 +78,7 @@ def should_be_renamed(token: tokenize.TokenInfo) -> bool:
line_index = token.start[0] - 1
line = updated_text_lines[line_index]
suffix = line[token.end[1] :]
new_symbol = renamed_target_types[token.string]
new_symbol = renamed_target_types.target_renames[token.string]
updated_text_lines[line_index] = f"{new_symbol}{suffix}"

return FixedBUILDFile(request.path, content="".join(updated_text_lines).encode("utf-8"))
Expand All @@ -82,7 +90,7 @@ async def fix(
) -> FixResult:
digest_contents = await Get(DigestContents, Digest, request.snapshot.digest)
fixed_contents = await MultiGet(
Get(FixedBUILDFile, RenameRequest(file_content.path, file_content.content))
Get(FixedBUILDFile, RenameTargetsInFileRequest(file_content.path, file_content.content))
for file_content in digest_contents
)
snapshot = await Get(
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -6,10 +6,11 @@
import pytest

from pants.backend.build_files.fix.deprecations.renamed_targets_rules import (
RenameRequest,
RenamedTargetTypes,
RenameTargetsInFileRequest,
fix_single,
)
from pants.util.frozendict import FrozenDict


@pytest.mark.parametrize(
Expand All @@ -29,8 +30,8 @@
def test_rename_deprecated_target_types_noops(lines: list[str]) -> None:
content = "\n".join(lines).encode("utf-8")
result = fix_single(
RenameRequest("BUILD", content=content),
RenameTargetsInFileRequest({"deprecated_name": "new_name"}),
RenameTargetsInFileRequest("BUILD", content=content),
RenamedTargetTypes(FrozenDict({"deprecated_name": "new_name"})),
)
assert result.content == content

Expand All @@ -46,7 +47,7 @@ def test_rename_deprecated_target_types_noops(lines: list[str]) -> None:
)
def test_rename_deprecated_target_types_rewrite(lines: list[str], expected: list[str]) -> None:
result = fix_single(
RenameRequest("BUILD", content="\n".join(lines).encode("utf-8")),
RenameTargetsInFileRequest({"deprecated_name": "new_name"}),
RenameTargetsInFileRequest("BUILD", content="\n".join(lines).encode("utf-8")),
RenamedTargetTypes(FrozenDict({"deprecated_name": "new_name"})),
)
assert result.content == "\n".join(expected).encode("utf-8")
2 changes: 1 addition & 1 deletion src/python/pants/core/goals/update_build_files.py
Original file line number Diff line number Diff line change
Expand Up @@ -382,7 +382,7 @@ async def maybe_rename_deprecated_targets(
old_bytes = "\n".join(request.lines).encode("utf-8")
new_content = await Get(
FixedBUILDFile,
renamed_fields_rules.RenameFieldsInFileRequest(path=request.path, content=old_bytes),
renamed_targets_rules.RenameTargetsInFileRequest(path=request.path, content=old_bytes),
)

return RewrittenBuildFile(
Expand Down

0 comments on commit 2417cfe

Please sign in to comment.