Add new version of `LockfileMetadata` to support checking for identically specified requirements #12782

chrisjrn · 2021-09-08T16:52:40Z

This factors out versioning capabilities into LockfileMetadata so that it's possible to easily change the set of validation requirements for a lockfile. V1 represents the original lockfile version (where constraints and an invalidation digest are set). V2 allows for the old behaviour, but also allows specifying the input requirements for a lockfile, and verifying that the user requirements are ~~a non-strict subset of~~ identical to the input requirements.

We decided to replace the requirements hex digest with requirements strings to allow us to test whether the lockfile produces a compatible environment rather than an identical environment, which will be useful for user lockfile support when we eventually enable that. In the meantime, tool lockfiles still test for an identical environment, but the extra data in the lockfile will allow for more fine-grained error messages in a future version.

The implementations of _from_json_dict and is_valid_for are a bit repetitive; I can factor out the common behaviour with a bit of work, but given we expect to delete the V1 implementation before too long.

Currently this does not add the platforms capability to the header, but now it's going to be easy enough to bump the version number if we want to add more fields.

Closes #12610

chrisjrn · 2021-09-08T18:58:15Z

src/python/pants/backend/python/goals/lockfile.py

+    metadata = LockfileMetadata.new(
+        req.requirements_hex_digest,
+        req.interpreter_constraints,
+        {Requirement.parse(i) for i in req.requirements},


I'm not absolutely convinced that this is the best place to put this parsing logic. It may be worth moving it over to new at some point, but for now, this works.

This can fail and lead to confusing error messages. We probably want to catch those and enrich the error, but not clear to me the best way to do that. Mind adding a TODO(#12314) please? (No need to fix now)

Adding ~~an error~~ a TODO in here. As mentioned on the other location where I'm using .parse, the exception that gets raised doesn't play nice with mypy, and that's going to get in the way of improving this. I'm going to return to this in the future.

Eric-Arellano

Thanks, Chris.

As discussed in #12683, I originally thought we might not want to put version numbers in the lockfile metadata, even it makes total sense to do internally. My main thought is it might suggest we support more versions than the current and latest - but I realize now that's not a good reason. It's valid for Pants 2.11 to only support lockfile versions 3 and 4, for example. And I think that is more clear. So, I think I now agree with your decision to surface version.

If we do add version, I think we want to update 2.7 to set version = 1 before the stable release goes out. We still have time to make that breaking change. Could you please split out this PR into that prework—i.e. not do the "compatible" requirements change yet—so we can cherry-pick?

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

This permits creating new versions of `LockfileMetadata` with different data requirements with minimal changes to the external API. Generally speaking, end-users should interact with `LockfileMetadata` and not the concrete subclasses. [ci skip-build-wheels] [ci skip-rust]

[ci skip-rust] [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]

chrisjrn · 2021-09-08T22:58:53Z

Updated description to refer to the fact that the original version is now called "V1" rather than "V0", and the former V1 is now V2.

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

…ible-requirements-only [ci skip-rust] [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]

chrisjrn · 2021-09-09T16:27:15Z

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

@@ -612,6 +612,7 @@ def _validate_metadata(
        requirements.lockfile_hex_digest,
        request.interpreter_constraints,
        python_setup.interpreter_universe,
+        {},  # TODO(chrisjrn): include requirements strings here


At the moment I'm not passing the actual requirements strings in, because we're only validating for tool lockfiles. This will change once we start using user lockfiles (which themselves send down requirements strings)

Eric-Arellano · 2021-09-09T16:29:29Z

Thanks Chris! Before I look more closely at the code, I wanted to revisit the metadata format discussion. We've talked in the past about how the digest is only useful for tool lockfiles, whereas the list of requirements is only necessary for user lockfiles (but maybe still insightful for tool lockfiles?)

Now that you've landed the version argument, we also have a lot more flexibility. Previously, I suggested that we use the metadata headers as a proxy for the metadata version. That's no longer necessary.

This means that we could do a scheme like:

// Tool lockfile
{
  "invalidation_hash": "abc",
  "interpreter_constraints": ["==3.9.*"],
  "version": 2
}

// User lockfile
{
  "requirements": ["foo"],
  "interpreter_constraints": ["==3.9.*"],
  "version": 2
}

Or, to be more verbose:

// Tool lockfile
{
  "invalidation_hash": "abc",
  "requirements": null,
  "interpreter_constraints": ["==3.9.*"],
  "version": 2
}

// User lockfile
{
  "invalidation_hash": null,
  "requirements": ["foo"],
  "interpreter_constraints": ["==3.9.*"],
  "version": 2
}

Or, your format where you set all the metadata even if it's not applicable.

With this discussion, it's worth considering how we'll handle platforms, which is mutually exclusive with interpreter constraints.

What are your thoughts on the format? I personally think I prefer the simplest: only put metadata for what the caller actually needs. Keep our Pants boilerplate to a minimum.

…eing more verbose # 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-09T16:51:25Z

My previous suggestion on the original ticket was to include a purpose field as well as a version field, which IMO will make it easier for us to document what the correct set of fields are, once we have a docs page for users about how to generate their lockfiles. I might be wrong about that though.

I think there are two pertinent questions here:

Do we always want to store generate-time requirements, even if we're using digests? I think the answer here is yes, because if we have access to the requirements strings for a given target, then we can give a more meaningful error message (by stating what the diverging requirements are).
What do we do with platforms? If we're certain that these are mutually exclusive with interpreter constraints then it makes sense to have only one or the other. Is there a situation where we want to accept a platform and then exclude specific interpreter constraints? I could think of having e.g. platform macosx_11_0_arm64-cp-39-cp39 but needing to exclude a specific version (e.g. CPython==3.9.4) because there's a severe bug in it. Wouldn't making platforms and ICs mutually exclusive prevent expressing that description?

Eric-Arellano · 2021-09-09T16:54:31Z

I think the answer here is yes, because if we have access to the requirements strings for a given target, then we can give a more meaningful error message (by stating what the diverging requirements are).

Makes sense. At that point, I suggest getting rid of the digest. It's no longer necessary - you can do exact string comparisons for tool lockfiles, whereas compatible check for user lockfiles. The digest maybe improves perf, but feels like a micro-optimization and we should go with a simpler lockfile format. Wdyt?

chrisjrn · 2021-09-09T17:32:02Z

That's a fair point. If we do go with that approach, then we're going to need to add a new field to specify whether we want an exact requirements match, or whether we want to allow only compatible requirements. I considered whether purpose might be enough for us here, but it's not clear that all tools are going to want exact requirements into the future (e.g. flake8 may like extra user requirements set).

Having either the digest or the requirements strings makes it clear which behaviour is going to be applied; having only strings is going to require that behaviour flag on the side.

Also, did you have any thoughts about platforms vs ICs?

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

…ents strings instead # 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]

chrisjrn · 2021-09-10T15:12:51Z

For paper trail purposes -- we've decided to remove the requirements_invalidation_digest, and we're just going to with a list of requirement string literals. Eric was satisfied that we could determine the matching behaviour by the context in which the lockfile is consumed, either way, we don't need to support user lockfiles until V3 any way.

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

…ible-requirements-only # Conflicts: # 3rdparty/python/lockfiles/user_reqs.txt

chrisjrn · 2021-09-10T15:34:07Z

src/python/pants/backend/python/subsystems/python_tool_base.py

@@ -160,6 +161,7 @@ def pex_requirements(
                    importlib.resources.read_binary(*self.default_lockfile_resource),
                ),
                lockfile_hex_digest=hex_digest,


n.b. We still calculate the hex digest while V1 lockfiles are still supported.

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

…r requirements mismatches # 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]

chrisjrn · 2021-09-10T18:24:33Z

@Eric-Arellano This implements the lockfile metadata per our discussion yesterday and is probably ready for review

Eric-Arellano

Thanks! Could you please update the PR description to explain how we reached this decision about removing the hash and using a list of requirements?

Eric-Arellano · 2021-09-10T21:37:57Z

src/python/pants/backend/python/util_rules/lockfile_metadata_test.py.orig

@@ -0,0 +1,142 @@
+# Copyright 2021 Pants project contributors (see CONTRIBUTORS.md).


Wait I'm confused still - what is this being used for?

I could swear I deleted this. FIXING.

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

Eric-Arellano · 2021-09-10T21:44:15Z

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

@@ -527,10 +530,17 @@ def _validate_metadata(
    python_setup: PythonSetup,
 ) -> None:

+    req_strings = (
+        {Requirement.parse(i) for i in requirements.req_strings}


Ditto that this can fail, and please adding a TODO to improve the error message.

The error that comes up here is pkg_resources.extern.packaging.requirements.InvalidRequirement, which mypy balks at:

src/python/pants/backend/python/util_rules/pex.py:19:1: error: Skipping analyzing "pkg_resources.extern.packaging.requirements": found module but no type hints or library stubs [import]

I'll add the TODO, but it may end up taking some work to actually improve this error message.

Yeah, we handle it like this:

pants/src/python/pants/backend/python/target_types.py

Lines 807 to 824 in fd71177

def parse_requirements_file(content: str, *, rel_path: str) -> Iterator[Requirement]:

"""Parse all `Requirement` objects from a requirements.txt-style file.

This will safely ignore any options starting with `--` and will ignore comments. Any pip-style

VCS requirements will fail, with a helpful error message describing how to use PEP 440.

"""

for i, line in enumerate(content.splitlines()):

line = line.strip()

if not line or line.startswith("#") or line.startswith("-"):

continue

try:

yield Requirement.parse(line)

except Exception as e:

raise ValueError(

format_invalid_requirement_string_error(

line, e, description_of_origin=f"{rel_path} at line {i + 1}"

)

)

Oh, so we're just handling the generic exception? That makes sense.

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

Eric-Arellano · 2021-09-10T21:53:20Z

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

+        This static method should be used in place of the `LockfileMetadata` constructor.
+        """
+
+        return LockfileMetadataV2(valid_for_interpreter_constraints, requirements)


I'm a little confused on this part. Why is it hardcoded to V2?

new should produce the current version of the metadata header, but this means that we only have to update which constructor is called once.

To clarify: This keeps the API to places that need to create a new LockfileMetadata object reasonably consistent. We don't want to support creating older versions, but we do want to support deserialising them until the deprecation process is complete. Having the new method in place means the factory method has a consistent name, and consumers will get directed to new parameters that need to be added by the type checker. I suspect it's a lower support burden than trying to keep track of which version of the class is the current one.

Oh! Okay that last paragraph makes way more sense to me. Thanks for explaining. Could you please add that explanation to the docstring: when writing lockfiles, we always use the latest metadata version, even if we support reading older versions.

Well, I guess you do already say "the most recent version". I'm trying to figure out why this was still confusing me. Maybeee something like this:

Create an instance the most up-to-date subclass of LockfileMetadata.

Note that we always use the most recent lockfile version when writing lockfiles, even if we support reading older versions.

This static method should be used in place of the `LockfileMetadata` constructor. This gives calling sites a predictable method to call to construct a new `LockfileMetadata` for writing, while still allowing us to support _reading_ older, deprecated metadata versions.

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

…quirements` # 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]

…ible-requirements-only # Conflicts: # 3rdparty/python/lockfiles/user_reqs.txt # 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]

Eric-Arellano · 2021-09-14T15:44:12Z

3rdparty/python/lockfiles/flake8.txt

@@ -4,10 +4,14 @@
 #
 # --- BEGIN PANTS LOCKFILE METADATA: DO NOT EDIT OR REMOVE ---
 # {
-#   "version": 1,
-#   "requirements_invalidation_digest": "815457a1baf6226c993e5468ccdf64c69fe7214d3d9237911c762733e0130526",
+#   "version": 2,
 #   "valid_for_interpreter_constraints": [


This name has grown on me btw :) especially when paired with generated_with_requirements.

The only reason why naming things is a hard problem in computer science is because people insist on being concise 😆

Eric-Arellano · 2021-09-14T15:46:13Z

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

+        This static method should be used in place of the `LockfileMetadata` constructor.
+        """
+
+        return LockfileMetadataV2(valid_for_interpreter_constraints, requirements)


Well, I guess you do already say "the most recent version". I'm trying to figure out why this was still confusing me. Maybeee something like this:

Create an instance the most up-to-date subclass of LockfileMetadata.

Note that we always use the most recent lockfile version when writing lockfiles, even if we support reading older versions.

Eric-Arellano · 2021-09-14T15:47:19Z

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

+    ) -> _T:
+        """Construct a `LockfileMetadata` subclass from the supplied JSON dict.
+
+        *** Not implemented. Subclasses should override. ***


Hm, imo better is to set @abstractmethod and subclass ABC? MyPy doesn't like the interaction of ABC and dataclasses - if it complains, use # type: ignore[error-code].

It's complaining, and it breaks the type checking in subclasses too. That's a bit too much complaining for me to be comfortable with suppressing.

Eric-Arellano · 2021-09-14T15:47:33Z

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

+    def _header_dict(self) -> dict[Any, Any]:
+        """Produce a dictionary to be serialized into the lockfile header.
+
+        Subclasses should call `super` and update the resulting dictionary


Suggested change

Subclasses should call `super` and update the resulting dictionary

Subclasses should call `super` and update the resulting dictionary.

Eric-Arellano · 2021-09-14T15:48:34Z

src/python/pants/backend/python/util_rules/lockfile_metadata_test.py.orig

@@ -0,0 +1,142 @@
+# Copyright 2021 Pants project contributors (see CONTRIBUTORS.md).


Wait I'm confused still - what is this being used for?

# 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-14T18:13:09Z

Thanks! Note that we will not cherry-pick this to Pants 2.7 @chrisjrn

chrisjrn requested a review from Eric-Arellano September 8, 2021 16:52

chrisjrn commented Sep 8, 2021

View reviewed changes

Eric-Arellano reviewed Sep 8, 2021

View reviewed changes

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 mentioned this pull request Sep 8, 2021

[12610] add versions to lockfiles #12788

Merged

Christopher Neugebauer added 7 commits September 8, 2021 13:50

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]

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]

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]

