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

PubGrub resolver #1453

Open
wants to merge 1 commit into
base: spr/main/293590d9
Choose a base branch
from
Open

PubGrub resolver #1453

wants to merge 1 commit into from

Conversation

maciektr
Copy link
Contributor

@maciektr maciektr commented Jul 17, 2024

Stack:

⚠️ Part of a stack created by spr. Do not merge manually using the UI - doing so may have unexpected results.

@maciektr maciektr force-pushed the spr/main/d55b5ecb branch from 6ac7666 to 3c2d40c Compare July 17, 2024 13:00
@maciektr maciektr force-pushed the spr/main/3ba8d8f0 branch from 57c7fea to 5e3f300 Compare July 17, 2024 13:00
@maciektr maciektr changed the title PubGrub resolver initial impl, blocking, no-graph PubGrub resolver initial implementation Oct 14, 2024
@maciektr maciektr force-pushed the spr/main/d55b5ecb branch 2 times, most recently from 57efb17 to 7bf873b Compare October 20, 2024 18:40
@maciektr maciektr force-pushed the spr/main/d55b5ecb branch 2 times, most recently from 121762e to c33266d Compare October 22, 2024 09:32
@maciektr maciektr changed the base branch from spr/main/3ba8d8f0 to main October 22, 2024 13:46
@maciektr maciektr changed the title PubGrub resolver initial implementation PubGrub resolver Oct 22, 2024
@maciektr maciektr changed the base branch from main to spr/main/293590d9 October 22, 2024 13:46
@maciektr maciektr force-pushed the spr/main/d55b5ecb branch 2 times, most recently from 06a2207 to 73b73d5 Compare October 26, 2024 20:38
@maciektr maciektr force-pushed the spr/main/d55b5ecb branch 3 times, most recently from ef18fc7 to 8aba988 Compare November 4, 2024 22:52
commit-id:d55b5ecb
@maciektr maciektr requested a review from mkaput November 25, 2024 14:42
@maciektr maciektr marked this pull request as ready for review November 25, 2024 14:42
@maciektr maciektr requested a review from a team as a code owner November 25, 2024 14:42
@@ -142,6 +145,9 @@ xxhash-rust = { version = "0.8", features = ["xxh3"] }
zip = { version = "0.6", default-features = false, features = ["deflate"] }
zstd = "0.13"

[patch.'https://github.com/pubgrub-rs/pubgrub.git']
pubgrub = { git = 'https://github.com/software-mansion-labs/pubgrub.git', branch = 'dev' }
Copy link
Member

Choose a reason for hiding this comment

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

why are you doing this instead of just depending on our fork directly?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

semver-pubgrub depends on pubgrub as well 🫤

Copy link
Member

Choose a reason for hiding this comment

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

can't you change this dependency in our semver-pubgrub fork?

packages: FxOnceMap<ManifestDependency, Arc<VersionsResponse>>,
}

pub(crate) type FxOnceMap<K, V> = OnceMap<K, V>;
Copy link
Member

Choose a reason for hiding this comment

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

why?

@@ -165,6 +165,7 @@ fn fetch_with_nested_paths() {
Scarb::quick_snapbox()
.arg("fetch")
.current_dir(&t)
.timeout(std::time::Duration::from_secs(60))
Copy link
Member

Choose a reason for hiding this comment

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

why?

@@ -71,6 +71,19 @@ fn usage() {

###

GET /index/3/b/bar.json
Copy link
Member

Choose a reason for hiding this comment

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

how did it happen that this request got repeated?

let cloned_lockfile = lockfile.clone();
thread::Builder::new()
.name("scarb-resolver".into())
.spawn(move || {
Copy link
Member

Choose a reason for hiding this comment

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

consider moving this to a separate named function to: reduce indentation, have something named in stack trace

Comment on lines +24 to +25
// Backpressure is provided by at a more granular level by `DistributionDatabase`
// and `SourceDispatch`, as well as the bounded request channel.
Copy link
Member

Choose a reason for hiding this comment

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

what do these two names refer to?

mod solution;
mod state;

pub async fn resolve<'c>(
Copy link
Member

Choose a reason for hiding this comment

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

This function is the core algorithm, so I think it would be good to have it commented (at least the above the signature, and even better describe the steps of the algorithm).

fn format_error(err: PubGrubError<PubGrubDependencyProvider>) -> anyhow::Error {
match err {
PubGrubError::NoSolution(derivation_tree) => {
anyhow::format_err!(
Copy link
Member

Choose a reason for hiding this comment

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

nit: import directly

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

Successfully merging this pull request may close these issues.

3 participants