Skip to content
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

Refactor export #5156

Merged
merged 3 commits into from
Apr 3, 2022
Merged

Refactor export #5156

merged 3 commits into from
Apr 3, 2022

Conversation

dimbleby
Copy link
Contributor

@dimbleby dimbleby commented Feb 6, 2022

Pull Request Check List

  • Added tests for changed code.
  • Updated documentation for changed code.

Refactoring poetry export to deal with bugs encountered in the testcase written for #5141, in particular "bug 1" of #5154.

This has turned out to be a rather larger set of changes than I had intended or than I am completely happy about submitting in a single MR. But the fact is that I kept encountering bugs, and there is no real improvement until they all are fixed.

Outline of the changes made:

  • when selecting a locked package in the walk, make sure that not only does its version satisfy the constraint on the requirement, but also that its markers are compatible with the markers on the requirement
  • when selecting a locked package in the walk, avoid selecting two different versions of the package with overlapping markers
  • remove oddities from __walk_dependency_level()
    • (specifically: an undesirable special case for the top-level dependencies, and this which I can't make sense of except as some sort of workaround for the other bugs)
    • in fact I have reworked this as a very plain iterative breadth-first search
  • removed some unused parameters pinned_versions and with_nested

We also need a fix in poetry-core to include markers in the hash of a Dependency, I'll do that separately. I expect that'll prevent the pipeline from passing on this MR. Edit: I rearranged this MR so that it doesn't rely on further poetry-core changes, but it seems that the tests do still require a recent version of that project.

The proof of all this pudding is in the testcase at the end of it all test_exporter_doesnt_confuse_repeated_packages, where we finally manage to export a set of requirements that is not impossible, doesn't have overlaps, and generally looks good.

Again, apologies for the size of the MR. I actually think that overall this is a simplification, but there was rather a lot of code to simplify. Do ask if you have any questions.

@dimbleby
Copy link
Contributor Author

dimbleby commented Feb 7, 2022

Ooh, poetry already has a root package (just not derived from the lock file). I should be able to undo most of the changes here, just keeping the core algorithm improvements.

@finswimmer
Copy link
Member

Hey @dimbleby,

please note that the export command will be replaced by the export-plugin. See #4824. The export-plugin is located at https://github.com/python-poetry/poetry-export-plugin.

fin swimmer

@dimbleby
Copy link
Contributor Author

dimbleby commented Feb 7, 2022

I've pushed a version that uses the existing root package, which removes large amounts of uninteresting changes in the unit tests.

The remaining impact on existing unit tests is that poetry export now includes the project-level python version in the markers - which I am happy is sensible, and correct.

Am not overly concerned about the plugin, most of the changes here are in the locker anyway.

Still seems to need recent poetry-core, would it be good to tag and release a new version of that project soon?

@dimbleby dimbleby force-pushed the refactor-export branch 2 times, most recently from 0a4da5a to d76d1ce Compare February 8, 2022 16:06
@dimbleby
Copy link
Contributor Author

dimbleby commented Mar 5, 2022

rebased for:

  • merge conflicts with typing changes
  • to pick up poetry-core 1.1.0a7, which allows the new testcase to pass

@ESKYoung ESKYoung mentioned this pull request Mar 6, 2022
2 tasks
@dimbleby dimbleby force-pushed the refactor-export branch 5 times, most recently from 3d718f8 to 2d61c34 Compare March 8, 2022 23:48
src/poetry/packages/locker.py Show resolved Hide resolved
src/poetry/utils/exporter.py Outdated Show resolved Hide resolved
@dimbleby
Copy link
Contributor Author

I added another testcase showcasing a fix made in this MR relating to extras. On master, test_exporter_handles_extras_next_to_non_extras results in:

another-thing==1.0.0
localstack-ext==1.0.0
localstack==1.0.0
something-else==1.0.0
something==1.0.0

but another-thing is wrong, that should only be installed if localstack-ext[baz] is installed, which it isn't.

src/poetry/utils/exporter.py Outdated Show resolved Hide resolved
@lovesegfault
Copy link
Contributor

I've been hitting an annoying bug that is fixed by this, thanks for the fix @dimbleby :)

@dimbleby
Copy link
Contributor Author

dimbleby commented Apr 3, 2022

ugh, merge conflicts.

@abn I don't think this is waiting on anything, can we merge it?

@abn
Copy link
Member

abn commented Apr 3, 2022

Ugh. My bad. I needed to do a once over and we would be good. I'll get to it today.

Copy link
Member

@abn abn left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Approving this for now. The logic is improved, current behaviour is largely maintained outside of project level python version marker propagation. This drops unused features but they can be reintroduced later as required.

@abn abn merged commit fb13b3a into python-poetry:master Apr 3, 2022
@dimbleby dimbleby deleted the refactor-export branch April 3, 2022 18:07
@Agalin
Copy link

Agalin commented Apr 21, 2022

Any chance for another beta release? It seems to finally fix our dependency issues mentioned in #3511 while beta1 is not compatible with the export plugin.

Copy link

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.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Feb 29, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants