main_binary takes minutes on first run

[epage]

cargo test takes a long time with because the first run of main_binary takes a long time while holding the cargo locking, blocking other tests

Example

running 6 tests
test blackbox_backup ... test blackbox_backup has been running for over 60 seconds
test blackbox_help ... test blackbox_help has been running for over 60 seconds
test blackbox_help ... ok
test blackbox_no_args ... ok
test clean_error_on_non_archive ... ok
test empty_archive ... ok
test incomplete_version ... ok
test blackbox_backup ... ok

From https://ci.appveyor.com/project/sourcefrog/conserve/builds/20036005

Cause

• If a bare cargo test is run, cargo uses target/debug for the OUT_DIR
• main_binary passes --target <TRIPLET> to cargo which changes the OUT_DIR to target/<TRIPLE>/debug
  • This means cargo tests build artifacts cannot be reused and those parts need to be rebuilt

Background:

In Issue #4 we had to choose between performance and correctness (if you do specify --target, main_binary should run that target's bin) and sided on correctness

Workarounds

1. Call into escargot without passing in current_target

2. Call cargo test --target <TRIPLE>

[killercup]

I noticed that, too.

Workaround 3: Only pass --target when it's different from the target used to compile the test.

[epage]

Workaround 3: Only pass --target when it's different from the target used to compile the test.

Could you clarify this?

The trade off is between when a user explicitly lists a --target and how cargo handles the default target.

As a possible hack in assert_cmd, even if I had a lookup table of default targets (keyed off of the current OS), there is still a trade off. Some people in their CI always pass --target (if doing it for one build target, its easier to do it for all), we'd have to choose between hurting that person's performance and hurting the performance of the person running a bare cargo test.

I guess another hack is to have a build script that looks up TARGET_DIR and if it matches a certain pattern, pass current_target or not.

[killercup]

My idea was to try and detect if cargo was run with a --target arg -- if so, pass that ourselves, otherwise omit it. My hope is that leads to cargo using the same target directory that was used when building the tests itself.

(Cargo also sets a TARGET env var, but only for build scripts it seems.)

[epage]

As far as I know, there is no direct way to detect --target. TARGET is unconditionally set. So, as mentioned earlier, the best I can figure is inferring whether --target was passed in based on other information.

[killercup]

Ah I see, sorry for the confusion. This a kinda unfortunate situation.

I wonder how common the explicit --target is, and if the cargo team might be able to help us?

[epage]

I wonder how common the explicit --target is, and if the cargo team might be able to help us?

If someone is creating a CLI using trust, its exclusively used in the CI while (I'm assuming) rarely used by the developer.

Once I have time to get back to cargo-tarball, we'll be able to lower our reliance on --target in a CI for simple cases but it will still be important for someone who wants to offer a lot of choice to their end users.

[epage]

@matklad says the following should be guaranteed to work for bin targets

// Adapted from
// https://github.com/rust-lang/cargo/blob/485670b3983b52289a2f353d589c57fae2f60f82/tests/testsuite/support/mod.rs#L507
fn target_dir() -> PathBuf {
    env::current_exe().ok().map(|mut path| {
        path.pop();
        if path.ends_with("deps") {
            path.pop();
        }
        path
    }).unwrap()
}

fn cargo_review_deps_exe() -> PathBuf {
    target_dir().join(format!("cargo-review-deps{}", env::consts::EXE_SUFFIX))
}

fn base_cmd() -> Assert {
    Assert::command(&[&cargo_review_deps_exe()])
        .with_args(&["review-deps"])
}

Source: assert-rs/assert_cli#118 (comment)

This would effectively give us auto-detection of --target which would fix the problems we are having.

Downsides

• No support for main_binary. We'd always have to take in a path
  • We could use env!("CARGO_PKG_NAME") but that is not guaranteed to work. Is it good enough? Maybe a name change to pkg_binary? This has the bonus of working regardless of the number of bins in the crate.
• Doesn't help us with examples.
  • I'm concerned about having split-behavior API; two seemingly similar functions acting in very different ways. We would have to expose bin targets one way and non-bin another way.
  • One workaround is to tell people to directly use escargot for examples, bypassing us having to choose the policy of current_target or not
• Sounds like it won't work during integration tests
  • Probably a good thing for people to only be running these during integration tests anyways
  • But we can't enforce that as far as I'm aware
  • My biggest concern is this "happening to work" for someone but they might sometimes be running stale binaries.

[epage]

#67 is forcing me in making a decision I've been procastinating.

Right now, we have a tension between #4 and #57. matklad's proposal resolves that tension but it would make cargo_bin function differently than main_binary and cargo_example such that I'm considering removing them both and telling people to use escargot if the want something besides cargo_bin.

In summary, matklad's approach would

• auto deduce --target and --features (main_binary should reuse the caller's target #4)
• no performance penalty from target or features being out of sync (main_binary takes minutes on first run #57)
• no cargo overhead (main_binary takes ~300ms per invocation #6) to be worked around with caching
• would require dropping main_binary and cargo_example but we'd point people to escargot.

Any thoughts?