[12610] add versions to lockfiles #12788

chrisjrn · 2021-09-08T20:12:27Z

This adds a version field to lockfiles, starting at version 1, which will allow for simpler validation logic when we start updating the expected metadata in lockfile headers.

This also adds new error messages to LockfileMetadata.from_lockfile

Added per request by @Eric-Arellano in #12782.

# 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]

Eric-Arellano

Thank you for splitting this up!

src/python/pants/backend/python/util_rules/lockfile_metadata.py

Eric-Arellano · 2021-09-08T20:14:38Z

src/python/pants/backend/python/util_rules/lockfile_metadata.py

@@ -38,40 +53,53 @@ def from_lockfile(cls, lockfile: bytes) -> LockfileMetadata:
            elif in_metadata_block:
                metadata_lines.append(line[2:])

+        error_suffix = "To resolve this error, you will need to regenerate the lockfile."


"by running ./pants generate-lockfiles"

Should I mention --resolve={tool_name}?

If possible, yes, that would be excellent! But I'm not sure how easy that will be to wire up.

Thankfully, as easy as it was to wire in the other request you requested.

Eric-Arellano · 2021-09-08T20:16:47Z

src/python/pants/backend/python/util_rules/lockfile_metadata.py

        if not metadata_lines:
            # TODO(#12314): Add a good error.
