Fixing platform_release pep440 #722

npapapietro · 2024-04-17T23:47:13Z

Resolves: python-poetry python-poetry/poetry#7418

Added tests for changed code.
Updated documentation for changed code.

Can add more tests if requested. Any feedback welcome as well, attempted to fix with smallest changes and no side effects.

Examples of conditions this should fix

'tegra' in platform_release
'platform_release <= 5.10.123-tegra

When parsing versions, I added an additional semver-ish regex that will catch patterns that match certain platform_release versions that aren't quite pep440 or semver. Examples are either WSL2 builds, raspi builds or nvidia edge device builds

5.15.146.1-microsoft-standard-WSL2
6.6.20+rpt-rpi-v8
5.10.120-tegra

radoering · 2024-04-22T16:02:39Z

Interesting approach. I'll try to take a closer look later this week.

For future reference: #552 is a previous attempt to solve the issue by just making platform_release "non-version-like". The comments in this PR might be interesting to understand why it is not that simple.

npapapietro · 2024-04-22T16:13:54Z

Because this addresses some issues for folks using edge devices (my team using -tegra nvidia devices), it was important to keep it version-like still. The big takeaways here are:

Adding a string comparison flag str_cmp to detect when 'substring' in platform_release for simple testing
Since OS versions aren't semver like, we choose to cheese it a bit and extract the maj,min,patch components and then relegate the rest of build tag to the Version classes post arg.

radoering

I think with some improvements and some more unit tests for the changed methods the PR could be accepted.

One general advice: It makes the review easier if you re-arrange your commits so that there is a corresponding unit test for each change in the same commit. For example:

In the first commit, you changed the validate() method, but you did not extend a validate test.
In the second commit, you changed the version constraint parser, but you did not extend test_parse_constraint.

If you change a method, please take a look if there is a unit test for this method and extend the test.

src/poetry/core/version/markers.py

src/poetry/core/constraints/version/parser.py

src/poetry/core/constraints/version/patterns.py

npapapietro · 2024-04-27T15:54:03Z

Thank you for your review, I will work to fixup my commit flow to match change+unit test and follow up on your comments.

npapapietro · 2024-04-29T02:55:16Z

I have forced pushed over my previous commits with 3 commits hopefully tying change+test into each commit. There might be some overlap between tests. Most of the diff is reorganizing commits and included your suggestions.

If you change a method, please take a look if there is a unit test for this method and extend the test.

I added several more tests to the PR by looking at the methods I modified. Please let me know if there are ones I overlooked or additional test cases you would like to see.

radoering

Thanks for reorganizing the commits. This really makes the review easier. I added some further comments.

The hardest part is to consider all combinations of operators in the methods of Constraint. I mentioned some examples in the last comments, which can be added in test_constraints if we can handle them.

src/poetry/core/version/markers.py

radoering · 2024-05-01T12:38:29Z

tests/constraints/generic/test_main.py

@@ -18,6 +18,8 @@
        ("==win32", Constraint("win32", "=")),
        ("!=win32", Constraint("win32", "!=")),
        ("!= win32", Constraint("win32", "!=")),
+        ("not integra", Constraint("tegra", "not in")),
+        ("integra", Constraint("tegra", "in")),


It's a bit confusing that there is no space after in. And actually, it should be tegra not in. I'm still wondering if it's worth to change the string representation of generic in/not in constraints because that will influence other parts of the code. We had to adapt SingleMarker._CONSTRAINT_RE besides the regexes in the generic constraints parser. Maybe, it's still worth it to avoid future confusion.

Yeah, this is how the {op}{value} is being generated. I was unsure about changing that method and introducing regression bugs with everything else. I'll play around with it to see if I can tweak it to increase readability without introducing many side effects

Investigated a bit further, I can without much effort space out the operator from the value not integra -> not in tegra.

Changing the order of the values would require some reworking.
This one https://github.com/npapapietro/poetry-core/blob/ddfc68da2382e4581b6c4e6b278a7be56d584d44/src/poetry/core/constraints/generic/parser.py#L31 would require increasing the complexity of the regex itself or if statements to ensure mixing not in/in operators with others are still satisfied.

