https://pantsbuild.org/ logo
Join Slack
Powered by
thread re jvm tool lockfile path in pants source (...
# development
f
thread re jvm tool lockfile path in pants source (https://github.com/pantsbuild/pants/pull/13811#discussion_r763288383)
what is the preferred path for such lockfiles? cc @witty-crayon-22786 @hundreds-father-404 @ancient-vegetable-10556
the comment suggests: 
3rdparty/(jvm|tool)/${name}.lockfile
so maybe 
3rdparty/jvm/tool/${TOOL}.lockfile
?
so that we can still use other 
3rdparty/jvm
subpaths as we see fit
w
the 
(jvm|tool)
was sortof meant to suggest either-or… i think the question there is whether there is any benefit to namespacing lockfiles by platform… there may not be
in which case 
3rdparty/tool/black.lockfile
is fine too. there will be subsystem naming helping to enforce encourage no collisions here
h
I suggest that you use default tool lockfiles right next to the tool itself. For Black, we have 
src/.../lint/black/lockfile.txt
The 
3rdparty/
pattern is only for when we use a custom lockfile different than the default. Which iiuc should never happen in pantsbuild/pants - no reason to not use the default given how little JVM code we have
w
@hundreds-father-404: this is for the default value of the option: i.e., where we suggest to put the lockfile if you’ve set a custom version
f
I prefer Eric’s suggestion, which fits with the 
resource
target for the lockfile being next to the same code it is for.
@witty-crayon-22786: the default option value will now just be 
<default>
w
er
h
Oh? Why would we be setting the default value for where the lockfile of a custom version should be? In Python, you use either 
"<default>"
or you choose whatever arbitrary path you want I don't want to be imposing a default location on folks
f
and the existing generate-lockfiles goal will skip lockfiles where the path is 
<default>
w
but
The 
3rdparty/
 pattern is only for when we use a custom lockfile different than the default.
is exactly the relevant case here. this option comes into play when an end user needs to actually generate a lockfile
f
right, then they need to set the lockfile location explicitly
➕ 1
h
to whichever arbitrary path they want
w
why though?
f
because how else should Pants know to prefer the resource over the explicit path?
coke 1
h
I don't see how else it would work? We need a way to say you want to use the default, which is currently the magic string 
"<default>"
. Otherwise, we have to assume it's a custom lockfile
w
because the specified version doesn’t match the builtin version
h
If that happens, then we should eagerly error and say you need to explicitly set the option to whatever location you want. Or, use the default version
f
we don’t match a specified version to the version used to generate the default lockfile
w
as it stands, you have to set two things whenever you change the version: you change the version, and then we say “ah ah ah! you forgot to choose a lockfile location.”
h
This is what Chris's validation code does for Python. We validate that it's safe to use the default, else generate an awesome error message
you have to set two things whenever you change the version: you change the version, and then we say “ah ah ah! you forgot to choose a lockfile location.”
Yep! That's exactly what we want imo! You are opting out of the defaults
w
…twice
f
I am fixing up 
JvmToolBase
currently. Just going to have the paths be the existing resource file locations. If we want to make further changes, they can be a separate PR.
w
i don’t see why you need to opt out twice (with two different options)
h
Because you are changing different things with those two options...one is the path for your lockfile, one is the version to use the tool. They're coupled, but not the same thing
I don't think we should be assuming a default lockfile path to write to
w
i do. ~every other tool does.
cargo, poetry, go, etc
npm, yarn, etc
h
Those also aren't optimized for monorepos and multilingual repos. And they have a single lockfile (ack on Cargo and workspaces)
☝️ 1
a
I think it’s really important for Pants’ usecase that each tool have dependencies that don’t have to change in lockstep with each other
w
sure, and people will have to name lockfiles if they create more than one. but i don’t think that’s a good argument for needing to choose filenames for all of them
h
Setting up a custom lockfile path is a one-time thing done by a codebase admin. For example, Asher set up Toolchain to never use the defaults. Now that that's done, he never needs to worry about it Minimizing boilerplate here seems like the wrong goal fwict. More important is being explicit rather than trying to guess where you want the default
w
Asher set up Toolchain to never use the defaults. Now that that’s done, he never needs to worry about it
hm, no: only half of them are. i made changes to more of them, and some tools still remain as defaults
a
TO be clear, we’re talking about tool lockfiles here, right?
w
yes, tool lockfiles. user lockfiles will need names, although i think that we should add a default location for the first lockfile for each language as well
h
I don't want people to think that they're forced to use a particular setup or reorganize their code. That's already something we are trying really hard to counteract in our docs If you're incrementally adopting Pants and don't use a 
3rdparty/
convention, I don't want people to misunderstand in thinking they need to because that's what Pants defaulted to
w
i am fairly confident that anyone who is disgruntled about the (default) location of the lockfile will be able to find out that you can change it.
h
All in favor of the 
help
message saying something like "A good convention is to put this lockfile in a folder named `3rdparty/jvm`". But making it the default behavior makes it seem like we are forcing you into it imo, and you might not realize it's even an option because why would you bother with it?
w
i think that having a default is preferable. the alternative is boilerplate in 
pants.toml
, and boilerplate in the help
people who don’t care where that file is located (which i expect will be 98% of them) shouldn’t ever need to read the help for that option.
(not to mention that we can delete a lot of handling code for “you forgot to set option X”)
h
Oh hm, I was thinking about how this default will actually work. We can't automatically generate the lockfile as a side-effect yet, you have to run the relevant goal What would that look like with a default path? We eagerly error when consuming the lockfile and tell you to run the goal like normal, then when you run it it uses the default path?
w
correct
h
(not to mention that we can delete a lot of handling code for “you forgot to set option X”)
Almost all the code is checking whether the lockfile is valid or not. It's not checking if the option is set. I don't think that argument is very compelling tbh
w
DEFAULT_TOOL_LOCKFILE
is mentioned 19 times in 5 files (and hardcoded in 2 more)
f
https://github.com/pantsbuild/pants/pull/13822 fixes the immediate brokenness
h
Thanks, Tom. Sounds like we all agree with using 
<default>
as a magic string
w
i agree that we should land https://github.com/pantsbuild/pants/pull/13822 to fix a bug / make things consistent. but i still think that we should add a default for all of these in the medium term, rather than using the 
<default>
magic string
f
it maintains the status quo to use a magic string. but those options could have been 
str | None
and used 
None
as a default
the python options are actually tri-states since they also can use the second magic string 
<none>
👍 1
I chose to require a lockfile for JVM tools and left out support for 
<none>
➕ 1
w
yea, although the 
<none>
case is actually necessary i think
since we don’t have a facility for setting an option to the python literal 
None
f
it is easy to add. instead of requesting a classpath using a lockfile, the artifact requirements can be used, as we were doing prior to introducing tool lockfiles there
also 
<none>
!= None (i.e., 
<default>
)
not specifying the option means use the default embedded lockfile; 
<none>
means don’t use a lockfile whatsoever and re-resolve each time
👍 1
w
one last thought on this for now though: another reason why i think that we should set a default is because of user resolves: https://github.com/pantsbuild/pants/issues/12742
if we don’t have a default for user resolves, every repo will be forced to set one at creation time (which is more obviously unnecessary boilerplate than tools are)
since we will need that convention, it’s another reason for tools having one too
h
yeah that is true on user lockfiles. A few months ago I accepted that as acceptable for initial one-time setup I don't want to die on this hill tho, I'm not blocking with having a default lockfile path
Oh @witty-crayon-22786, very related is https://github.com/pantsbuild/pants/issues/12742. I think that using 
"default"
as the name of the default resolve is a non-starter when we merge Python and Java to use the same resolve universe We could use default names like 
py-default
and 
jvm-default
, although I continue to lean a bit more in favor of approach #2 that you have to give an explicit name
altho I'm willing to rethink that preference. I do recognize that naming things is really hard, so asking you to come up with a name for a lockfile when you just want to try out Pants is an additional barrier to adoption..
f
@hundreds-father-404: re https://github.com/pantsbuild/pants/pull/13777#discussion_r763489820,
Want me to handle merging the two goals? I’m now working on lockfiles as my main project so the context switch isn’t bad for me
yes. I don’t have the context across both JVM and Python lockfiles to figure out what we want here and I don’t have a particular viewpoint, other than having an easy way to make JVM lockfiles which is what I built thus far.
👍 1
and those who have particular thoughts here can figure out the agreed-upon way forward 🙂
w
I do recognize that naming things is really hard, so asking you to come up with a name for a lockfile when you just want to try out Pants is an additional barrier to adoption.
yea. and i think that even if the default name is more awkward, we should not ask users to do this. as mentioned before, i think that 98% of users will be fine with a default, and never think twice about changing it (as evidenced by lockfiles in ~all of the other tools)
👍 1
h
Okay. I'll incorporate into my lockfile plan looking into what a default path would look like Still scoping the best way to approach this project, whether I start with: 1) unifying everything into 
generate-lockfiles
2) hooking up 
compatible_resolves
3) switching to pex for lockfile generation 4) Stu's proposal for disambiguating dep inference
Coming full circle...I continue to think it's a good idea for tool lockfiles to behave the same as now, where you either use the magic string 
<default>
or you set a custom path. That has worked really well for Python it seems But for user lockfiles, I agree that we should set a default like 
{"jvm": "3rdparty/jvm/artifacts.lockfile"}
. That's necessary for Pants to work out-of-the-box -- What should that default path be? I don't know what's idiomatic in JVM
w
yea, that makes sense.
there is no idiom for lockfiles on the JVM
range resolves are rare, which is one of the primary motivating factors (obviously there are others, but that’s one of the big ones in other ecosystems)
👍 1
h
k. I like the 
3rdparty/jvm
part, but any opinion 
artifacts.lockfile
part? In Python, we're using 
.txt
-- also why are we doing 
.lockfile
instead of 
.json
?
w
so i think that something like 
3rdparty/jvm/artifacts.lockfile
should be fine.
re the lockfile suffix: i don’t know. it could be 
json
, but since it’s not actually meant for hand-editting, the need for syntax highlighting is reduced a bit
👍 1
h
how about 
{jvm: 3rdparty/jvm/lockfile.json, py: 3rdparty/py/lockfile.txt}
?
w
sure.
a
I tend to disagree — the JSON here is actually encoding for a file format, it’s not arbitrary JSON
h
Meaning, you are +1 or -1 for ending in 
.json
?
a
I am -1 for ending in 
.json
if it’s not arbitrary JSON
h
Oh hm, Rust and Yarn both end in 
.lock
. Sg But 
3rdparty/jvm/lockfile.lock
is redudant. Any suggestions? 
artifacts.lock
seems fine other than upsetting non-American English speakers 🙈
a
We might want to go with 
.pantslock
or 
.jvmlock
artifacts.lock
is not correct, as it also contains dependencies of artifacts 😄
👍 1
w
i think that putting 
default
in the name might make sense. so 
default.lock
maybe. because users using multiple resolves will need to choose other names for the rest
➕ 1
a
(which are artifacts, but not ones that are declared or accessible to anything)
h
k, then should the resolve be called 
jvm-default
or just 
jvm
?
I guess if it's the default, you're not writing it out very frequently. So the extra characters isn't that offensive. Meaning I think 
jvm-default
👍 1
Open in Slack
PreviousNext
Powered by