[internal] Add infrastructure to support deprecating field names #13666

kaos · 2021-11-18T12:15:30Z

Inspired by #13164

Pre-work for renaming the pypi_repositories field to repositories for python_distribution targets.

Signed-off-by: Andreas Stenius <andreas.stenius@svenskaspel.se> # Building wheels and fs_util will be skipped. Delete if not intended. [ci skip-build-wheels]

kaos · 2021-11-18T12:17:00Z

~~Still TODO: teach update-build-files how to fix.~~

EDIT: Done.

# 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]

kaos · 2021-11-18T15:16:30Z

It works! :D With a dummy deprecation, this is what it looks like..

$ ./pants_from_sources update-build-files
16:13:22.93 [WARN] DEPRECATED: option 'interpreter_search_paths' in scope 'python-setup' will be removed in version 2.10.0.dev0.

Moved to `[python-bootstrap] search_path`.
16:13:22.93 [WARN] DEPRECATED: option 'interpreter_search_paths' in scope 'python-setup' will be removed in version 2.10.0.dev0.

Moved to `[python-bootstrap] search_path`.
16:13:22.93 [WARN] DEPRECATED: scope python-setup will be removed in version 2.10.0.dev0.

Use scope python instead (options: interpreter_constraints, requirement_constraints, interpreter_search_paths)
Updated BUILD:
  - Format with Black
Updated docker/res/BUILD:
  - Format with Black
Updated helloworld/BUILD:
  - Rename `pex_binary.entry_point` to `pex_binary.foo_blarg`
Updated helloworld/greet/BUILD:
  - Format with Black
Updated helloworld/util/BUILD:
  - Format with Black

