link_deps() behaves poorly with spaces in paths

[cuviper]

Spaced paths are common on Windows, e.g. "C:\Program Files", but this is getting broken into separate arguments. Given "-L C:\Program Files...", rustc says "error: multiple input filenames provided", as "Files..." is considered separate from the "-L".

Ref: https://ci.appveyor.com/project/cuviper/rayon/build/1.0.14/job/k65psqsgq50nt4dy

[laumann]

Is this in link_deps()? The only path-handling uses std::env::split_paths() which seems to work correctly with spaces:

fn main() {
    let path = "/full/path/with spaces;/another/space example";
    for p in std::env::split_paths(path) {
        println!("{:?}", p);
    }
}

gives:

"/full/path/with spaces"
"/another/space example"

Could the problem be TestCx::split_maybe_args()? It is used to split up different argument strings (including target_rustcflags), but I would not expect it to work with spaces :-)

[cuviper]

Well, the problem starts from using link_deps(), wherever the actual bug may lie. 😄

Note that your "{:?}" (Debug) added the quotation marks though.

So link_deps() flattens to target_rustcflags, and split_maybe_args() tries to split it again... I'm sure it will have no idea whether a space is part of an argument or a separator. Maybe link_deps() should populate a proper vector of arguments instead of flattening?

[laumann]

Well, the problem starts from using link_deps(), wherever the actual bug may lie. 😄

Wait, so it was working with paths with spaces before introducing link_deps()? The only thing that link_deps() should do is populate target_rustcflags - how was it populated before?

Goes looking at the referenced PR

Ah, I think I see it. In the PR:

-    config.target_rustcflags = Some("-L ../target/debug/ -L ../target/debug/deps/".to_owned());
+    config.link_deps();

target_rustcflags did not contain paths with spaces before.

I think the problem is split_maybe_args. An immediate workaround is to revert the above change, but that might not be a satisfactory solution 😄 To be honest, I don't think paths with spaces ever worked in compiletest, it's just never been used.

Thanks for reporting this! I think it might be a good change to push back to rustc's compiletest.

[cuviper]

Right. We were only adding the cargo target dirs before, not all of PATH -- I'll just go back to that, not a big deal. But since your README now recommends link_deps(), I gave it a shot.

[laumann]

I'll add it as a caveat to using link_deps() in the README for the time being. Sorry for the trouble, and thanks again for reporting it.

[laumann]

Fortunately, this is really easy to reproduce, even on Linux. Just create a directory with spaces in the name and run cargo test in the test-project. I'll try to fix it soon. In the meantime, I've added a note to the README (see #82).

[LPGhatguy]

This looks like it's still broken in the latest release of compiletest-rs. Has there been any progress, or maybe a documented workaround?

This breaks compiletest on Windows completely.

[laumann]

@LPGhatguy It has not been fixed yet. If you read the discussion, I think we worked out what the correct fix should be, but it is potentially a rather big breaking change, and I haven't had time to sit down and implement it.