Cargo: "Documenting..." messages should use `\` on windows

[carols10cents]

Discovered in https://github.com/rust-lang/cargo/pull/4627#issuecomment-339289446, the "Documenting" messages aren't using \ on windows and should be:

failures:
---- doc_multiple_targets_same_name stdout ----
    running `C:\projects\cargo\target\debug\cargo.exe doc --all`
thread 'doc_multiple_targets_same_name' panicked at '
Expected: execs
    but: expected to find:
[DOCUMENTING] foo v0.1.0 (file:///C:/projects/cargo/target/cit/t10/foo[/]foo)
did not find in output:
 Documenting bar v0.1.0 (file:///C:/projects/cargo/target/cit/t10/foo/bar)
 Documenting foo v0.1.0 (file:///C:/projects/cargo/target/cit/t10/foo/foo)
    Finished dev [unoptimized + debuginfo] target(s) in 0.68 secs
', C:\Users\appveyor\.cargo\registry\src\github.com-1ecc6299db9ec823\hamcrest-0.1.1\src\core.rs:31:12
note: Run with `RUST_BACKTRACE=1` for a backtrace.

[carols10cents]

@matklad it looks like the "Compiling" messages don't use windows paths either, should they?

I'm basing this on the observation that this test, and many others, don't use square brackets either....

[matklad]

@carols10cents Yeah, I think ideally all paths should use file separators, native to the current platform.

But this is an ultra cosmetic things actually, so it might be better to fix something else instead of this :)

[hban]

Are you talking about file URLs? Those use forward slashes even on Windows, AFAIK. For example, calling UrlCreateFromPath on "C:\D\E F\G.txt" will produce "file:///C:/D/E%20F/G.txt".

[matklad]

@hban that makes sense actually... Which actually brings the question of why do we show URLs here?

I think that instead of

 Documenting foo v0.1.0 (file:///C:/projects/cargo/target/cit/t10/foo/foo)

The user should see

 Documenting foo v0.1.0 (C:\projects\cargo\target\cit\t10\foo\foo)

[matklad]

If anyone wants to fix it, the relevant Display impl is over here:

https://github.com/rust-lang/cargo/blob/a5b13e82db645bb5af33fc59d67da63ea30a17af/src/cargo/core/source/source_id.rs#L366

We need to make sure that this impl is not relied on by anything important, for example, that it doesn't accidentally leak to the lockfile. It seems to be the case, but it's better to double check.

Note that after the change the paths in Documenting ... and in lockfile will look slightly different, but that is probably not problematic.

[dwijnand]

If this happens we'll want to update https://doc.rust-lang.org/cargo/reference/specifying-dependencies.html#overriding-dependencies (search for "note the file:// in the build output")

[zachlute]

So, I started working on this. Got the change done and seems to be working fine, but as I was updating tests I ran into a question.

https://github.com/rust-lang/cargo/blob/b15ee255e5cf625f15ff3f5ecdc6efb76e402768/tests/testsuite/alt_registry.rs#L364

If I fix this to work with the change, by removing the file://, does that still accurately test that the registry is using the local file and not something else (here and in a bunch of other places)?

Another example:

https://github.com/rust-lang/cargo/blob/b15ee255e5cf625f15ff3f5ecdc6efb76e402768/tests/testsuite/overrides.rs#L279

Is just changing that to ([..]) fine or does that invalidate the test?

[dwijnand]

Thanks for picking this up!

I would consider using:

let root = if cfg!(windows) { r#"C:\"# } else { "/" };

in front of the [..].

Btw, beware of this change: https://github.com/rust-lang/cargo/commit/d5fc8dc3a77b39d7bb3a5671c539d06b9dcc61fa#diff-b03a0182b2cc2abbd16483fb77b37a6d.

[zachlute]

So, that check doesn't quite work because the file may be on a different drive. The closest I've got right now is a function in tests\testsuite\support\mod.rs that looks like this:

/// Gets a match expression suitable for matching the root directory on the platform.
pub fn root_match () -> &'static str {
    // On Windows, we need to use a wildcard for the drive,
    // because we don't actually know what it will be.
    if cfg!(windows) { r#"[..]:\"# } else { "/" }
}

And then at each of the call sites you need to do something like this:

p.cargo("build")
        .with_stderr(format!(
            "\
[UPDATING] registry `{root}[..]`
[COMPILING] bar v0.1.0 ({root}[..])
[COMPILING] foo v0.0.1 (CWD)
[FINISHED] dev [unoptimized + debuginfo] target(s) in [..]
", root=root_match()),
        ).run();

I can do that, but it's a bunch of now-failing tests to update, so I figured I'd check to see if that sounded crazy before I did the work.

As for the CWD thing, I think it already just kind of accidentally works because the CWD replacement is done both for the URL and for the actual CWD path, which will match what what we're converting the URL to.

[dwijnand]

So the CWD macro is doing two things, both of which against you (in a sense).

First of all it replaces, as you said, both the URL and the path, exactly what this issue is looking to change, so it makes all the tests agnostic to this change.

But, more importantly, that includes the root, which means that that match won't match. That's what I think is causing those now-failing tests.

So you're going to kind of have to find a way to avoid the CWD macro, or opt-out of it, or something 😕.

[zachlute]

Ah, I see. That makes sense. But that’s because the replace is happening on the output. Could we change it to do “CWD -> Actual CWD Path” on the input?

Maybe that’s the solution to the other problem, too. Instead of replacing at the call site, just expand the uh...’template system’ to include ‘[[ROOT]]’ (and probably add some delimiters like that) and then do the match replacement in the evaluation.

Thoughts?

[dwijnand]

If you can make it work, sure!