Generate `jvm_artifact` targets from `pom.xml` #20336

grihabor · 2023-12-25T14:45:08Z

No description provided.

src/python/pants/jvm/target_types.py

benjyw

I'm no expert on the JVM backend, but I guess this was mostly inspired by the scala equivalent? Looks good, with a couple of comments.

benjyw · 2024-01-26T23:42:19Z

src/python/pants/jvm/target_types.py

+def parse_pom_xml(content: bytes) -> Iterator[Coordinate]:
+    root = ET.fromstring(content.decode("utf-8"))
+    match = re.match(r"^(\{.*\})project$", root.tag)
+    assert match, root.tag


Can this assert fail on a bad pom? If so we want to throw a sensible error. Asserts should never fire on bad input, only on bad code.

sure, added an exception with a better message

src/python/pants/jvm/target_types.py

Co-authored-by: Benjy Weinberger <[email protected]>

grihabor

Thank you for review, @benjyw! Ready for another round

grihabor · 2024-01-29T22:48:17Z

src/python/pants/jvm/target_types.py

+def parse_pom_xml(content: bytes) -> Iterator[Coordinate]:
+    root = ET.fromstring(content.decode("utf-8"))
+    match = re.match(r"^(\{.*\})project$", root.tag)
+    assert match, root.tag


sure, added an exception with a better message

benjyw

Looking good! Just a couple of small things left.

benjyw · 2024-02-03T14:56:59Z

src/python/pants/backend/java/target_types_test.py

@@ -0,0 +1,234 @@
+# Copyright 2023 Pants project contributors (see CONTRIBUTORS.md).


Thanks for writing a test. The copyright should be 2024, but more importantly this file should be a sibling of jvm/target_types.py, whose logic it tests, not under the java backend.

benjyw · 2024-02-03T14:58:50Z

src/python/pants/jvm/target_types.py

+    root = ET.fromstring(content.decode("utf-8"))
+    match = re.match(r"^(\{.*\})project$", root.tag)
+    if not match:
+        raise ValueError(f"Unexpected root tag `{root.tag}`, expected project")


Let's add the path to the pom.xml in the error message, so it's clear which one we're talking about.

src/python/pants/jvm/target_types.py

Co-authored-by: Benjy Weinberger <[email protected]>

grihabor

Thank you, @benjyw! Ready for another round

grihabor · 2024-02-04T00:08:54Z

src/python/pants/backend/java/target_types_test.py

@@ -0,0 +1,234 @@
+# Copyright 2023 Pants project contributors (see CONTRIBUTORS.md).


grihabor · 2024-02-04T00:09:46Z

src/python/pants/jvm/target_types.py

+    root = ET.fromstring(content.decode("utf-8"))
+    match = re.match(r"^(\{.*\})project$", root.tag)
+    if not match:
+        raise ValueError(f"Unexpected root tag `{root.tag}`, expected project")


benjyw · 2024-02-04T21:27:12Z

src/python/pants/jvm/target_types.py

@@ -517,7 +517,7 @@ async def generate_from_pom_xml(
    )
    files = await Get(DigestContents, Digest, pom_xml.snapshot.digest)
    mapping = request.generator[JvmArtifactsPackageMappingField].value
-    coordinates = parse_pom_xml(files[0].content)
+    coordinates = parse_pom_xml(files[0].content, pom_xml_path=pom_xml.snapshot.files[0])


