Compile tests for multiple packages #1828

fhahn · Jul 24, 2015

This PR for #1528 is still pretty rough (and so far only added multiple package support for tests), but I wanted to make sure the overall approach is fine.

I have made some progress and cargo is now able to compile and execute tests for multiple packages.

In order to execute the doc tests for multiple packages as well, I added to_doc_test to Compilation (used here). Previously it only executed the doc tests for Compilation.package. Another option would be to change Compilation.package to be a Vec<Package>, because in order to execute the tests of multiple packages, multiple packages are compiled as "top level package". But this would require a changes at a couple of other places.

rust-highfive · Jul 24, 2015

r? @alexcrichton

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

alexcrichton · Jul 24, 2015

alexcrichton reviewed Jul 24, 2015

View changes

alexcrichton · Jul 24, 2015

src/bin/test.rs

@@ -26,7 +26,7 @@ pub const USAGE: &'static str = "
 Execute all unit and integration tests of a local package

 Usage:
-    cargo test [options] [--] [<args>...]
+    cargo test [options] [-p SPEC --package SPEC]... [--] [<args>...]


cc @BurntSushi, you mentioned on docopt/docopt#275 that you were thinking of proposing the ability to do this in the options section instead of up here in the usage section, do you know if there's been progress on that? I wouldn't mind proposing an idea or two on that thread to see if I can get discussion rolling again, it'd be great to fix :)

@alexcrichton Thanks for the reminder! I have a candidate implementation working in 0.6.69. See my proposal for details: docopt/docopt#275 (Even though it's working in the Rust implementation, I probably wouldn't add it into Cargo quite yet in case the proposal changes. If it's ultimately rejected, I might still leave it in because I think it's really really useful.)

Sounds good to me, thanks @BurntSushi!

alexcrichton · Jul 24, 2015

alexcrichton reviewed Jul 24, 2015

View changes

alexcrichton · Jul 24, 2015

src/cargo/ops/cargo_compile.rs

+fn resolve_dependencies<'a>(options: &CompileOptions<'a>,
+                 package: &Package,
+                 source: Option<Box<Source + 'a>>
+                 ) -> (Vec<Package>, Resolve, SourceMap<'a>) {


Stylistically these kinds of functions are formatted as this in cargo:

fn foo(a: A, b: B) -> Ret { // ...

alexcrichton · Jul 24, 2015

alexcrichton reviewed Jul 24, 2015

View changes

alexcrichton · Jul 24, 2015

src/cargo/ops/cargo_compile.rs

+                 package: &Package,
+                 source: Option<Box<Source + 'a>>
+                 ) -> (Vec<Package>, Resolve, SourceMap<'a>) {
+    let override_ids = source_ids_from_config(options.config, package.root()).unwrap();


All the unwrap() calls in this function should be converted to try!

alexcrichton · Jul 24, 2015

alexcrichton reviewed Jul 24, 2015

View changes

alexcrichton · Jul 24, 2015

src/cargo/ops/cargo_compile.rs

+    };
+    let pkgid = pkgids.first().unwrap();
+    let to_build = packages.iter().find(|p| p.package_id() == *pkgid).unwrap();
+    let to_builds = packages.iter().filter(|p| pkgids.iter().find(|&op| *op == p.package_id()).is_some()).collect::<Vec<&Package>>();


Can you be sure that lines are word-wrapped to 80 characters in Cargo?

alexcrichton · Jul 24, 2015

alexcrichton reviewed Jul 24, 2015

View changes

alexcrichton · Jul 24, 2015

src/cargo/ops/cargo_rustc/mod.rs

                                   build_config, profiles));

    let mut queue = JobQueue::new(cx.resolve, deps, cx.jobs());

+    let _p = profile::start("preparing build directories");


This ends up finishing the profiling when it's dropped, which is now at the end of the function (which is why this wanted an explicit scope)

alexcrichton · Jul 24, 2015

alexcrichton reviewed Jul 24, 2015

View changes

alexcrichton · Jul 24, 2015