Actually, I have no idea why this regex is so complicated. I think we can simplify it so it will not be an issue. See my other comment. I think I am still in favor of bringing it in the correct order despite some additional effort. Adapting STR_CMP_CONSTRAINT and SingleMarker._CONSTRAINT_RE should not be too complicated. In STR_CMP_CONSTRAINT we just have to swap op and value and in SingleMarker._CONSTRAINT_RE we can just add an "or" with swapped order for the new operators.

radoering · 2024-05-01T12:55:57Z

src/poetry/core/constraints/generic/constraint.py

+        if self._operator in {"in", "not in"} and other.operator == "==":
+            return bool(self._trans_op_str[self._operator](other.value, self._value))


I think that is incomplete. For example, platform_release != "5.10.123-tegra" allows "tegra" not in platform_release, it even allows_all. Talking about allows_all and allows_any: I suppose these need some extensions, too.

radoering · 2024-05-01T13:04:57Z

src/poetry/core/constraints/generic/constraint.py

-            if other.operator == "!=" and self.operator == "!=":
+            if (other.operator == "!=" and self.operator == "!=") or (
+                other.operator == "not in" and self.operator == "not in"
+            ):


I think some combinations are missing here. For example, the intersection of platform_release != "5.10.123-tegra" and "tegra" not in platform_release is "tegra" not in platform_release.

radoering · 2024-05-01T13:06:56Z

src/poetry/core/constraints/generic/constraint.py

-            if other.operator == "==" and self.operator == "==":
+            if (other.operator == "==" and self.operator == "==") or (
+                other.operator == "in" and self.operator == "in"
+            ):


I think some combinations are missing here. For example, the union of platform_release != "5.10.123-tegra" and "tegra" not in platform_release is platform_release != "5.10.123-tegra".

npapapietro · 2024-05-01T16:49:26Z

Will address comments and fix issues in coming days

npapapietro · 2024-05-10T00:54:12Z

I've updated the allows method and tests related to intersection and union. I definitely need a sanity check. I included a logic table to double check myself.

radoering

I think the logic for allows is not yet completely correct. However, while reviewing it, I noticed that this method had already been quite a mess with a vague interface and some bugs before. I think we have to fix it first for the existing operators (and increase test coverage) before introducing new operators. Therefore, I created #732. I hope it does not take too long until this one gets reviewed so we can proceed here.

In the meantime, please take a look at my other comments concerning the regexes.

radoering · 2024-05-18T08:46:15Z

src/poetry/core/version/markers.py

+        if self._swapped_name_value:
+            constraint = f'"{self._value}" {operator} {self._name}'
+        else:
+            constraint = f"{self._name} {operator} '{self._value}'"


Suggested change

constraint = f"{self._name} {operator} '{self._value}'"

constraint = f'{self._name} {operator} "{self._value}"'

Nitpick: I'd use the same "quote schema" as above.

radoering · 2024-05-18T09:37:07Z

src/poetry/core/constraints/generic/parser.py

