-
Notifications
You must be signed in to change notification settings - Fork 2.3k
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
exporting packages with features #5688
Conversation
I don't know what order these things can all be committed in to keep the tests passing at all times! |
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 @dimbleby for picking this up. Some comments in relation to fixing the duplicated attributes in core's Dependency
implementation.
One possible order for these changes could be as follows.
In the long term we need to fix this coupling somehow :( |
Long-term, I suppose that this repository should simply not know anything about poetry-plugin-export; because that plugin should be no more special than any other. (Also that would fix the circular dependency where both projects depend on one another). Perhaps we can just do that now, must we test the export plugin from here? |
It is possible yes. However, might require a deprecation cycle for the command itself. The issue is |
reworked fix is now simpler, and exports like this in that existing testcase: localstack-ext==1.0.0 ; {MARKER_PY36}
-localstack==1.0.0 ; {MARKER_PY36}
+localstack-ext[bar]==1.0.0 ; {MARKER_PY36}
+localstack[foo]==1.0.0 ; {MARKER_PY36} ie we get both localstack-ext and localstack-ext[bar] This is cosmetically ugly when, as here, both variations have the same markers. But in cases where they have different markers eg some extra is required on windows only - this seems like it is giving the right answers where the previous try didn't. Indeed something like this is the case in #5537, where what we now export is:
it would have been a mistake to combine this into
|
This pull request has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs. |
Fix for #5537, see also python-poetry/poetry-plugin-export#65
Have
get_project_dependency_packages()
return packages with features suitably set, so that the exporter can record them. Features are accumulated as we walk the tree.With that set of fixes, this test sees this diff
which is python-poetry/poetry-plugin-export#66
Direct manipulation of
_extras
is not great, we should addset_extras(extras)
or similar inpoetry-core
. That's a little exciting -_extras
gets hashed so careless callers can go wrong - but it's not worse thanset_constraint()
as also used by the locker (and, similarly, is safe to do in this code).Also we can remove
requires_extras
from poetry-core, it's unused.