https://pantsbuild.org/ logo
Powered by
h

hundreds-father-404

01/21/2022, 12:33 AM
@witty-crayon-22786 @ancient-vegetable-10556 do you know if this JVM code is right?
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
Fwict with Git blame, this code is old. Perhaps it was not updated when we added multiple resolves?
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
w

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
I believe that all of the callsites which use Materialize use it only for tools with lockfiles
User requirements are fetched via ClasspathEntryRequest
h

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",
            ),
        ],
    )
a

ancient-vegetable-10556

01/21/2022, 5:08 PM
Two reasons
1. java_parser_launcher was written well before tool lockfiles existed
2. it never got updated
h

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
a

ancient-vegetable-10556

01/21/2022, 5:09 PM
ONE MOMENT 🙂
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
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
So it probably needs a small amount of braining beyond just converting it to use our tool infrastructure
h

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?
a

ancient-vegetable-10556

01/21/2022, 5:18 PM
Hrrrrrrrm. Maybe we need a third file for first-party support code?
h

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
a

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
Powered by