It just occurred to me that if files is empty (which it shouldn't be due to other checks, but I hate relying on non-local checks) this will crash with a non-useful error message. So I'd recommend checking len(files) and erroring with "Found no file at "

Sure b68ec95

benjyw

Nice! Thanks for your patience through the reviews.

grihabor · 2024-02-12T15:34:15Z

Thank you for time!

Add docs for the `jvm_artifacts` target implemented in #20336

Add docs for the `jvm_artifacts` target implemented in pantsbuild#20336

…#20919) Add docs for the `jvm_artifacts` target implemented in #20336

…#20920) Add docs for the `jvm_artifacts` target implemented in #20336

grihabor commented Dec 26, 2023

View reviewed changes

src/python/pants/jvm/target_types.py Outdated Show resolved Hide resolved

grihabor force-pushed the generate-jvm-artifact-targets-from-pom-xml branch from 5b53296 to 6eea77c Compare December 29, 2023 23:48

grihabor added 8 commits January 5, 2024 23:41

Generate jvm_artifact targets from pom.xml

c00bb06

Register JvmArtifactsTargetGenerator

9ddaa7e

Parse type annotation

9fd1826

Handle None

d2e16bf

Add test

83ceda1

Add more tests

e58a7b5

Use Coordinate

93c8427

Revert .gitignore

a1325fe

grihabor force-pushed the generate-jvm-artifact-targets-from-pom-xml branch from fbd176d to a1325fe Compare January 5, 2024 22:54

grihabor mentioned this pull request Jan 5, 2024

Move jvm Coordinate class to a separate module #20338

Merged

grihabor commented Jan 6, 2024

View reviewed changes

src/python/pants/jvm/target_types.py Show resolved Hide resolved

Remove unused error types

2c580c3

benjyw added the category:new feature label Jan 26, 2024

benjyw reviewed Jan 26, 2024

View reviewed changes

grihabor and others added 2 commits January 27, 2024 18:03

Update error text

285986c

Co-authored-by: Benjy Weinberger <[email protected]>

Better exception

952f8a3

grihabor commented Jan 29, 2024

View reviewed changes

grihabor requested a review from benjyw January 29, 2024 23:35

benjyw reviewed Feb 3, 2024

View reviewed changes

grihabor and others added 4 commits February 4, 2024 00:32

Rename tag -> element

cd3429a

Co-authored-by: Benjy Weinberger <[email protected]>

Move target_types_test.py

540d0f2

Better error message

4062f73

Fix year

e45d346

grihabor commented Feb 4, 2024

View reviewed changes

grihabor requested a review from benjyw February 4, 2024 00:11

grihabor added 2 commits February 4, 2024 01:43

Fix linters

7d4c26c

Format BUILD

f512dc3

benjyw reviewed Feb 4, 2024

View reviewed changes

Handle 0 files

b68ec95

benjyw approved these changes Feb 12, 2024

View reviewed changes

benjyw merged commit db3cb04 into pantsbuild:main Feb 12, 2024
24 checks passed

grihabor mentioned this pull request May 8, 2024

Add docs for jvm_artifacts target generator #20890

Merged

benjyw pushed a commit that referenced this pull request May 13, 2024

Add docs for jvm_artifacts target generator (#20890)

4012d9e

Add docs for the `jvm_artifacts` target implemented in #20336

grihabor added a commit to grihabor/pants that referenced this pull request May 13, 2024

Add docs for jvm_artifacts target generator (pantsbuild#20890)

ca8b199

Add docs for the `jvm_artifacts` target implemented in pantsbuild#20336

This was referenced May 13, 2024

Add docs for jvm_artifacts target generator (Cherry-pick of #20890) #20919

Merged

Add docs for jvm_artifacts target generator (Cherry-pick of #20890) #20920

Merged

grihabor added a commit to grihabor/pants that referenced this pull request May 13, 2024

Add docs for jvm_artifacts target generator (pantsbuild#20890)

a5adb6b

Add docs for the `jvm_artifacts` target implemented in pantsbuild#20336

huonw pushed a commit that referenced this pull request May 13, 2024

Add docs for jvm_artifacts target generator (Cherry-pick of #20890) (…

9fc635d

…#20919) Add docs for the `jvm_artifacts` target implemented in #20336

huonw pushed a commit that referenced this pull request May 13, 2024

Add docs for jvm_artifacts target generator (Cherry-pick of #20890) (…

fdc12e7

…#20920) Add docs for the `jvm_artifacts` target implemented in #20336

purajit mentioned this pull request Jun 26, 2024

Bump default terraform version to 1.9.0, add all known terraform versions inbetween #21110

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Generate `jvm_artifact` targets from `pom.xml` #20336

Generate `jvm_artifact` targets from `pom.xml` #20336

grihabor commented Dec 25, 2023

benjyw left a comment

benjyw Jan 26, 2024

grihabor Jan 29, 2024

grihabor left a comment

grihabor Jan 29, 2024

benjyw left a comment

benjyw Feb 3, 2024

grihabor Feb 4, 2024

benjyw Feb 3, 2024

grihabor Feb 4, 2024

grihabor left a comment

grihabor Feb 4, 2024

grihabor Feb 4, 2024

benjyw Feb 4, 2024

grihabor Feb 10, 2024

benjyw left a comment

grihabor commented Feb 12, 2024

		@@ -0,0 +1,234 @@
		# Copyright 2023 Pants project contributors (see CONTRIBUTORS.md).

Generate jvm_artifact targets from pom.xml #20336

Generate jvm_artifact targets from pom.xml #20336

Conversation

grihabor commented Dec 25, 2023

benjyw left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

grihabor left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

benjyw left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

grihabor left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

benjyw left a comment

Choose a reason for hiding this comment

grihabor commented Feb 12, 2024

Generate `jvm_artifact` targets from `pom.xml` #20336

Generate `jvm_artifact` targets from `pom.xml` #20336