test: Add _test_simple_multiple_platforms_with_extras

[hartikainen]

As requested in #2797 (comment) and aignas#2 (comment). Related to #2797. Closes #3374.

For the sake of full transparency, I used gemini-cli to generate most of the most recent solution.

[aignas]

Hah, actually this is a separate issue and I am not sure how we can fix it. So, a little bit of history here:

1. poetry will export a requirements.txt that will have both package[extra] and package and we were asked to only consider package[extra] when doing the installation.
2. However with the advent of universal files it means that we should do something differently - first resolve the compatible lines with the target platforms and do the filtering differently.

I remember we had this discussion with in one of @ewianda PRs, but I can't remember which one it is.

I think the issue that you are facing does not have an open issue yet.

[aignas]

What is more, we have a test that already tests for this behaviour in the case where the extras differ, but we don't have a test where extra is present in one and in another it is not.

PRs for this are welcome and we should have enough test coverage in places that the fix would be touching.

[hartikainen]

Thanks for the quick response and context! I'll look into the existing case where extras differ to see if that can be extended to this case. Let's see where I get.

[hartikainen]

I think I now have a solution for this: creating distinct repository name for each requirement line with extras. I'm not sure if this is the best solution though, because it leads to defining multiple, identical whl_library repositories even when different extras point to the same wheel file. This seems inefficient and unnecessary.

After looking around a bit, I realized that maybe following comment by @aignas would be a better solution?

rules_python/python/private/pypi/parse_requirements.bzl

Lines 109 to 114 in fe45faa

	# Get the longest match and fallback to original WORKSPACE sorting,
	# which should get us the entry with most extras.
	#
	# FIXME @aignas 2024-05-13: The correct behaviour might be to get an
	# entry with all aggregated extras, but it is unclear if we
	# should do this now.

[hartikainen]

I spend quite some time trying to wrap my head around the parse_requirements code and how to aggregate the extras. I could not figure out a way to make that work without breaking some other tests. I don't think I currently understand the requirement parsing stuff well enough to properly fix the underlying issue.

[rickeylev]

Creating another repo for foo[x] wouldn't be the thing to do, anyways. The extra doesn't affect the contents of the package (whl file downloaded). A repo is created for each unique (package, version) (i.e. one per unique artifact downloaded).

What the extra does is affect the Requires-Dist resolution later, when figuring out the list of dep targets. e.g. given foo[a] == 1.0, then the pypi_foo repo is created from the foo-whatever.whl file. A py_library(deps=...) is created based on the Requires-Dist entries in the wheel. Those entries can have conditions, like "only if extra X is requested".

Ignas would know this code better, but IIRC, the list of extras is kept and passed around somewhere so that evaluating the Requires-Dist lines has the right context.

[aignas]

This is a bit of a mess because historically the extra would result in a different set of deps wired in your closure. So in order to fix this there are two approaches:

1. Figure out how to aggregate the requirements without breaking existing tests, if there is no way, change the tests.
2. Enable pipstar by default (feat: enable pipstar by default #2949) - this would use starlark to parse the dependencies and we would not need to do this clever aggregation by extras anymore.

Just to outline the option number 2 (which is a better long term solution), the rough sketch is:

1. enable pipstar by default (I tried it briefly but it was not ready yet). It should be safe to do this in bzlmod and in theory it could even work in WORKSPACE with extra work on figuring out how the env_marker_setting can work.
2. to whl_library pass a list of extras for which we should create targets or create py_library targets for each extra found in the METADATA. The naming of the targets could be pkg for no extras, pkg__foo for extras==foo. If there is a dependency on pkg[bar] then we would need to translate that to a dependency on @<hub_name>//pkg:pkg__bar. If there is a dependency on pkg[bar,baz] then this translates to two dependencies - @<hub_name>//pkg:pkg__bar and @<hub_name>//pkg:pkg__baz. The naming convention of the targets is up for discussion.
3. to the hub_repository pass a list of extras for which we should generate extra aliases.
4. The parse_requirements code would only need to care about the extras and what need to be present where.

However, there could be still a need to fix the tests to make the behaviour more correct before we go the long way and implement option 2.

[aignas]

I am testing this in main and it seems that a universal requirements file might work better in this particular case - I cannot create a failing test case for parse_requirements function yet.

[aignas]

Actually, parse_requirements is not handling this correctly.

It returns output that will not work with the current implementation of the pip.parse, so it is to blame, but I need to look a little more at it where to do the fix.

[hartikainen]

I just rebased this branch off of main and updated the approach based on the solution two from @aignas's comment above (#3193 (comment)). For the sake of full transparency, I used gemini-cli to generate most of the solution, and reviewed the code myself. The solution is relatively simple and the tests now pass, at least locally on bzlmod. Any feedback is much welcome!