-            raise InvalidLockfileError("")
+            raise InvalidLockfileError(
+                "Could not find a pants metadata block in this lockfile. " + error_suffix


It would be really helpful to mention the problematic lockfile in all these error messages.

Could not find a Pants metadata block in the lockfile at

It would be. Let me figure out what it would take to be able to plumb that value in.

# 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]

chrisjrn · 2021-09-08T20:50:56Z

@Eric-Arellano How does it look now?

Eric-Arellano · 2021-09-08T21:03:33Z

src/python/pants/backend/python/util_rules/lockfile_metadata.py

    @classmethod
-    def from_lockfile(cls, lockfile: bytes) -> LockfileMetadata:
+    def from_lockfile(
+        cls, lockfile: bytes, lockfile_scope_name: str | None = None


If my read is correct, you always have the lockfile_path available. Only for tool lockfiles do you have the resolve_name (what you call lockfile_scope_name). If you're up for it, the error message would be even better if you could say

Could not find a Pants metadata block in the lockfile {path} for the resolve {resolve_name}.

Or if resolve_name is None,

Could not find a Pants metadata block in the lockfile {path}.

Also please call it resolve_name. This error applies to multiple user lockfiles too, and lockfile_scope_name is a misnomer there. The way to refer to this is resolve_name.

That makes sense. Making that change.

It looks like lockfile_path is only available for non-default lockfiles, so I've added lockfile_path but made it optional.

Eric-Arellano · 2021-09-08T21:04:08Z

src/python/pants/backend/python/util_rules/lockfile_metadata.py

+        if lockfile_scope_name:
+            lockfile_description = f"the lockfile for `{lockfile_scope_name}`"
+        else:
+            """this lockfile."""


This doesn't do anything. Also prefer ternary expressions: https://www.pantsbuild.org/docs/style-guide#prefer-conditional-expressions-ternary-expressions

Thanks for catching the typo

This is now a 4-level conditional, and it reads pretty poorly as a ternary expression, so I've kept it as an if statement for now. Let me know if this is a problem.

Eric-Arellano · 2021-09-08T21:07:27Z

src/python/pants/backend/python/util_rules/pex.py

+    lockfile_scope_name: str | None
+    if isinstance(request.requirements, (ToolDefaultLockfile, ToolCustomLockfile)):
+        lockfile_scope_name = request.requirements.options_scope_name
+    else:
+        lockfile_scope_name = None


As explained above, please call it resolve_name. Also generally prefer ternary expressions: https://www.pantsbuild.org/docs/style-guide#prefer-conditional-expressions-ternary-expressions

resolve_name = ( request.requirements.options_scope_name if isinstance(request.requirements, (ToolDefaultLockfile, ToolCustomLockfile)) else None )

In the future, this will need to capture the resolve name for multiple user lockfiles. It's fine to punt on that, but can you please add something like:

# TODO(#12314): Capture the resolve name for multiple user lockfiles.

Eric-Arellano · 2021-09-08T21:09:31Z

src/python/pants/backend/python/util_rules/lockfile_metadata.py

@@ -53,36 +42,46 @@ def from_lockfile(cls, lockfile: bytes) -> LockfileMetadata:
            elif in_metadata_block:
                metadata_lines.append(line[2:])

-        error_suffix = "To resolve this error, you will need to regenerate the lockfile."
+        error_suffix = "To resolve this error, you will need to regenerate the lockfile by running `./pants generate-lockfiles"


Formatting nit, this is over 100 lines and Black won't attempt to fix. You'd need to use implicit string concatenation.

# 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]

Eric-Arellano · 2021-09-08T22:28:10Z

src/python/pants/backend/python/util_rules/pex.py

-        lockfile_scope_name = request.requirements.options_scope_name
-    else:
-        lockfile_scope_name = None
+    lockfile_scope_name = (


Bump on calling this resolve_name

Missed that change;fixing now

Eric-Arellano

Yay! Thanks :)

# 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]

Eric-Arellano · 2021-09-09T01:10:15Z

Thanks! Can you please cherry-pick to Pants 2.7?

chrisjrn · 2021-09-09T15:11:38Z

@Eric-Arellano Sure thing; PR will be incoming shortly.

This adds a `version` field to lockfiles, starting at version 1, which will allow for simpler validation logic when we start updating the expected metadata in lockfile headers. This also adds new error messages to `LockfileMetadata.from_lockfile`. # Conflicts: # 3rdparty/python/lockfiles/user_reqs.txt # src/python/pants/backend/python/lint/black/lockfile.txt # src/python/pants/backend/python/lint/isort/lockfile.txt # src/python/pants/backend/python/subsystems/lambdex_lockfile.txt # 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]

…12816) See #12788

…uild#12778)) * Add python-docx to the module mapping dictionary ([pantsbuild#12775](pantsbuild#12775)) * Add python-pptx to the module mapping dictionary ([pantsbuild#12776](pantsbuild#12776)) * Add `opencv-python` to the default Python module mapping ([pantsbuild#12777](pantsbuild#12777)) * Add `PyMuPDF` to the default Python module mapping ([pantsbuild#12774](pantsbuild#12774)) * Deprecate `--list-provides` option. ([pantsbuild#12759](pantsbuild#12759)) * Upgrade default `isort` to latest `isort==5.9.3` ([pantsbuild#12756](pantsbuild#12756)) * `OutputPathField.value_or_default()` no longer has an `Address` argument ([pantsbuild#12837](pantsbuild#12837)) * Properly include file dependencies in docker build context ([pantsbuild#12758](pantsbuild#12758)) * DigestSubset should not short-circuit when there are ignores involved. ([pantsbuild#12648](pantsbuild#12648)) * Improve cache reuse for `./pants package` when using a constraints file or lockfile ([pantsbuild#12807](pantsbuild#12807)) * Upgrade to Pex 2.1.48 and leverage packed layout. ([pantsbuild#12808](pantsbuild#12808)) * Fix backports of std lib modules like `dataclasses` not working with dependency inference ([pantsbuild#12818](pantsbuild#12818)) * Warn if `[python-repos]` is set during lockfile generation ([pantsbuild#12800](pantsbuild#12800)) * Add `version` to lockfile metadata headers ([pantsbuild#12788](pantsbuild#12788)) * jvm: add missing space to avoid two words running together ([pantsbuild#12793](pantsbuild#12793)) * Fix a markdown issue in a help string. ([pantsbuild#12766](pantsbuild#12766)) [ci skip-rust]

Christopher Neugebauer added 3 commits September 8, 2021 12:57

Adds "version" field to LockfileMetadata

a1acfcf

# 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]

Updates lockfiles using generate_all_lockfiles.sh

91a756f

# 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]

Update error messages in from_lockfile

fce4cb6

# 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]

chrisjrn requested a review from Eric-Arellano September 8, 2021 20:12

Eric-Arellano reviewed Sep 8, 2021

View reviewed changes

Removes new, adds lockfile scope name to from_lockfile

2c79762

# 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]

Eric-Arellano reviewed Sep 8, 2021

View reviewed changes

Updates error message handling

29a6825

# 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]

Eric-Arellano reviewed Sep 8, 2021

View reviewed changes

Eric-Arellano approved these changes Sep 8, 2021

View reviewed changes

Address further code review concerns

0a08ec7

# 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]

Eric-Arellano approved these changes Sep 8, 2021

View reviewed changes

Christopher Neugebauer added 2 commits September 8, 2021 15:56

Fix lint complaint

32d3bf9

# 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]

fix pex_test

711b9d4

# 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]

Eric-Arellano merged commit 567ece2 into pantsbuild:main Sep 9, 2021

chrisjrn mentioned this pull request Sep 9, 2021

Cherry-pick #12788 to 2.7 branch #12816

Merged

Eric-Arellano pushed a commit that referenced this pull request Sep 9, 2021

Add version to lockfile metadata headers (cherry-pick of #12788) (#…

0a16816

…12816) See #12788

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[12610] add versions to lockfiles #12788

[12610] add versions to lockfiles #12788

chrisjrn commented Sep 8, 2021

Eric-Arellano left a comment

Eric-Arellano Sep 8, 2021

chrisjrn Sep 8, 2021

Eric-Arellano Sep 8, 2021

chrisjrn Sep 8, 2021

Eric-Arellano Sep 8, 2021

chrisjrn Sep 8, 2021

chrisjrn commented Sep 8, 2021

Eric-Arellano Sep 8, 2021

Eric-Arellano Sep 8, 2021

chrisjrn Sep 8, 2021

chrisjrn Sep 8, 2021

chrisjrn Sep 8, 2021

Eric-Arellano Sep 8, 2021

chrisjrn Sep 8, 2021

chrisjrn Sep 8, 2021

chrisjrn Sep 8, 2021

Eric-Arellano Sep 8, 2021

Eric-Arellano Sep 8, 2021

Eric-Arellano Sep 8, 2021

chrisjrn Sep 8, 2021

Eric-Arellano left a comment

Eric-Arellano commented Sep 9, 2021

chrisjrn commented Sep 9, 2021

[12610] add versions to lockfiles #12788

[12610] add versions to lockfiles #12788

Conversation

chrisjrn commented Sep 8, 2021

Eric-Arellano left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

chrisjrn commented Sep 8, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Eric-Arellano left a comment

Choose a reason for hiding this comment

Eric-Arellano commented Sep 9, 2021

chrisjrn commented Sep 9, 2021