diff --git a/helloworld/BUILD b/helloworld/BUILD
index 47cfc3d..3042a68 100644
--- a/helloworld/BUILD
+++ b/helloworld/BUILD
@@ -22,7 +22,7 @@ resources(
 #  https://www.pantsbuild.org/docs/python-run-goal.
 pex_binary(
     name="pex_binary",
-    entry_point="main.py",
+    foo_blarg="main.py",
 )

Eric-Arellano

Woohoo, this is awesome! Pardon the delayed review.

src/python/pants/core/goals/update_build_files_test.py

Eric-Arellano · 2021-11-24T22:07:17Z

src/python/pants/core/goals/update_build_files.py

+        request.path,
+        tuple(updated_text_lines),
+        change_descriptions=tuple(
+            f"Rename `{request.red(deprecated)}` to `{request.green(new)}`"


How about this? (Still using colors)

Rename the field deprecated to new for target type tgt_type

Eric-Arellano · 2021-11-24T22:09:13Z

src/python/pants/core/goals/update_build_files.py

+    request: RenameDeprecatedFieldsRequest,
+    renamed_field_types: RenamedFieldTypes,
+) -> RewrittenBuildFile:
+    target = None


Maybe call this pants_target. I wasn't sure until reading this for a few minutes if "target" meant some generic parsing term.

Eric-Arellano · 2021-11-24T22:26:09Z

src/python/pants/core/goals/update_build_files.py

+class RenamedFieldTypes(FrozenDict[Tuple[str, str], str]):
+    """Deprecated field type names for target to new names."""


How about FrozenDict[str, FrozenDict[str, str]], which maps target types to field renames. Then you can rewrite the implementation I think to more eagerly exit if the target type is not a match?

I'm not sure I followed the level code correctly...the level should be 1. A nested target should not exist - targets are always top-level.

At this point, might also be worth switching to a frozen dataclass rather than subclassing FrozenDict so that you can give a name to the variable like target_types_to_field_renames.

How about FrozenDict[str, FrozenDict[str, str]], which maps target types to field renames. Then you can rewrite the implementation I think to more eagerly exit if the target type is not a match?

I think you're referring to eagerly exit out of should_be_renamed, correct?
OK, will try it out.. see what it'll look like (not convinced it will win much, but perhaps the data structure will be easier to comprehend?)

I'm not sure I followed the level code correctly...the level should be 1. A nested target should not exist - targets are always top-level.

Agreed, there's a check that we only ever operate when level==1.

This is to protect against constructs, such as this, as an example:

python_distribution( provides=setup_py(my_field="value"), )

As we do not want to change my_field here... The level code might be overkill, but it plugs a potential hole.
And, as I write this, I realize I don't only look at the field name, but the enclosing target name as well (setup_py in this example) and as such, it would be safe to ignore the level all together.. thanks for the nudge, will get rid of it.

Oh, wait, no, the level is important for this reason..

python_distribution( name="dist", provides=setup_py(....), old_field="value", )

We must keep track of when we're seeing fields in setup_py, which we should ignore, and when we're back in python_distritbution, so we know that old_field is deprecated and should be replaced.

It's hard to make this distinction unless I keep track of when I enter and exit constructs, such as setup_py there..

I've made some refactorings that will hopefully be easier to follow now.

…ias_support

Eric-Arellano

Looks great!

Internal changes: * [internal] Remove superfluous f-string specifiers ([#13821](#13821)) * [internal] scala: extract annotations as consumed types ([#13810](#13810)) * [jvm] Split nailgun digest from input file digests ([#13813](#13813)) * [internal] jvm: add jre_major_version and use stderr to properly extract version ([#13812](#13812)) * [internal] Clean up Go `embed` support's handling of dependencies ([#13801](#13801)) * [internal] scala: handle package object syntax in parser ([#13809](#13809)) * [internal] java: fix junit sentinel rule setup ([#13815](#13815)) * [internal] upgrade to rust v1.57.0 ([#13807](#13807)) * [internal] add failing test for FrozenDict equality issue ([#13389](#13389)) * [internal] Use `PyObject` instead of `Value` in more places ([#13802](#13802)) * Remove MultiPlatform Process abstractions ([#12725](#12725)) * [internal] add `JvmToolBase` for lockfile handling for JVM tools ([#13777](#13777)) * [internal] Port `MergeDigests` to Rust ([#13773](#13773)) * [jvm] Spawn nailgun servers outside the pool lock ([#13796](#13796)) * [internal] DRY loading internal Go binaries ([#13800](#13800)) * [internal] Convert unit tests to use pytest ([#13798](#13798)) * [internal] remove dead code - socket util. ([#13797](#13797)) * [internal] Reorganize Go parser scripts ([#13791](#13791)) * Adds Jackson core/datatype to `JVM_ARTIFACT_MAPPINGS` ([#13792](#13792)) * [internal] go: initial support for embedding resources ([#13743](#13743)) * [internal] Refer to `go.mod` path when downloading packages ([#13786](#13786)) * [internal] More robust Go dependency inference tests ([#13785](#13785)) * [internal] `tailor` doesn't add `go_package` for `testdata` folder ([#13783](#13783)) * [internal] Add Scala backend to dryrun for wheel builds. ([#13772](#13772)) * [internal] Unify JVM thirdparty resolves ([#13771](#13771)) * [internal] scala: infer dependencies from consumed symbols and intermediate scopes ([#13758](#13758)) * [internal] java: infer scala encoded symbols ([#13739](#13739)) * [internal] scala: parse and report package scopes ([#13738](#13738)) * [internal] go: configure included env vars for `GoSdkProcess` ([#13734](#13734)) * Fix some `./cargo audit` vulnerabilities. ([#13728](#13728)) * [internal] fix non-empty __init__.py ([#13730](#13730)) * Compute RepositoryPex directly from addresses. ([#13716](#13716)) * Upgrade to cargo-audit 0.16.0. ([#13729](#13729)) * Simplify `NativeHandler`. ([#13727](#13727)) * [internal] Switch to a maintained fork of the `fuse` crate for `brfs`. ([#13679](#13679)) * [internal] Add infrastructure to support deprecating field names ([#13666](#13666)) * [internal] Introduce OptionalPex/OptionalPexRequest. ([#13712](#13712)) * [internal] tailor adds go_package targets ([#13703](#13703)) * [internal] Remove unused testproject for pants-plugin ([#13704](#13704)) * [internal] Rename ambiguous `subpath` variable for Go code ([#13701](#13701)) * [internal] scala: generate the JVM names seen by Java code for Scala code ([#13696](#13696)) * Use RequirementsPexRequest in run_pex_binary.py. ([#13693](#13693)) * [internal] Refactor finding owning `go_mod` for first-party packages ([#13695](#13695)) * [internal] repl: add append_only_caches / run_in_workspace attributes to ReplRequest ([#13599](#13599)) * [internal] switch back to official `cargo-ensure-prefix` crate ([#13692](#13692)) * [internal] scala: extract type names from all Type.* AST nodes ([#13685](#13685)) * [internal] Convert unit tests to use pytest ([#13652](#13652)) * Unblock codegen support for java export analysis (#13645) ([#13675](#13675)) * [internal] upgrade to Rust 2021 Edition ([#13644](#13644)) * [internal] Don't store derived values on `go_first_party_package` targets ([#13676](#13676)) * Upgrade to py03 0.15.1. ([#13725](#13725)) * Add PyPDF2 to module mapping ([#13717](#13717)) * Upgrade console and indacatif. ([#13726](#13726))

[internal] Add infrastructure to support deprecating field names

4e696c8

Signed-off-by: Andreas Stenius <andreas.stenius@svenskaspel.se> # Building wheels and fs_util will be skipped. Delete if not intended. [ci skip-build-wheels]

kaos requested a review from Eric-Arellano November 18, 2021 12:15

[internal] Teach update-build-files to fix deprecated field names.

7d7b002

# 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]

Eric-Arellano reviewed Nov 24, 2021

View reviewed changes

kaos added 2 commits November 25, 2021 08:10

Merge remote-tracking branch 'upstream/main' into deprecated_field_al…

6ddf8b1

…ias_support

Refactoring after review feedback.

22edec8

Eric-Arellano approved these changes Nov 25, 2021

View reviewed changes

kaos merged commit e0dd7a7 into pantsbuild:main Nov 26, 2021

kaos deleted the deprecated_field_alias_support branch November 26, 2021 07:39

illicitonion mentioned this pull request Dec 6, 2021

Prep for 2.9.0.dev3 #13823

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[internal] Add infrastructure to support deprecating field names #13666

[internal] Add infrastructure to support deprecating field names #13666

kaos commented Nov 18, 2021

kaos commented Nov 18, 2021 •

edited

Loading

kaos commented Nov 18, 2021

Eric-Arellano left a comment

Eric-Arellano Nov 24, 2021

Eric-Arellano Nov 24, 2021

Eric-Arellano Nov 24, 2021

kaos Nov 25, 2021

kaos Nov 25, 2021

kaos Nov 25, 2021

kaos Nov 25, 2021

Eric-Arellano left a comment

		class RenamedFieldTypes(FrozenDict[Tuple[str, str], str]):
		"""Deprecated field type names for target to new names."""

[internal] Add infrastructure to support deprecating field names #13666

[internal] Add infrastructure to support deprecating field names #13666

Conversation

kaos commented Nov 18, 2021

kaos commented Nov 18, 2021 • edited Loading

kaos commented Nov 18, 2021

Eric-Arellano left a comment

Choose a reason for hiding this comment

Eric-Arellano Nov 24, 2021

Choose a reason for hiding this comment

Eric-Arellano Nov 24, 2021

Choose a reason for hiding this comment

Eric-Arellano Nov 24, 2021

Choose a reason for hiding this comment

kaos Nov 25, 2021

Choose a reason for hiding this comment

kaos Nov 25, 2021

Choose a reason for hiding this comment

kaos Nov 25, 2021

Choose a reason for hiding this comment

kaos Nov 25, 2021

Choose a reason for hiding this comment

Eric-Arellano left a comment

Choose a reason for hiding this comment

kaos commented Nov 18, 2021 •

edited

Loading