-
-
Notifications
You must be signed in to change notification settings - Fork 611
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
Track dependencies for local packages that get combined #1507
Conversation
Thank you! Do you think it would be appropriate to combine this fix with this one (or an alternate form)? |
I think the fix in either of those is a good one. Fixing that lost info will probably surface more cases of the bug fixed here, since this is triggered by url-requirements, so I'd say merge it after this PR or combine the two. It's choosing one stakeholder over another, but likely that lost info hasn't been a deal breaker so far? As I mentioned elsewhere, I'll take a look at the case of a local package dependency + combining extras, to see if it's real/failing/new. Not sure if you have initial thoughts on it, but that could slow us down, depending on the preference of failing case. #1385 looks maybe worth another review, since there are a bunch of complex additions to the tests that someone here might have a good idea about simplifying, maybe with some of the more recent test utilities ( |
I've added an xfail test case for this, and it fails on master as well, so I interpret that as unblocking this PR (at least for this reason). |
b49c0fc
to
66db8b8
Compare
@AndydeCleyre I cherry-picked your version of the fix. I also tried to take the test from the original PR, but between updating the test to fail without the fix (very important!) and adding If you all think this PR is too bloated now, I can split the last two commits back out. Otherwise, we should update the PR description to include this second fix. |
26cd552
to
e44886a
Compare
…ependency" (commit d720fe6) This brings back copy_ireq_dependencies and combines its use with that of the proxies-for-naming. Editable and url_requirement ireqs are not cached by the Repository, so when dependencies are queried for them once, no dependencies will be found later for the copy made by combine_install_requirements. copy_ireq_dependencies links those dependencies at the time of copy.
round of resolution. By this point, we've frozen the dependencies.
Fixes jazzband#1054 Co-authored-by: Andrew <harta5039@gmail.com>
e44886a
to
036e7c3
Compare
I'm going to close this in favor of focusing related attention on your own #1519. Please let me know if I'm making a mistake. Thanks! |
Works for me! (I had left it open in case we wanted it as an incremental improvement, while progressing on the bigger change.) |
Fixes #1505
Partially revert "Fix a regression where multiple declarations of a dependency"
(commit d720fe6)
This brings back copy_ireq_dependencies and combines its use with that of the
proxies-for-naming. Editable and url_requirement ireqs are not cached by the
Repository, so when dependencies are queried for them once, no dependencies
will be found later for the copy made by combine_install_requirements.
copy_ireq_dependencies links those dependencies at the time of copy.
Contributor checklist
Maintainer checklist
backwards incompatible
,feature
,enhancement
,deprecation
,bug
,dependency
,docs
orskip-changelog
as they determine changelog listing.