Fixing platform_release pep440

[npapapietro]

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]

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]

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.

[npapapietro]

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]

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.

[radoering]

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]

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]

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]

Will address comments and fix issues in coming days

[npapapietro]

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.

[npapapietro]

I managed to make some changes to the regex and how we represent the marker internally. Now all markers should be correctly represented.

"tegra" in platform_release is now "tegra" in for the single marker instead of integra

[radoering]

Looks good so far.

I think we can drop one regex. (See code comments.)

"tegra" in platform_release is now "tegra" in for the single marker instead of integra

I wonder if we should use quotes or not because the representation of the non-swapped constraints is without quotes (e.g. !=win32 instead of !="win32"). However, I understand that there is more ambiguity with in and not in so I think it is fine the way it is.

By the way, #732 has been merged into main so you can rebase (sorry for the conflicts) and we can take a closer look at the allows methods.

[npapapietro]

I have hopefully deconflicted correctly the PR that was merged earlier and addressed your comments on the regex.