Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[internal] Rename ./pants lock to ./pants generate-user-lockfile and ./pants tool-lock to ./pants generate-lockfiles #12641

Merged
merged 3 commits into from
Aug 26, 2021

Conversation

Eric-Arellano
Copy link
Contributor

@Eric-Arellano Eric-Arellano commented Aug 25, 2021

We currently have separate goals for user lockfiles vs tool lockfiles because the implementations are different right now, e.g. specs being different. It's easier to keep separate while iterating. We will unify them, but are not yet ready.

We want to release tool lockfiles as stable in Pants 2.7, so it should be using the final proper name of ./pants generate-lockfiles rather than ./pants tool-lock.

Once user lockfiles are stable in 2.8, we will kill generate-user-lockfile in favor of a unified generate-lockfiles goal.

generate-lockfiles vs. lock vs resolve vs. something else

See #12641 (comment) for why I went with generate-lockfiles, and the above thread for alternative proposals.

[ci skip-rust]
[ci skip-build-wheels]

…tool-lock` to `./pants lock`

# Rust tests and lints will be skipped. Delete if not intended.
[ci skip-rust]

# Building wheels and fs_util will be skipped. Delete if not intended.
[ci skip-build-wheels]
@benjyw
Copy link
Contributor

benjyw commented Aug 25, 2021

So the plan is that once user lockfiles are stable as a feature they too will use lock, and not user-lock ?

@Eric-Arellano
Copy link
Contributor Author

Yes, exactly.

@cczona
Copy link
Member

cczona commented Aug 25, 2021

I know it's a lockfile, so lock makes a certain amount of sense. But as a purely semantic issue, I'm concerned that "lock" as a verb sounds like more potent security than what this is. For that reason, I'd vote for resolve or a fourth option.

@tdyas
Copy link
Contributor

tdyas commented Aug 25, 2021

Rust uses cargo update.

@jsirois
Copy link
Contributor

jsirois commented Aug 25, 2021

Why is it an explicit lock is even a goal long term? I may have missed that discussion. Presumably though, if I opt in to locks:

  1. Any needed locks do not exist - I create them automatically as part of the rule chain that resolves things to achieve some other goal.
  2. I run ./pants {update-locks, update, ...?} if I have locks and want to regenerate them (in part if we end up supporting that) or in whole.

@Eric-Arellano
Copy link
Contributor Author

Good point @jsirois. Indeed, I think lock is possibly temporary as we punt on #12014. update-locks would make more sense than lock, similar to cargo update.

Still, in the meantime we have to choose lock vs resolve vs python-lock vs something else.

@kaos
Copy link
Member

kaos commented Aug 25, 2021

I'll just toss in my 2 cents here..

TL;DR;
My vote would be for update-lockfile.

Longer version:

I kind of like how make addresses this, so with that as inspiration, it would look like this

./pants make 3rdparty/python/locks/black.txt

However, "make" doesn't really fit well here.. so I think "update" makes more sense. "tool-lock" or just "lock" doesn't really signal to me that we're actually going to refresh the lock file. Maybe if the provided target was a requirements file, "lock" signals that taken the requirements, we lock them down, producing a lockfile. That has bearing in my head.

So, if the target is a lockfile, then "update" or "refresh" or something along those lines makes more sense imo. Could also consider the pip verb "freeze" (but that is arguably interchangeable with "lock").

Without any target however, a bare "update" doesn't cover it, then it would have to convey more in the goal verb itself, such as "update-lockfile" (I think using "lockfile" rather than just the shorter "lock" also gives a clearer picture of what it refers to, as "lock" is a bit too abstract).

Also, I think update-lockfile ought to work regardless of target language, so wouldn't have to add python in there, imo, as long as the implementation is able to discern what to do, based on the target lockfile itself.

@chrisjrn
Copy link
Contributor

Why is it an explicit lock is even a goal long term? I may have missed that discussion. Presumably though, if I opt in to locks:

  1. Any needed locks do not exist - I create them automatically as part of the rule chain that resolves things to achieve some other goal.
  2. I run ./pants {update-locks, update, ...?} if I have locks and want to regenerate them (in part if we end up supporting that) or in whole.

Producing a user lockfile is producing an artefact that people will check into source control, which is not necessarily true of rule chains that resolve for other goals (yes for fmt, probably not for build).

I am less in favour of update-lockfile (or things that insofar as lockfiles may not already be in place when it's first run, but that may just be me overthinking things

@kaos
Copy link
Member

kaos commented Aug 25, 2021

[...] lockfiles may not already be in place when it's first run

Very good point.

I agree, that when a lockfile is not present, update doesn't really fit either.. (overlooked that scenario). So, what you guys feel for generate-lockfiles then..?
This goes the other way around, that in cases that the files do exist, it ought to rather be regenerate-lockfiles, however, simply "generate" can still work, if we treat the existing files as being discardable and of no concern, and that we simply replace them with the newly generated ones.

@chrisjrn
Copy link
Contributor

continuing the bikeshedding: how about pin-dependencies?

@Eric-Arellano
Copy link
Contributor Author

So, what you guys feel for generate-lockfiles then..?

I like it! I was typing up this proposal but had to leave for the doctor the same time you sent in your suggestion :)

Per comment in #12591 (comment), I think it's plausible we'll want to only have ./pants update-lockfiles in the future because generation is automatic. But for now, generate-lockfiles is more accurate than update-lockfiles because this is the only way to generate. We can deprecate generate-lockfiles in favor of update-lockfiles when all the required work is landed for those semantics.

I also like that there's no ambiguity what this will do, unlike a vague lock or resolve. This is hopefully not a goal you need to run frequently, so the extra length is fine imo.

Producing a user lockfile is producing an artefact that people will check into source control, which is not necessarily true of rule chains that resolve for other goals (yes for fmt, probably not for build).

@chrisjrn Once #12014 is figured out, running ./pants fmt will generate/update the relevant tool lockfiles if necessary as a side effect. This is how Cargo does things, and it works well.

@Eric-Arellano Eric-Arellano changed the title [internal] Rename ./pants lock to ./pants user-lock and ./pants tool-lock to ./pants lock [internal] Rename ./pants lock to ./pants generate-user-lockfile and ./pants tool-lock to ./pants generate-lockfiles Aug 26, 2021
# Rust tests and lints will be skipped. Delete if not intended.
[ci skip-rust]

# Building wheels and fs_util will be skipped. Delete if not intended.
[ci skip-build-wheels]
# Rust tests and lints will be skipped. Delete if not intended.
[ci skip-rust]

# Building wheels and fs_util will be skipped. Delete if not intended.
[ci skip-build-wheels]
Copy link
Member

@stuhood stuhood left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

These longer generate-* names are fairly clearly temporary, so I'm fine with this until the semantics are nailed down.

@Eric-Arellano Eric-Arellano merged commit 86bc802 into pantsbuild:main Aug 26, 2021
@Eric-Arellano Eric-Arellano deleted the lock-goal branch August 26, 2021 23:37
@illicitonion illicitonion mentioned this pull request Aug 30, 2021
illicitonion added a commit to illicitonion/pants that referenced this pull request Aug 30, 2021
* Clean up the config parsing code. ([pantsbuild#12678](pantsbuild#12678))

* [internal] `--generate-lockfiles-resolve` does not determine `PythonLockfileRequest` for unspecified tools ([pantsbuild#12692](pantsbuild#12692))

* [internal] Add missing `resources` targets for default tool lockfiles ([pantsbuild#12670](pantsbuild#12670))

* [internal] Reorganize lockfile code to no longer be in `backend/python/experimental` ([pantsbuild#12669](pantsbuild#12669))

* [internal] Test that default tool lockfiles work on macOS ([pantsbuild#12666](pantsbuild#12666))

* [internal] Improve the error message for stale/invalid lockfiles ([pantsbuild#12618](pantsbuild#12618))

* [internal] Rename `./pants lock` to `./pants generate-user-lockfile` and `./pants tool-lock` to `./pants generate-lockfiles` ([pantsbuild#12641](pantsbuild#12641))

* [internal] Only run macOS tests in CI annotated with `@pytest.mark.platform_specific_behavior` ([pantsbuild#12665](pantsbuild#12665))

* [internal] Test that Python tools work with all expected interpreter versions ([pantsbuild#12656](pantsbuild#12656))

* [internal] Factor out a helper rule for setting up `--resolve-all-constraints` ([pantsbuild#12630](pantsbuild#12630))

* [internal] Add links to changelog and Twitter in Pants PyPI project page. ([pantsbuild#12636](pantsbuild#12636))

* [internal] Update CoC with proper markdown + https links. ([pantsbuild#12637](pantsbuild#12637))

* GitHub issue templates ([pantsbuild#12617](pantsbuild#12617))

* Register subsystems and target types by backend/plugin. ([pantsbuild#12623](pantsbuild#12623))

* [internal] Sort input requirements for `LockfileMetadata` ([pantsbuild#12615](pantsbuild#12615))

* [internal] Expect lockfile metadata to be defined ([pantsbuild#12616](pantsbuild#12616))

* Simplify subsystem registration. ([pantsbuild#12620](pantsbuild#12620))

* [internal] Use constants for magic strings `<none>` and `<default>` for tool lockfiles ([pantsbuild#12613](pantsbuild#12613))

* [internal] Refactor `lockfile_metadata.py` to be more class-based ([pantsbuild#12611](pantsbuild#12611))

* Embrace that help strings are markdown. ([pantsbuild#12606](pantsbuild#12606))

* Stop fetching a lockfile request just to figure out the requirements digest ([pantsbuild#12607](pantsbuild#12607))

* [internal] Refactor MyPy first party plugins ([pantsbuild#12599](pantsbuild#12599))

* [internal] Refactor MyPy config file setup ([pantsbuild#12593](pantsbuild#12593))

* [internal] use `is_union()` rather than `union.is_instance()`. ([pantsbuild#12595](pantsbuild#12595))

* fs_util can use mTLS and ignore certificates ([pantsbuild#12586](pantsbuild#12586))

* [internal] Fix Pylint lockfile to actually be wired up ([pantsbuild#12590](pantsbuild#12590))

* [internal] Update lockfile TODOs with current state of project ([pantsbuild#12592](pantsbuild#12592))

* [internal] Check for lockfile staleness by using context's interpreter constraints, rather than global constraints  ([pantsbuild#12566](pantsbuild#12566))

* [internal] Refactor Pylint first-party plugins ([pantsbuild#12583](pantsbuild#12583))

* [internal] Fix Pytest and IPython to check for stale lockfiles ([pantsbuild#12582](pantsbuild#12582))

* [internal] Do not cache lockfile generation ([pantsbuild#12674](pantsbuild#12674))
illicitonion added a commit that referenced this pull request Aug 30, 2021
* Clean up the config parsing code. ([#12678](#12678))

* [internal] `--generate-lockfiles-resolve` does not determine `PythonLockfileRequest` for unspecified tools ([#12692](#12692))

* [internal] Add missing `resources` targets for default tool lockfiles ([#12670](#12670))

* [internal] Reorganize lockfile code to no longer be in `backend/python/experimental` ([#12669](#12669))

* [internal] Test that default tool lockfiles work on macOS ([#12666](#12666))

* [internal] Improve the error message for stale/invalid lockfiles ([#12618](#12618))

* [internal] Rename `./pants lock` to `./pants generate-user-lockfile` and `./pants tool-lock` to `./pants generate-lockfiles` ([#12641](#12641))

* [internal] Only run macOS tests in CI annotated with `@pytest.mark.platform_specific_behavior` ([#12665](#12665))

* [internal] Test that Python tools work with all expected interpreter versions ([#12656](#12656))

* [internal] Factor out a helper rule for setting up `--resolve-all-constraints` ([#12630](#12630))

* [internal] Add links to changelog and Twitter in Pants PyPI project page. ([#12636](#12636))

* [internal] Update CoC with proper markdown + https links. ([#12637](#12637))

* GitHub issue templates ([#12617](#12617))

* Register subsystems and target types by backend/plugin. ([#12623](#12623))

* [internal] Sort input requirements for `LockfileMetadata` ([#12615](#12615))

* [internal] Expect lockfile metadata to be defined ([#12616](#12616))

* Simplify subsystem registration. ([#12620](#12620))

* [internal] Use constants for magic strings `<none>` and `<default>` for tool lockfiles ([#12613](#12613))

* [internal] Refactor `lockfile_metadata.py` to be more class-based ([#12611](#12611))

* Embrace that help strings are markdown. ([#12606](#12606))

* Stop fetching a lockfile request just to figure out the requirements digest ([#12607](#12607))

* [internal] Refactor MyPy first party plugins ([#12599](#12599))

* [internal] Refactor MyPy config file setup ([#12593](#12593))

* [internal] use `is_union()` rather than `union.is_instance()`. ([#12595](#12595))

* fs_util can use mTLS and ignore certificates ([#12586](#12586))

* [internal] Fix Pylint lockfile to actually be wired up ([#12590](#12590))

* [internal] Update lockfile TODOs with current state of project ([#12592](#12592))

* [internal] Check for lockfile staleness by using context's interpreter constraints, rather than global constraints  ([#12566](#12566))

* [internal] Refactor Pylint first-party plugins ([#12583](#12583))

* [internal] Fix Pytest and IPython to check for stale lockfiles ([#12582](#12582))

* [internal] Do not cache lockfile generation ([#12674](#12674))
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants