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

Track dependencies for local packages that get combined #1507

Closed
wants to merge 5 commits into from

Conversation

richafrank
Copy link
Contributor

@richafrank richafrank commented Oct 13, 2021

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
  • Provided the tests for the changes.
  • Assure PR title is short, clear, and good to be included in the user-oriented changelog
Maintainer checklist
  • Assure one of these labels is present: backwards incompatible, feature, enhancement, deprecation, bug, dependency, docs or skip-changelog as they determine changelog listing.
  • Assign the PR to an existing or new milestone for the target version (following Semantic Versioning).

@richafrank richafrank changed the title Track second level dependencies for local packages Track dependencies for local packages that get combined Oct 13, 2021
@AndydeCleyre
Copy link
Contributor

Thank you!

Do you think it would be appropriate to combine this fix with this one (or an alternate form)?

@richafrank
Copy link
Contributor Author

richafrank commented Oct 13, 2021

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 (make_package et al). (The few calls there to os.path.join with a single argument also jumped out at me as unnecessary.)

@atugushev atugushev added bug fix resolver Related to dependency resolver labels Oct 16, 2021
@atugushev atugushev added this to the 6.4.1 milestone Oct 16, 2021
@richafrank
Copy link
Contributor Author

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.

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).

@richafrank richafrank force-pushed the a-little-from-column-a branch 2 times, most recently from b49c0fc to 66db8b8 Compare October 17, 2021 22:58
@richafrank
Copy link
Contributor Author

richafrank commented Oct 22, 2021

Do you think it would be appropriate to combine this fix with this one (or an alternate form)?

@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 make_package, I ended up re-writing it.

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.

richafrank and others added 5 commits February 8, 2022 13:23
…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.
@AndydeCleyre
Copy link
Contributor

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!

@richafrank
Copy link
Contributor Author

Works for me! (I had left it open in case we wanted it as an incremental improvement, while progressing on the bigger change.)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
resolver Related to dependency resolver
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Second level dependencies missing on for local file packages on >= 6.3.1
3 participants