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

investigate "remaining universe" fork and marker construction #5482

Closed
BurntSushi opened this issue Jul 26, 2024 · 2 comments
Closed

investigate "remaining universe" fork and marker construction #5482

BurntSushi opened this issue Jul 26, 2024 · 2 comments
Labels
resolver Related to the package resolver

Comments

@BurntSushi
Copy link
Member

In PR #5163, @konstin brought up a concern that the way I was creating the "remaining universe" fork in the case of incomplete markers was either not correct or, perhaps, was doing more than what was needed. I spent some time trying to come up with a packse scenario that demonstrated why it was needed. My first attempt was this:

name = "fork-remaining-universe-partitioning"

[resolver_options]
universal = true

[expected]
satisfiable = true

[root]
requires = [
  "a>=2 ; sys_platform == 'windows'",
  "a<2 ; sys_platform == 'illumos'",
]

[packages.z.versions."1.0.0"]
[packages.a.versions."1.0.0"]
requires = [
  "b>=2 ; os_name == 'linux'",
  "b<2 ; os_name == 'darwin'",
  "z ; sys_platform == 'windows'",
]
[packages.a.versions."2.0.0"]
[packages.b.versions."1.0.0"]
[packages.b.versions."2.0.0"]

But as @konstin points out, it doesn't actually exercise the code path in question. I was wrong to think about this in terms of "parent" forks, since those are handled at a different point in the code. So I tried to provoke a scenario where the "remaining universe" fork needed to be constructed by starting from the existing forks, rather than creating its own. I think this requires a double sibling split, but I'm not sure. This is where I stopped:

name = "fork-remaining-universe-partitioning2"

[resolver_options]
universal = true

[expected]
satisfiable = true

[root]
requires = [
  "a>=2 ; sys_platform == 'windows'",
  "a<2 ; sys_platform == 'illumos'",
  "b>=2 ; os_name == 'linux'",
  "b<2 ; os_name == 'darwin'",
  "y ; sys_platform == 'foo'",
  "z ; os_name == 'bar'",
]

[packages.a.versions."1.0.0"]
[packages.a.versions."2.0.0"]
[packages.b.versions."1.0.0"]
[packages.b.versions."2.0.0"]
[packages.y.versions."1.0.0"]
[packages.z.versions."1.0.0"]

For now, we decided to move forward with #5163 in its current state since it fixes known bugs and doesn't introduce any known regressions. But this is something we should circle back to.

@BurntSushi BurntSushi added the resolver Related to the package resolver label Jul 26, 2024
@BurntSushi
Copy link
Member Author

Also, I think that my next step here would be to try @konstin's suggestion in the linked PR and see if it can be knocked down.

@BurntSushi
Copy link
Member Author

I think this is no longer relevant with #5887 merged. Namely, while we are still dealing with incomplete markers, we are doing it more systematically in a way that deals with overlapping markers as well. So this should be fixed (if it was an issue before).

@BurntSushi BurntSushi closed this as not planned Won't fix, can't repro, duplicate, stale Aug 16, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
resolver Related to the package resolver
Projects
Status: Done
Development

No branches or pull requests

1 participant