-
Notifications
You must be signed in to change notification settings - Fork 11
Fix resolvo treating options as var requirements #1258
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
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change | ||||||
|---|---|---|---|---|---|---|---|---|
|
|
@@ -3086,3 +3086,69 @@ async fn request_for_all_component_picks_correct_version( | |||||||
| let solution = run_and_print_resolve_for_tests(&mut solver).await.unwrap(); | ||||||||
| assert_resolved!(solution, "mypkg", version = version); | ||||||||
| } | ||||||||
|
|
||||||||
| /// Verify that when solving the dependencies of a package, the build options of | ||||||||
| /// the candidates do not factor into the selection of the candidate. | ||||||||
| #[rstest] | ||||||||
| #[case::step(step_solver())] | ||||||||
| #[case::resolvo(resolvo_solver())] | ||||||||
| #[tokio::test] | ||||||||
| async fn build_options_not_checked_on_dependencies(#[case] mut solver: SolverImpl) { | ||||||||
| // Suppose a platform exists | ||||||||
| let spi_platform_2024_1_1_1 = | ||||||||
| make_build!({"pkg": "spi-platform/2024.1.1.1", "compat": "x.x.a.b"}); | ||||||||
| // And a library package targets that platform version | ||||||||
| let openimageio_1_2_3 = make_build!( | ||||||||
| { | ||||||||
| "pkg": "openimageio/1.2.3", | ||||||||
| "build": { | ||||||||
| "options": [ | ||||||||
| { "pkg": "spi-platform/~2024.1.1.1" }, | ||||||||
| ], | ||||||||
| }, | ||||||||
| }, | ||||||||
| [spi_platform_2024_1_1_1] | ||||||||
| ); | ||||||||
| // And an application package depends on that library package | ||||||||
| let my_app_4_5_6 = make_build!( | ||||||||
| { | ||||||||
| "pkg": "my-app/4.5.6", | ||||||||
| "build": { | ||||||||
| "options": [ | ||||||||
| { "pkg": "openimageio" }, | ||||||||
| ], | ||||||||
| }, | ||||||||
| "install": { | ||||||||
| "requirements": [ | ||||||||
| { "pkg": "openimageio", "fromBuildEnv": true }, | ||||||||
| ], | ||||||||
| }, | ||||||||
| }, | ||||||||
| [openimageio_1_2_3] | ||||||||
| ); | ||||||||
| // And a new version of the platform is published | ||||||||
| let spi_platform_2025_1_1_1 = | ||||||||
| make_build!({"pkg": "spi-platform/2025.1.1.1", "compat": "x.x.a.b"}); | ||||||||
| let repo = make_repo!([ | ||||||||
| spi_platform_2024_1_1_1, | ||||||||
| openimageio_1_2_3, | ||||||||
| my_app_4_5_6, | ||||||||
| spi_platform_2025_1_1_1, | ||||||||
| ]); | ||||||||
|
|
||||||||
| // Then we want to solve for my-app using the new platform version. | ||||||||
| // Although the only build of openimageio was built using the old platform, | ||||||||
| // it is expected to be resolvable when using the new platform. | ||||||||
| let repo = Arc::new(repo); | ||||||||
|
|
||||||||
| solver.add_repository(repo); | ||||||||
| // The platform version is specified as a build option rather than a request | ||||||||
| solver.update_options(option_map! { | ||||||||
| "spi-platform" => "~2025.1.1.1" | ||||||||
| }); | ||||||||
|
Comment on lines
+3146
to
+3148
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. IIRC this is not the same as adding actual
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Collaborator
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Host options get lumped in with the other options but they are hard requirements. 🤔
Collaborator
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. But it is true that if I add an actual var request then both solvers fail as unsolvable.
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The step solver once used the current set of options as hard requirements, but it hasn't been that way for a long time. So I think resolvo should to the same - it can guide with these if needed but shouldn't consider them constraints - only actual requests. I can only assume that the host options are also getting injected as requests through either the above or some other code path...
Collaborator
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. On a lark I tried changing the resolvo code to completely ignore options and all the tests now pass. I think this says more about the test coverage than it does about resolvo.
Collaborator
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This is the one test that calls spk/crates/spk-solve/src/solvers/solver_test.rs Line 1117 in 16abbd4
When I comment out that line the test still passes.
Collaborator
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @dcookspi pointed out that outside of the solver we are turning options into var requests, including host options so I think that explains how host options become hard requirements. spk/crates/spk-cli/common/src/flags.rs Lines 299 to 300 in 16abbd4
It also suggests that it would be a proper change to make the resolvo internals stop turning options into var requests itself. That does make the new test pass. But we need more tests to show what the expected behavior is in the presence of options. Up to now resolvo has been treating all options as hard var requirements and it has taken a decent amount of time for that to show up as a problem in practice. After the change to not turn options into var requests is in place, resolvo won't be using options for anything. I'm imagining a test where the repo has many variants of the same package with a color var set to red, blue, etc. Then if there is an option
Collaborator
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
I created a test over in #1268. |
||||||||
| solver.add_request(request!("my-app")); | ||||||||
|
|
||||||||
| let solution = run_and_print_resolve_for_tests(&mut solver).await.unwrap(); | ||||||||
| assert_resolved!(solution, "my-app", "4.5.6"); | ||||||||
| assert_resolved!(solution, "openimageio", "1.2.3"); | ||||||||
| } | ||||||||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So basically options are not used by the solver at all now, but this field is kept around to uphold the
SolverTraitcontract of being able to callupdate_optionsand then see those options withget_options.If a test is created that shows options doing something meaningful in the step solver then this field could start getting used again.