@@ -27,7 +28,7 @@ def parse_constraint(constraints: str) -> BaseConstraint:
    or_groups = []
    for constraints in or_constraints:
        and_constraints = re.split(
-            r"(?<!^)(?<![=>< ,]) *(?<!-)[, ](?!-) *(?!,|$)", constraints
+            r"(?<!^)(?<![=>< ,(in|not in)]) *(?<!-)[, ](?!-) *(?!,|$)", constraints


Suggested change

r"(?<!^)(?<![=>< ,(in|not in)]) *(?<!-)[, ](?!-) *(?!,|$)", constraints

r"\s*,\s*", constraints

Actually, this regex does not what it pretends to do. The (in|not in) are just (partially duplicated) characters inside of []. To support in and not in [=>< ,nt] instead of [=>< ,(in|not in)] would be sufficient (still allowing more than intended).

However, I wonder why this regex is so complicated in the first place although we only supported == and != so far? I did some code spelunking and it seems like the regex was like that from the start. Evaluating all usages of the parser, which are mostly with internally created strings, I think we can simplify the regex. At least, I am willing to take the risk because it seems like no tests will fail with the simplication.

radoering · 2024-05-18T13:00:32Z

tests/constraints/generic/test_main.py

@@ -18,6 +18,8 @@
        ("==win32", Constraint("win32", "=")),
        ("!=win32", Constraint("win32", "!=")),
        ("!= win32", Constraint("win32", "!=")),
+        ("not integra", Constraint("tegra", "not in")),
+        ("integra", Constraint("tegra", "in")),


Actually, I have no idea why this regex is so complicated. I think we can simplify it so it will not be an issue. See my other comment. I think I am still in favor of bringing it in the correct order despite some additional effort. Adapting STR_CMP_CONSTRAINT and SingleMarker._CONSTRAINT_RE should not be too complicated. In STR_CMP_CONSTRAINT we just have to swap op and value and in SingleMarker._CONSTRAINT_RE we can just add an "or" with swapped order for the new operators.

npapapietro marked this pull request as ready for review April 18, 2024 15:30

npapapietro force-pushed the pep440-platform-release branch 2 times, most recently from e7f3881 to 3e16582 Compare April 18, 2024 21:42

Secrus requested a review from radoering April 22, 2024 10:42

radoering requested changes Apr 27, 2024

View reviewed changes

src/poetry/core/version/markers.py Outdated Show resolved Hide resolved

src/poetry/core/constraints/version/parser.py Outdated Show resolved Hide resolved

src/poetry/core/constraints/version/patterns.py Outdated Show resolved Hide resolved

npapapietro force-pushed the pep440-platform-release branch from 3e16582 to ddfc68d Compare April 29, 2024 02:46

radoering requested changes May 1, 2024

View reviewed changes

npapapietro force-pushed the pep440-platform-release branch from ddfc68d to 94be7d4 Compare May 10, 2024 00:51

npapapietro added 4 commits May 9, 2024 19:52

in/notin added to constraint

6b8cdd8

String comparison parsing

5eeadad

Implement release pattern matching

cf6847b

Allows, intersect, union methods updates

157cbe1

npapapietro force-pushed the pep440-platform-release branch from 94be7d4 to 157cbe1 Compare May 10, 2024 00:52

radoering mentioned this pull request May 18, 2024

constraints: fix allows, allows_any and allows_all for generic constraints and increase test coverage #732

Open

2 tasks

radoering requested changes May 18, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fixing platform_release pep440 #722

Fixing platform_release pep440 #722

npapapietro commented Apr 17, 2024 •

edited

radoering commented Apr 22, 2024

npapapietro commented Apr 22, 2024

radoering left a comment

npapapietro commented Apr 27, 2024

npapapietro commented Apr 29, 2024 •

edited

radoering left a comment

radoering May 1, 2024

npapapietro May 1, 2024

npapapietro May 3, 2024

radoering May 18, 2024

radoering May 1, 2024

radoering May 1, 2024

radoering May 1, 2024

npapapietro commented May 1, 2024

npapapietro commented May 10, 2024

radoering left a comment

radoering May 18, 2024

radoering May 18, 2024

radoering May 18, 2024

		if self._operator in {"in", "not in"} and other.operator == "==":
		return bool(self._trans_op_str[self._operator](other.value, self._value))

	constraint = f"{self._name} {operator} '{self._value}'"
	constraint = f'{self._name} {operator} "{self._value}"'

	r"(?<!^)(?<![=>< ,(in\|not in)]) (?<!-)[, ](?!-) (?!,\|$)", constraints
	r"\s,\s", constraints

Fixing platform_release pep440 #722

Are you sure you want to change the base?

Fixing platform_release pep440 #722

Conversation

npapapietro commented Apr 17, 2024 • edited

radoering commented Apr 22, 2024

npapapietro commented Apr 22, 2024

radoering left a comment

Choose a reason for hiding this comment

npapapietro commented Apr 27, 2024

npapapietro commented Apr 29, 2024 • edited

radoering 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

npapapietro commented May 1, 2024

npapapietro commented May 10, 2024

radoering 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

npapapietro commented Apr 17, 2024 •

edited

npapapietro commented Apr 29, 2024 •

edited