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

Use workspaces and switch to a single Cargo.lock. #36032

Closed
wants to merge 1 commit into from
Closed

Use workspaces and switch to a single Cargo.lock. #36032

wants to merge 1 commit into from

Conversation

ahmedcharles
Copy link
Contributor

This involves hacking the code used to run cargo test on various
packages, because it reads Cargo.lock to determine which packages should
be tested. This change implements a blacklist, since that will catch new
crates when they are added in the future.

@rust-highfive
Copy link
Collaborator

r? @brson

(rust_highfive has picked a reviewer for you, use r? to override)

@ahmedcharles
Copy link
Contributor Author

r? @alexcrichton

And does anyone know why unwind is picked as the root crate in the Cargo.lock?

@rust-highfive rust-highfive assigned alexcrichton and unassigned brson Aug 27, 2016
@alexcrichton
Copy link
Member

Thanks for the PR @ahmedcharles! One of my original intentions was that adding a crate was super easy (e.g. didn't require unduly modifying config files), so I'm a little uneasy with these manual lists. I'm afraid it'll make for confusing errors when adding a crate to the compiler or another tool unfortunately.

One interesting point, though, is that libstd's and libtest's crate DAGs are quite stable, unlike the compiler's. I wonder if we could implement a hardcoded list of those crates, and avoid having to write down the compiler's DAG as well? I'm envisioning something like this:

  • First, read Cargo.lock to determine what set of crates are path dependencies (no source listed)
  • If testing libstd or libtest, pass -p for the hardcoded list of crates in that step we already know about.
  • If testing librustc, take all names minus libstd minus libtest minus known tools, and pass them all as -p flags.

This is all kinda unfortunate to do, but hopefully rust-lang/rfcs#1133 will make it easier to implement this as we won't have to split apart the suites.

@ahmedcharles
Copy link
Contributor Author

Isn't the issue here that bootstrap is forced to read Cargo.lock to determine dependencies rather than have it issue a cargo command which returns a list of dependencies? My preferred solution would be to have a cargo command that returns a list of dependencies and perhaps has modifiers for only listing path ones, or perhaps listing which type they are in the output, so it can be filtered. (I'd be willing to try making this change in cargo, if you agree and have pointers.)

I'm not actually sure how that RFC is related to this at all.

If there's a change in cargo in the foreseeable future, then I don't really think it matters much what gets hardcoded today, since it will only last until the next cycle before it can be improved.

@alexcrichton
Copy link
Member

Yeah we just need to know what set of -p arguments to pass through to Cargo. The closest command is likely cargo metadata, but even that may not have all the pieces we're looking for. Conceptually there are actually three workspaces here, not one, which may cause some confusion with that.

The RFC would allows to take these three "pseudo workspaces" and move them all into one, so we can just pass -p with all crates and it'll "just work". That is, we can stop sequencing cargo within one stage of the compiler (which will be nice!).

I'd prefer to not land this until we alleviate the restriction of having to modify the build system when adding a crate. Right now we don't have that restriction, and having a workspace isn't really buying us that much as it's all managed internally anyway, so I'd consider this a step backwards if we can't figure it out for now.

@ahmedcharles
Copy link
Contributor Author

I was under the impression that this would be necessary for getting rustbuild into a better shape for distribution by distros, since it's mentioned in #34687. I assumed that was more than 'casual' priority, hence why I worked on this. :)

If using a single workspace is the desired approach, I'd prefer to be able to make forward progress, somehow. If I can write bootstrap in a way that doesn't require updating on crate additions, probably based on cargo metadata, will that be considered for acceptance? My perspective is that if I want to test all of my local crates in one go, how do I do that with cargo today, assuming a workspace with multiple crates? I'd rather solve the general problem, if that makes sense? Perhaps even a cargo test --local-recusive would be useful for running tests on path dependencies? Though, I guess a cargo workspace test would also work for building/testing everything in the workspace.

As an aside: I'm still not sure how the RFC about making cargo aware of libstd dependencies (to fix cross compiling) will help it tell you how to test a set of path dependences of a given crate (in a generic way)? They seem like two completely distinct problems, so I'm clearly missing something here still. Note, the issue with this commit isn't specific to libstd, it's IMO, a lack of support for testing either path dependencies or workspaces.

@alexcrichton
Copy link
Member

Oh right, yes! I had forgotten about rustbuild + distros.

So thinking about it more, I think cargo metadata may actually work well here. You'll basically want to call that, rebuild the dependency graph, and then you can use that to figure out all the crates that are path dependencies that need to be tested for each suite. Let me know if you need help with this!

I'm still not sure how the RFC about making cargo aware of libstd dependencies (to fix cross compiling) will help it tell you how to test a set of path dependences of a given crate (in a generic way)?

I wouldn't worry about it, it's not relevant to this PR anyway.

@bors
Copy link
Contributor

bors commented Sep 1, 2016

☔ The latest upstream changes (presumably #35718) made this pull request unmergeable. Please resolve the merge conflicts.

@Ericson2314
Copy link
Contributor

Ericson2314 commented Sep 1, 2016

As far as I'm concerned, this is a great foundation upon which to continue working after rust-lang/rfcs#1133 :).

@bors
Copy link
Contributor

bors commented Sep 3, 2016

☔ The latest upstream changes (presumably #35957) made this pull request unmergeable. Please resolve the merge conflicts.

@alexcrichton
Copy link
Member

@ahmedcharles any update here about using cargo metadata to avoid hardcoded lists?

@ahmedcharles
Copy link
Contributor Author

I've been busy with work lately, but I'll get back to this soon.

@alexcrichton
Copy link
Member

Ok no worries! Let me know if any surprises come up.

@bors
Copy link
Contributor

bors commented Sep 14, 2016

☔ The latest upstream changes (presumably #35021) made this pull request unmergeable. Please resolve the merge conflicts.

This involves hacking the code used to run cargo test on various
packages, because it reads Cargo.lock to determine which packages should
be tested. This change implements a blacklist, since that will catch new
crates when they are added in the future.
@bors
Copy link
Contributor

bors commented Sep 22, 2016

☔ The latest upstream changes (presumably #36551) made this pull request unmergeable. Please resolve the merge conflicts.

Manishearth added a commit to Manishearth/rust that referenced this pull request Oct 8, 2016
Leverage Cargo workspaces in rustbuild

This is a continuation of rust-lang#36032 which implements the change to use `cargo metadata` to learn about the crate graph.
@alexcrichton
Copy link
Member

Landed with #37016

@ahmedcharles ahmedcharles deleted the cargo branch March 24, 2017 03:45
djrenren pushed a commit to djrenren/compiletest that referenced this pull request Aug 26, 2019
Leverage Cargo workspaces in rustbuild

This is a continuation of rust-lang/rust#36032 which implements the change to use `cargo metadata` to learn about the crate graph.
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.

6 participants