Allow removing headers from lockfile #16574
Conversation
This refactor makes the code a little bit cleaner and make adding or using additional options on PythonSetup when working with GeneratePythonLockfile simpler.
| @@ -617,6 +617,15 @@ class PythonSetup(Subsystem): | |||
| ), | |||
| advanced=True, | |||
| ) | |||
| add_headers_to_lockfile = BoolOption( | |||
There was a problem hiding this comment.
Hm, I thought I left a comment but I can't find it. We already have [python].invalid_lockfile_behavior = "ignore". We could reuse that so that when set to ignore, we don't add lockfile headers. There's no reason to add them in that case. It avoids adding Yet Another Option. What do you think?
There was a problem hiding this comment.
tbh, I agree with you that adding more options is bad. we have to many as it is.
however, not sure it is super intuitive to use the invalid_lockfile_behavior option to influence lock file generation....
as far as I can understand from the invalid_lockfile_behavior help it is basically telling pants how to deal with issues in the lock file when trying to read/resolve it...
also looking at the existing options in the subsystem, I think many of them will go away as pants removes support for constraints files and poetry lock files.... so maybe adding an option here is not that bad.
I am conflicted. 🤷🏼♂️
There was a problem hiding this comment.
also looking at the existing options in the subsystem, I think many of them will go away as pants removes support for constraints files and poetry lock files.... so maybe adding an option here is not that bad.
"Go away" might end up meaning "live for a long time in the deprecated section". I've been planning today to deprecate a few options, indeed. Although we're actively having discussions about how Pants needs to give more backwards compatibility #16547, so there's a chance it will live forever.
however, not sure it is super intuitive to use the invalid_lockfile_behavior option to influence lock file generation....
It might not be extremely intuitive, although I do think these options are closely coupled. If you turn off header generation, you must also set invalid_lockfile_behavior to ignore, or Pants will error that the header is missing. That suggests to me these should live in the same option.
While the name "invalid_lockfile_behavior" isn't perfect, basically setting it to "ignore" means to tell Pants to not try to get all fancy telling you when the lockfile is stale.
There was a problem hiding this comment.
as far as I can understand from the
invalid_lockfile_behaviorhelp it is basically telling pants how to deal with issues in the lock file when trying to read/resolve it...
As Eric said: Pants actively consumes the header to decide whether a lockfile is stale ... not just when it is broken/invalid. So stripping the header and/or ignoring a broken header would essentially mean that users would need to manually remember to update lockfiles after requirements/settings edits.
Disabling the header would also mean not being able to take advantage of #15704 (when it is implemented), so the entire lockfile would always be re-written, even after adding a single dep, for example.
For pex based lock files, adding those headers makes the lockfile a non-standard json file which makes it harder to process it by tools that expect json (like pex, for example).
So this PR adds the option to opt out of having those headers.
Fixes: #15740