src/cargo/ops/cargo_rustc/mod.rs

        try!(cx.prepare(pkg, targets));
        prepare_init(&mut cx, pkg, &mut queue, &mut HashSet::new());
        custom_build::build_map(&mut cx, pkg, targets);
+        try!(compile(targets, pkg, &mut cx, &mut queue));
    }

    // Build up a list of pending jobs, each of which represent compiling a
    // particular package. No actual work is executed as part of this, that's
    // all done next as part of the `execute` function which will run
    // everything in order with proper parallelism.


This comment is now dangling.

alexcrichton · Jul 24, 2015

Yeah this all looks like a pretty good approach to me!

bors · Aug 4, 2015

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

fhahn · Sep 6, 2015

I finally had time to work on this PR again. I have started adding tests, but I am not sure how to test cargo test and cargo bench, because the output order is non deterministic for multiple packages (e.g. test_cargo_test::test_multiple_packages) Any ideas whats a good way to test this would be?

bors · Sep 8, 2015

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

alexcrichton · Sep 9, 2015

The nondeterminism may be solved by -j1, but if that doesn't work then just testing that they're accepted should be fine.

alexcrichton · Sep 9, 2015

alexcrichton reviewed Sep 9, 2015

View changes

alexcrichton · Sep 9, 2015

src/bin/bench.rs

@@ -26,7 +26,7 @@ pub const USAGE: &'static str = "
 Execute all benchmarks of a local package

 Usage:
-    cargo bench [options] [--] [<args>...]
+    cargo bench [options] [-p SPEC --package SPEC]... [--] [<args>...]


I think that new syntax in docopt may actually allow for nicer definitions of this, something like:

Options: -p, --package=SPEC ... Description ...

I believe it's still somewhat experimental, but this may be a good place to get a good start on it?

alexcrichton · Sep 9, 2015

alexcrichton reviewed Sep 9, 2015

View changes

alexcrichton · Sep 9, 2015

src/cargo/ops/cargo_compile.rs

@@ -48,7 +48,7 @@ pub struct CompileOptions<'a> {
    /// Flag if the default feature should be built for the root package
    pub no_default_features: bool,
    /// Root package to build (if None it's the current one)
-    pub spec: Option<&'a str>,
+    pub spec: Vec<&'a str>,


This may be much easier to take if it's a &'a [String], that way all the commands above can avoid casting and collection (e.g. they're just &options.flag_package

alexcrichton · Sep 9, 2015

alexcrichton reviewed Sep 9, 2015

View changes

alexcrichton · Sep 9, 2015

src/cargo/ops/cargo_compile.rs

        if let Some(source) = source {
            registry.preload(package.package_id().source_id(), source);
        } else {
-            try!(registry.add_sources(&[package.package_id().source_id()
-                                               .clone()]));
+            try!(registry.add_sources(&[package.package_id().source_id().clone()]));


Were the changes around here necessary? (looks to be pretty similar)

alexcrichton · Sep 9, 2015

alexcrichton reviewed Sep 9, 2015

View changes

alexcrichton · Sep 9, 2015

src/cargo/ops/cargo_compile.rs

            dev_deps: true, // TODO: remove this option?
            features: &features,
-            uses_default_features: !no_default_features,
+            uses_default_features: !options.no_default_features,


Same as above, probably good to cut down to as few changes as possible.

(ping on this)

alexcrichton · Sep 9, 2015

alexcrichton reviewed Sep 9, 2015

View changes

alexcrichton · Sep 9, 2015

src/cargo/ops/cargo_compile.rs

+            id
+        }).filter(|id|
+            id.is_ok()
+        ).map(|id| id.unwrap()).collect::<Vec<&PackageId>>()


This may be better expressed as:

spec.iter().filter_map(|p| { match resolved_with_overrides.query(p) { Ok(p) => Some(p), Err(..) => { invalid_spec.push(p); None } } }).collect::<Vec<_>>();

I'm also a little hesitant about throwing away the error information, I think invalid specifications will want to retain the error information about why they were invalid

alexcrichton · Sep 9, 2015

alexcrichton reviewed Sep 9, 2015

View changes

alexcrichton · Sep 9, 2015

