< witty crayon 22786> < ancient vegetable 10556> do you know Pants #development

Join Slack

<@U06A03HV1> <@U021C96KUGJ> do you know if this JV...

# development

hundreds-father-404

01/21/2022, 12:33 AM

@witty-crayon-22786 @ancient-vegetable-10556 do you know if this JVM code is right?

hundreds-father-404

01/21/2022, 12:33 AM

https://github.com/pantsbuild/pants/blob/9cc48833fe7a875eebefd280002a58b42dc375d2/src/python/pants/jvm/resolve/coursier_fetch.py#L715-L731 I'm surprised that we are calling

coursier fetch

there via

Get(CoursierResolvedLockfile, ArtifactRequirements, artifact_requirements)

. I would expect us to instead by reading from a lockfile already saved on-disk

hundreds-father-404

01/21/2022, 12:34 AM

Fwict with Git blame, this code is old. Perhaps it was not updated when we added multiple resolves?

hundreds-father-404

01/21/2022, 12:36 AM

I get that we might need to still download stuff if you don't already have it downloaded locally. But this line means we will generate the lockfile from scratch

witty-crayon-22786

01/21/2022, 1:13 AM

I'm away from my computer, but it depends how it is being used. If you give the method a lockfile (and only a lockfile) then requirements won't be used afaik

witty-crayon-22786

01/21/2022, 1:14 AM

I believe that all of the callsites which use Materialize use it only for tools with lockfiles

witty-crayon-22786

01/21/2022, 1:14 AM

User requirements are fetched via ClasspathEntryRequest

hundreds-father-404

01/21/2022, 5:07 PM

Ah ha, you're right. That makes more sense. So why do we have entries like this for tools, rather than using a tool lockfile? How do we decide when to do which?

Copy code

# java_parser_launcher.py
ArtifactRequirements.from_coordinates(
        [
            Coordinate(
                group="com.fasterxml.jackson.core", artifact="jackson-databind", version="2.12.4"
            ),
            Coordinate(
                group="com.fasterxml.jackson.datatype",
                artifact="jackson-datatype-jdk8",
                version="2.12.4",
            ),
            Coordinate(
                group="com.github.javaparser",
                artifact="javaparser-symbol-solver-core",
                version="3.23.0",
            ),
        ],
    )

ancient-vegetable-10556

01/21/2022, 5:08 PM

Two reasons

ancient-vegetable-10556

01/21/2022, 5:08 PM

1. java_parser_launcher was written well before tool lockfiles existed

ancient-vegetable-10556

01/21/2022, 5:08 PM

2. it never got updated

hundreds-father-404

01/21/2022, 5:09 PM

So should it be fixed? We're using this pattern in 6 places:

Copy code

src/python/pants/backend/scala/dependency_inference/scala_parser.py
src/python/pants/backend/scala/compile/scalac.py
src/python/pants/backend/scala/goals/repl.py
src/python/pants/backend/codegen/protobuf/scala/rules.py
src/python/pants/backend/java/dependency_inference/java_parser.py
src/python/pants/backend/java/dependency_inference/java_parser_launcher.py

ancient-vegetable-10556

01/21/2022, 5:09 PM

ONE MOMENT 🙂

ancient-vegetable-10556

01/21/2022, 5:11 PM

HOWEVER, this is a weird special case of tool — it’s first-party Pants code, that is written to interface with specialist Python code. It’s not meant to be something that can be adjusted by end users, it’s there to provide core pants functionality

➕ 1

ancient-vegetable-10556

01/21/2022, 5:12 PM

If we can generate a lockfile based on hardcoded requirements like that, that might save some resolve work, and will likely make cold starts more effective, but we do not want to make a thing that end-users can upgrade, because that will probably break our code

ancient-vegetable-10556

01/21/2022, 5:14 PM

So it probably needs a small amount of braining beyond just converting it to use our tool infrastructure

hundreds-father-404

01/21/2022, 5:15 PM

Okay, cool, that makes sense to not want to expose it to users! So then sounds like this code path is correct and shouldn't be changed. Just it's in the wrong place,

coursier_fetch.py

which is now

user_resolves.py

. Probably move it to

jvm_tool.py

. Maybeeee rename to put

Tool

in the name to make this clear? Wdyt?

ancient-vegetable-10556

01/21/2022, 5:18 PM

Hrrrrrrrm. Maybe we need a third file for first-party support code?

hundreds-father-404

01/21/2022, 5:22 PM

This type also is used for tool lockfiles, though.

Copy code

lockfile = await Get(CoursierResolvedLockfile, ValidatedJvmToolLockfileRequest(tool))
    source_files, tool_classpath = await MultiGet(
        ....
        Get(
            MaterializedClasspath,
            MaterializedClasspathRequest(
                lockfiles=(lockfile,),
            ),
        ),
    )

In fact, I'm thinking we might want to consolidate

MaterializedClasspath

ValidatedJvmToolLockfileRequest

+ reading the lockfile with the engine rather than

open()

MaterializedClasspathRequest -> MaterializedClasspath

is what you use to download/set up a tool

ancient-vegetable-10556

01/21/2022, 5:27 PM

I definitely support merging

MaterializedClasspath

ValidatedJvmToolLockfileRequest

— that one exists entirely because constructing the

ArtifactRequirements

for metadata validation needed a rule. Solving a problem with more indirection and all that.

👍 1

Open in Slack

Previous Next