Add tests for new V2 metadata.

b416cb9

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

Documentation cleanups

2acde95

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

Fix pex_test

1ea583a

# 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 force-pushed the chrisjrn/12610-compatible-requirements-only branch from 93bbf5b to 1ea583a Compare September 8, 2021 22:54

Christopher Neugebauer added 5 commits September 8, 2021 16:55

fix pex_test

24b122c

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

Merge remote-tracking branch 'origin/main' into chrisjrn/12610-compat…

9acafa6

…ible-requirements-only [ci skip-rust] [ci skip-build-wheels]

Commit correct lockfiles

673bfb2

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

Make the lockfiles script work properly on mac os

e8762cd

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

Do not check for lockfile staleness if no requirements info is provided

b05167c

# 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 commented Sep 9, 2021

View reviewed changes

Makes generate_all_lockfiles.sh pass shellcheck at the expense of b…

7c529e7

…eing more verbose # 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]

Christopher Neugebauer added 3 commits September 9, 2021 10:38

prod CI

b7847c2

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

Make V2 no longer rely on the digest, and start using actual requirem…

184b645

…ents strings instead # 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]

Adds new versions of lockfiles _without_ the digest any more

667d4db

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

Christopher Neugebauer added 2 commits September 10, 2021 08:28

Adds more comprehensive and useful tests for V2 metadata

