-
-
Notifications
You must be signed in to change notification settings - Fork 621
test: Add _test_simple_multiple_platforms_with_extras
#3193
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
test: Add _test_simple_multiple_platforms_with_extras
#3193
Conversation
# NOTE(hartikainen): Perhaps this is part of the problem? | ||
# This should say `package[extra]==0.7.0` for `linux_x86_64` platform and | ||
# `package==0.7.0` for `linux_arm64` | ||
"requirement": "package[extra]==0.7.0", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hah, actually this is a separate issue and I am not sure how we can fix it. So, a little bit of history here:
- poetry will export a
requirements.txt
that will have bothpackage[extra]
andpackage
and we were asked to only considerpackage[extra]
when doing the installation. - 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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
78aab20
to
856e5d3
Compare
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 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
|
Run with: ```console bazel test //tests/pypi/extension:test_simple_multiple_extras_same_whl ```
The `pypi_repo_name` function was using `*target_platforms` to accept a variable number of positional arguments. This is changed to a keyword argument `target_platforms` with a default value of empty string, making the function signature more explicit and the call site clearer.
856e5d3
to
061e8fe
Compare
I spend quite some time trying to wrap my head around the |
Creating another repo for What the extra does is affect the 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. |
This is a bit of a mess because historically the
Just to outline the option number 2 (which is a better long term solution), the rough sketch is:
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. |
As requested in #2797 (comment) and aignas#2 (comment). Related to #2797.
Failure output: