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

Add 6000 words of documentation on The Algorithm #298

Merged
merged 2 commits into from
Aug 22, 2022

Conversation

Gankra
Copy link
Contributor

@Gankra Gankra commented Aug 19, 2022

No description provided.

@Gankra
Copy link
Contributor Author

Gankra commented Aug 19, 2022

(Haven't actually tried building this, sleep time)

Comment on lines 118 to 120
All of our analysis derives from the output of [cargo metadata][] and [cargo_metadata][]'s
interpretation of that, so it's worth discussing how we use it, and what we believe to
be true of its output.
Copy link
Collaborator

Choose a reason for hiding this comment

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

I think this should probably be "our interpretation of that" rather than "cargo_metadata's interpretation of that"


Cargo metadata produces the build graph in a kind of awkward way where some information
for the packages is in `"packages"` and some information is in `"resolve"`, and we need
to manually compute lots of facts like "roots", "only for tests", and "[topological sort][]"
Copy link
Collaborator

Choose a reason for hiding this comment

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

the topological sort link here appears to be broken in github's renderer, but it might be fine in ours.

and therefore require a desugarring to produce "fake" nodes, are *workspace members*.
These are the packages that will be tested if you run `cargo test --workspace`.

Actually doing this desugarring is really messy, because a lot of things about the "real"
Copy link
Collaborator

Choose a reason for hiding this comment

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

(I could easily be wrong on this, but) Isn't it "desugaring" rather than "desugarring"?

describe happens in a for loop).

If all we care about is finding out if a package has some criteria, then all
we need to do is run depth-first-search ([DFS][]) from the Root Node and see if it reaches
Copy link
Collaborator

Choose a reason for hiding this comment

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

I'm not sure if the code is technically a depth-first search anymore given that it's now using a priority queue rather than a stack (since #296). Heaps don't preserve node order in the same way that a stack would.

I suppose it's more of a Best First Search but with the edge weights simplified because no successor can have better hereustics than its parent, and the hereustics have nothing to do with distance to target (though that is a potential thing we could do, by adding a distance into the sorting key)

Comment on lines 611 to 614
It's also worth noting that, due to audit-as-crates-io (and maybe other shenanigans
like patches?) you can end up in a situation where a third-party depends on a
first-party, or third-parties have `policy.criteria` entries. This is why
Step 3a is interleaved with Step 2.
Copy link
Collaborator

Choose a reason for hiding this comment

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

Yup, [patch] can also do this.

@mystor
Copy link
Collaborator

mystor commented Aug 22, 2022

The test failure is existing, and I've put up a fix in #301

@mystor mystor merged commit ba66c53 into mozilla:main Aug 22, 2022
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.

2 participants