poetry-core icon indicating copy to clipboard operation
poetry-core copied to clipboard

Fixing platform_release pep440

Open npapapietro opened this issue 1 year ago • 9 comments

Resolves: python-poetry/poetry#7418

  • [x] 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

npapapietro avatar Apr 17 '24 23:04 npapapietro

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.

radoering avatar Apr 22 '24 16:04 radoering

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.

npapapietro avatar Apr 22 '24 16:04 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 avatar Apr 27 '24 15:04 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.

npapapietro avatar Apr 29 '24 02:04 npapapietro

Will address comments and fix issues in coming days

npapapietro avatar May 01 '24 16:05 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.

npapapietro avatar May 10 '24 00:05 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

npapapietro avatar May 30 '24 02:05 npapapietro

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

npapapietro avatar Jun 02 '24 18:06 npapapietro

I was traveling for work, but now I am getting back to this. I will be asking some clarification questions as I work through you latest comments.

npapapietro avatar Jun 23 '24 17:06 npapapietro

I want to get some more test cases in to ensure I have covered edge cases. This topic is indeed finicky

npapapietro avatar Jul 01 '24 03:07 npapapietro

I added a few more tests, fixed and simplified a few things. I think I will take another look next week and - if everything still makes sense to me - finally merge it.

Thanks for your endurance.

radoering avatar Jul 20 '24 11:07 radoering