7f7b2d1

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

Merge remote-tracking branch 'origin/main' into chrisjrn/12610-compat…

a20a83b

…ible-requirements-only # Conflicts: # 3rdparty/python/lockfiles/user_reqs.txt

chrisjrn commented Sep 10, 2021

View reviewed changes

chrisjrn marked this pull request as draft September 10, 2021 15:51

Christopher Neugebauer added 2 commits September 10, 2021 08:52

Pre-code review tidy up

046d027

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

Tests both versions of metadata in pex_test, adds error messages fo…

54d032c

…r requirements mismatches # 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 marked this pull request as ready for review September 10, 2021 16:38

Fix typo in tests

7f90722

# 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 changed the title ~~Add new version of LockfileMetadata to support checking for compatible requirements~~ Add new version of LockfileMetadata to support checking for ~compatible~ identically specified requirements Sep 10, 2021

chrisjrn changed the title ~~Add new version of LockfileMetadata to support checking for ~compatible~ identically specified requirements~~ Add new version of LockfileMetadata to support checking for identically specified requirements Sep 10, 2021

chrisjrn requested a review from Eric-Arellano September 10, 2021 18:24

Eric-Arellano reviewed Sep 10, 2021

View reviewed changes

Christopher Neugebauer added 3 commits September 13, 2021 08:40