src/cargo/ops/cargo_compile.rs

+                                 invalid_spec.join(", "))));
+    }
+
+    let pkgid = pkgids[0];
    let to_build = packages.iter().find(|p| p.package_id() == pkgid).unwrap();


Isn't this variable obsolete now? There could be more than one package being built?

alexcrichton · Sep 9, 2015

alexcrichton reviewed Sep 9, 2015

View changes

alexcrichton · Sep 9, 2015

src/cargo/ops/cargo_rustc/mod.rs

@@ -54,8 +54,9 @@ pub struct TargetConfig {

 // Returns a mapping of the root package plus its immediate dependencies to
 // where the compiled libraries are all located.
-pub fn compile_targets<'a, 'cfg: 'a>(targets: &[(&'a Target, &'a Profile)],
-                                     pkg: &'a Package,
+pub fn compile_targets<'a, 'cfg: 'a>(pkg_targets: &'a Vec<(&Package,


&'a Vec<T> should basically always be avoided in favor of &'a [T] explicitly.

alexcrichton · Sep 9, 2015

alexcrichton reviewed Sep 9, 2015

View changes

alexcrichton · Sep 9, 2015

src/cargo/ops/cargo_rustc/mod.rs

-    if targets.is_empty() {
+
+    let &(pkg, _) = pkg_targets.first().unwrap();
+    if pkg_targets.iter().any(|&(_, ref targets)| targets.is_empty()) {


This isn't quite right any more I think because singling out the first package isn't really possible any more. This entire if may actually just be able to go away.

alexcrichton · Sep 9, 2015

alexcrichton reviewed Sep 9, 2015

View changes

alexcrichton · Sep 9, 2015

src/cargo/ops/cargo_rustc/mod.rs

-    } else {
-        deps.iter().find(|p| p.package_id() == resolve.root()).unwrap()
-    };
+    let root = deps.iter().find(|p| p.package_id() == resolve.root()).unwrap();


I think this logic will need to be updated, the notion of a "root" package doesn't really exist any more, so those which are depending on it probably need to be updated.

At the moment -p SPEC only builds packages that are dependencies of the "root" package, and all generated binaries are stored relative to that root package. I think this root package is mostly used to get a base path for the build output and I think this notation should still be valid?

Hm the placement of the output directory should be relatively independent from the root package (e.g. it's just a default), as in once the output directory is chosen the root package is never needed after that.

I'll try to audit for usage of this regardless, though, to see what's up.

alexcrichton · Sep 9, 2015

alexcrichton reviewed Sep 9, 2015

View changes

alexcrichton · Sep 9, 2015

src/cargo/ops/cargo_rustc/mod.rs

                                   build_config, profiles));

    let mut queue = JobQueue::new(cx.resolve, deps, cx.jobs());

-    // Prep the context's build requirements and see the job graph for all
-    // packages initially.
    {
        let _p = profile::start("preparing build directories");


This profile marker isn't quite profiling the same thing any more, could you be sure to keep the scope the same as it was before?

alexcrichton · Sep 9, 2015

alexcrichton reviewed Sep 9, 2015

View changes

alexcrichton · Sep 9, 2015

src/cargo/ops/cargo_rustc/mod.rs

-                cx.compilation.libraries.insert(pkgid.clone(), v);
+
+                // Include immediate lib deps as well
+                for dep in cx.dep_targets(pkg, target, kind, profile).iter() {


This addition of .iter() I don't think is necessary

alexcrichton · Sep 27, 2015

Ok, I think those failures on CI are spurious (only seen 1/3 before...), but could you also squash these commits down? It's ok to have a few, but I'd prefer to avoid "Address @alexcrichton's feedback" commits if possible :)

fhahn · Sep 30, 2015

I've squashed the commits down to 6, but I could reduce the number even further if necessary.

alexcrichton · Sep 30, 2015

Looks like this may have accidentally picked up some other commits, perhaps a rebase is in order? I'll give this one last look-through but I'll probably just send it to bors.

alexcrichton · Sep 30, 2015

Now that I've actually gotten around to taking a peek, this looks fantastic to me. Thanks so much again for pushing on this @fhahn!

@bors: r+ d9615d7

bors · Sep 30, 2015

⌛️ Testing commit d9615d7 with merge 7931e58...

bors · Sep 30, 2015

☀️ Test successful - cargo-linux-32, cargo-linux-64, cargo-mac-32, cargo-mac-64, cargo-win-gnu-32, cargo-win-gnu-64, cargo-win-msvc-32, cargo-win-msvc-64

fhahn · Oct 1, 2015

Great, thanks @alexcrichton for your patience :)

rust-highfive assigned alexcrichton Jul 24, 2015

fhahn force-pushed the fhahn:multiple-package-parameters branch from 8b8df3e to 64a8cb0 Jul 24, 2015

fhahn force-pushed the fhahn:multiple-package-parameters branch from 64a8cb0 to 44957f6 Jul 24, 2015

fhahn force-pushed the fhahn:multiple-package-parameters branch from a56fd80 to 38335d8 Aug 2, 2015

fhahn force-pushed the fhahn:multiple-package-parameters branch 3 times, most recently from 35c6833 to 4f7f409 Sep 6, 2015

fhahn force-pushed the fhahn:multiple-package-parameters branch from a4e53be to 2ebf892 Sep 30, 2015

fhahn force-pushed the fhahn:multiple-package-parameters branch from 2ebf892 to 016e971 Sep 30, 2015

fhahn added some commits Sep 15, 2015

Add support for multiple -p options to `cargo update`
closes #1528

f0647ea

Use std::env::EXE_SUFFIX in new tests

Loading status checks…

d9615d7

fhahn force-pushed the fhahn:multiple-package-parameters branch from 016e971 to d9615d7 Sep 30, 2015

bors merged commit d9615d7 into rust-lang:master Sep 30, 2015
2 of 3 checks passed

2 of 3 checks passed

continuous-integration/travis-ci/pr The Travis CI build is in progress
Details

continuous-integration/appveyor AppVeyor build succeeded
Details

homu Test successful
Details

bors referenced this pull request Sep 30, 2015
Closed
Add `cargo rustdoc` for passing arbitrary flags to rustdoc #1977

fhahn deleted the fhahn:multiple-package-parameters branch Oct 1, 2015

SimonSapin referenced this pull request Oct 1, 2015
Merged
Run all unit tests with just one Cargo command. #7813

ehuss referenced this pull request Mar 31, 2019
Merged
Allow `cargo doc --open` with multiple packages. #6803

rust-lang/cargo

Join GitHub today

Compile tests for multiple packages #1828

Conversation

fhahn commented Jul 24, 2015

rust-highfive assigned alexcrichton Jul 24, 2015

This comment has been minimized.

rust-highfive commented Jul 24, 2015

fhahn force-pushed the fhahn:multiple-package-parameters branch from 8b8df3e to 64a8cb0 Jul 24, 2015

alexcrichton reviewed Jul 24, 2015 View changes

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

alexcrichton reviewed Jul 24, 2015 View changes

This comment has been minimized.

alexcrichton reviewed Jul 24, 2015 View changes

This comment has been minimized.

alexcrichton reviewed Jul 24, 2015 View changes

This comment has been minimized.

alexcrichton reviewed Jul 24, 2015 View changes

This comment has been minimized.

alexcrichton reviewed Jul 24, 2015 View changes

This comment has been minimized.

This comment has been minimized.

alexcrichton commented Jul 24, 2015

fhahn force-pushed the fhahn:multiple-package-parameters branch from 64a8cb0 to 44957f6 Jul 24, 2015

fhahn force-pushed the fhahn:multiple-package-parameters branch from a56fd80 to 38335d8 Aug 2, 2015

This comment has been minimized.

bors commented Aug 4, 2015

fhahn force-pushed the fhahn:multiple-package-parameters branch 3 times, most recently from 35c6833 to 4f7f409 Sep 6, 2015

This comment has been minimized.

fhahn commented Sep 6, 2015

This comment has been minimized.

bors commented Sep 8, 2015

This comment has been minimized.

alexcrichton commented Sep 9, 2015

alexcrichton reviewed Sep 9, 2015 View changes

This comment has been minimized.

alexcrichton reviewed Sep 9, 2015 View changes

This comment has been minimized.

alexcrichton reviewed Sep 9, 2015 View changes

This comment has been minimized.

alexcrichton reviewed Sep 9, 2015 View changes

This comment has been minimized.

This comment has been minimized.

alexcrichton reviewed Sep 9, 2015 View changes

This comment has been minimized.

alexcrichton reviewed Sep 9, 2015 View changes

This comment has been minimized.

alexcrichton reviewed Sep 9, 2015 View changes

This comment has been minimized.

alexcrichton reviewed Sep 9, 2015 View changes

This comment has been minimized.

alexcrichton reviewed Sep 9, 2015 View changes

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

alexcrichton reviewed Sep 9, 2015 View changes

This comment has been minimized.

alexcrichton reviewed Sep 9, 2015 View changes

This comment has been minimized.

This comment has been minimized.

alexcrichton commented Sep 27, 2015

fhahn force-pushed the fhahn:multiple-package-parameters branch from a4e53be to 2ebf892 Sep 30, 2015

sfackler and others added some commits Sep 27, 2015

fhahn force-pushed the fhahn:multiple-package-parameters branch from 2ebf892 to 016e971 Sep 30, 2015

This comment has been minimized.

fhahn commented Sep 30, 2015

fhahn added some commits Sep 15, 2015

fhahn force-pushed the fhahn:multiple-package-parameters branch from 016e971 to d9615d7 Sep 30, 2015

This comment has been minimized.

alexcrichton commented Sep 30, 2015

This comment has been minimized.

alexcrichton commented Sep 30, 2015

bors added a commit that referenced this pull request Sep 30, 2015

This comment has been minimized.

bors commented Sep 30, 2015

This comment has been minimized.

bors commented Sep 30, 2015

Hide details View details bors merged commit d9615d7 into rust-lang:master Sep 30, 2015 2 of 3 checks passed

fhahn force-pushed the fhahn:multiple-package-parameters branch from `8b8df3e` to `64a8cb0` Jul 24, 2015

alexcrichton reviewed Jul 24, 2015

View changes

alexcrichton reviewed Jul 24, 2015

View changes

alexcrichton reviewed Jul 24, 2015

View changes

alexcrichton reviewed Jul 24, 2015

View changes

alexcrichton reviewed Jul 24, 2015

View changes

alexcrichton reviewed Jul 24, 2015

View changes

fhahn force-pushed the fhahn:multiple-package-parameters branch from `64a8cb0` to `44957f6` Jul 24, 2015

fhahn force-pushed the fhahn:multiple-package-parameters branch from `a56fd80` to `38335d8` Aug 2, 2015

fhahn force-pushed the fhahn:multiple-package-parameters branch 3 times, most recently from `35c6833` to `4f7f409` Sep 6, 2015

alexcrichton reviewed Sep 9, 2015

View changes

alexcrichton reviewed Sep 9, 2015

View changes

alexcrichton reviewed Sep 9, 2015

View changes

alexcrichton reviewed Sep 9, 2015

View changes

alexcrichton reviewed Sep 9, 2015

View changes

alexcrichton reviewed Sep 9, 2015

View changes

alexcrichton reviewed Sep 9, 2015

View changes

alexcrichton reviewed Sep 9, 2015

View changes

alexcrichton reviewed Sep 9, 2015

View changes

alexcrichton reviewed Sep 9, 2015

View changes

alexcrichton reviewed Sep 9, 2015

View changes

fhahn force-pushed the fhahn:multiple-package-parameters branch from `a4e53be` to `2ebf892` Sep 30, 2015

fhahn force-pushed the fhahn:multiple-package-parameters branch from `2ebf892` to `016e971` Sep 30, 2015

fhahn force-pushed the fhahn:multiple-package-parameters branch from `016e971` to `d9615d7` Sep 30, 2015

bors merged commit `d9615d7` into rust-lang:master Sep 30, 2015
2 of 3 checks passed