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

Fix transitive subpackage dependency resolution #5603

Merged
merged 5 commits into from
Jan 28, 2025

Conversation

isuruf
Copy link
Contributor

@isuruf isuruf commented Jan 27, 2025

Description

Checklist - did you ...

  • Add a file to the news directory (using the template) for the next release's release notes?
  • Add / update necessary tests?
  • Add / update outdated documentation?

Fixes #3308

@isuruf isuruf requested a review from a team as a code owner January 27, 2025 18:42
@conda-bot conda-bot added the cla-signed [bot] added once the contributor has signed the CLA label Jan 27, 2025
Copy link

codspeed-hq bot commented Jan 27, 2025

CodSpeed Performance Report

Merging #5603 will not alter performance

Comparing isuruf:transitive-subpackage (aa8a2e9) with main (cf35214)

Summary

✅ 5 untouched benchmarks

Copy link
Contributor

@beckermr beckermr left a comment

Choose a reason for hiding this comment

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

I need to read in more detail, but should we add a specific test for which transitive deps get picked up into the envs?

Right now the tests only ensure that the recipe from the issue builds.

Copy link
Contributor

@beckermr beckermr left a comment

Choose a reason for hiding this comment

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

The logic looks correct, but I am fuzzy on the various m.is_cross branches. We should a test that specifically looks at the resolved envs for the recipe to ensure it has the right packages.

@isuruf
Copy link
Contributor Author

isuruf commented Jan 27, 2025

We should a test that specifically looks at the resolved envs for the recipe to ensure it has the right packages.

I'm not sure what you mean here. The test that I added fails to build with the current main branch.

@beckermr
Copy link
Contributor

What I mean is that in the original issue, the output of conda render was shown. This demonstrated that the various envs made during the build did not have the right packages in them.

I am saying that in addition to ensuring the recipe from the issue builds, we should explicitly verify that the envs made during the build have the correct transitive packages in them.

This will help future maintainers debug any test failures.

@isuruf
Copy link
Contributor Author

isuruf commented Jan 28, 2025

pre-commit.ci autofix

@beckermr beckermr enabled auto-merge (squash) January 28, 2025 11:58
@wolfv
Copy link
Contributor

wolfv commented Jan 28, 2025

I am not sure this is relevant, but in rattler-build we actually create variants for each exact=true so that the tree is unfolded correctly :)

@isuruf
Copy link
Contributor Author

isuruf commented Jan 28, 2025

I am not sure this is relevant, but in rattler-build we actually create variants for each exact=true so that the tree is unfolded correctly :)

That's a different issue. Or rather the second comment in the same issue. See #3308 (comment)

@beckermr beckermr merged commit 49872a5 into conda:main Jan 28, 2025
28 checks passed
@isuruf isuruf deleted the transitive-subpackage branch January 29, 2025 04:49
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cla-signed [bot] added once the contributor has signed the CLA
Projects
Status: 🏁 Done
Development

Successfully merging this pull request may close these issues.

Transitive constraints are not properly considered in multiple output recipes
4 participants