Address code review concerns

c06ccfb

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

Renames requirements field in lockfile header to `generated_with_re…

435d747

…quirements` # 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]

Merge remote-tracking branch 'origin/main' into chrisjrn/12610-compat…

8b1113e

…ible-requirements-only # Conflicts: # 3rdparty/python/lockfiles/user_reqs.txt # Building wheels and fs_util will be skipped. Delete if not intended. [ci skip-build-wheels]

chrisjrn requested a review from Eric-Arellano September 13, 2021 16:01

Christopher Neugebauer added 2 commits September 13, 2021 09:12

Add TODO for error-prone requirements parsing code

c5b2b31

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

(lint)

c360f50

# 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 14, 2021

View reviewed changes

Addresses code review concerns

531664a

# 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 14, 2021 17:03

Eric-Arellano approved these changes Sep 14, 2021

View reviewed changes

Eric-Arellano merged commit e1dd981 into pantsbuild:main Sep 14, 2021

chrisjrn deleted the chrisjrn/12610-compatible-requirements-only branch September 14, 2021 20:00

		@@ -0,0 +1,142 @@
		# Copyright 2021 Pants project contributors (see CONTRIBUTORS.md).

	def parse_requirements_file(content: str, *, rel_path: str) -> Iterator[Requirement]:
	"""Parse all `Requirement` objects from a requirements.txt-style file.

	This will safely ignore any options starting with `--` and will ignore comments. Any pip-style
	VCS requirements will fail, with a helpful error message describing how to use PEP 440.
	"""
	for i, line in enumerate(content.splitlines()):
	line = line.strip()
	if not line or line.startswith("#") or line.startswith("-"):
	continue
	try:
	yield Requirement.parse(line)
	except Exception as e:
	raise ValueError(
	format_invalid_requirement_string_error(
	line, e, description_of_origin=f"{rel_path} at line {i + 1}"
	)
	)

	Subclasses should call `super` and update the resulting dictionary
	Subclasses should call `super` and update the resulting dictionary.

Add new version of LockfileMetadata to support checking for identically specified requirements #12782

Add new version of LockfileMetadata to support checking for identically specified requirements #12782

Conversation

chrisjrn commented Sep 8, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

chrisjrn Sep 13, 2021 • edited Loading

Choose a reason for hiding this comment

Eric-Arellano left a comment

Choose a reason for hiding this comment

chrisjrn commented Sep 8, 2021

Choose a reason for hiding this comment

Eric-Arellano commented Sep 9, 2021

chrisjrn commented Sep 9, 2021

Eric-Arellano commented Sep 9, 2021

chrisjrn commented Sep 9, 2021

chrisjrn commented Sep 10, 2021

Choose a reason for hiding this comment

chrisjrn commented Sep 10, 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

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

Choose a reason for hiding this comment

Eric-Arellano commented Sep 14, 2021

Add new version of `LockfileMetadata` to support checking for identically specified requirements #12782

Add new version of `LockfileMetadata` to support checking for identically specified requirements #12782

chrisjrn commented Sep 8, 2021 •

edited

Loading

chrisjrn Sep 13, 2021 •

edited

Loading