More Rust/RLS integration #42146

nrc · 2017-05-22T02:16:13Z

alexcrichton · 2017-05-22T14:07:17Z

src/Cargo.toml

+  "tools/rls/test_data/goto_def",
+  "tools/rls/test_data/highlight",
+  "tools/rls/test_data/hover",
+  "tools/rls/test_data/rename",


Would it be possible to instead specify:

exclude = [ "tools/rls/test_data", ]

?

(this is a relatively new feature of Cargo)

That doesn't work - I get "error: failed to read /home/ncameron/rust2/src/tools/rls/test_data/Cargo.toml"

sorry, no I don't. Testing again...

error:

thread '<unnamed>' panicked at 'could not create cargo workspace: current package believes it's in a workspace when it's not: current: /home/ncameron/rust2/src/tools/rls/test_data/highlight/Cargo.toml workspace: /home/ncameron/rust2/src/Cargo.toml this may be fixable by adding `tools/rls/test_data/highlight` to the `workspace.members` array of the manifest located at: /home/ncameron/rust2/src/Cargo.toml', src/libcore/result.rs:859

Bah! Mind opening an issue on Cargo for this so we can be sure to fix it for the next release? And then you can leave a comment here saying # TODO: move these to exclude or something like that for the next release.

Hm this logic makes me think that exclude could work. Are you sure the above doesn't work? I've just added a test case to Cargo locally to and this works as expected.

I just tried again after a rebase to Rust master, checked Cargo submod is also on master, and updated system Rust for luck, still get the same error. Note that I get the error on running the RLS tests - so these are test directories inside the RLS directory inside Rust. I wonder if the nesting is exposing a Cargo bug?

alexcrichton · 2017-05-22T14:09:17Z

src/bootstrap/check.rs

+
+    let mut dylib_path = dylib_path();
+    dylib_path.insert(0, build.sysroot_libdir(&compiler, host));
+    cargo.env(dylib_path_var(), env::join_paths(&dylib_path).unwrap());


Oh we've actually got a helper for this I think

alexcrichton · 2017-05-22T14:10:01Z

src/bootstrap/check.rs

+    cargo.env(dylib_path_var(), env::join_paths(&dylib_path).unwrap());
+
+    // Avoid using the rustc shim.
+    cargo.env("RUSTC", build.compiler_path(compiler));


I think it'd be sufficient do to env_remove here perhaps?

alexcrichton · 2017-05-22T14:11:02Z

src/bootstrap/check.rs

+    // Avoid using the rustc shim.
+    cargo.env("RUSTC", build.compiler_path(compiler));
+
+    build.run(cargo.env("PATH", &path_for_cargo(build, compiler)));


Does the rls work the same way as cargo in the sense that it's matching output? Cargo will print the value of RUSTC at some point during testing which the testing itself didn't expect, so Cargo's tests are basically asserting that RUSTC=rustc so that's why this was necessary. Ideally though this modification of PATH wouldn't be necessary, but do the rls tests do basically the same thing?

The RLS does not do this. However, if I don't do this PATH hack, I get an error about different versions of the bitflags crate. Possibly cargo culting and I need to change something else?

alexcrichton · 2017-05-22T14:13:49Z

src/bootstrap/step.rs

@@ -474,6 +474,10 @@ pub fn build_rules<'a>(build: &'a Build) -> Rules {
         .dep(|s| s.name("tool-cargo"))
         .host(true)
         .run(move |s| check::cargo(build, s.stage, s.target));
+    rules.test("check-rls", "src/tools/rls")


This'll need to be coupled with a change to

rust/src/bootstrap/mk/Makefile.in

Lines 57 to 65 in 81734e0

src/tools/cargotest \

cargo \

src/test/pretty \

src/test/run-pass/pretty \

src/test/run-fail/pretty \

src/test/run-pass-valgrind/pretty \

src/test/run-pass-fulldeps/pretty \

src/test/run-fail-fulldeps/pretty \

$(BOOTSTRAP_ARGS)

to make sure we run these tests on one of the bots I think.

Also, while you're at it, mind doing a bit of cleanup for Cargo? The cargo path in test("check-cargo", "cargo") above should be src/tools/cargo and the Makefile.in entry should change as well (the submodule has moved now)

nrc · 2017-05-23T09:04:04Z

@alexcrichton review commit makes requested changes (unless I commented inline).

alexcrichton · 2017-05-23T14:10:22Z

src/bootstrap/check.rs

+    // Avoid using the rustc shim.
+    cargo.env_remove("RUSTC");
+
+    build.run(cargo.env("PATH", &path_for_cargo(build, compiler)));


Hm so just to confirm, did you test not setting PATH but setting RUSTC to the actual compiler itself? (not the shim)

I tried it both ways and got (different) errors

Can you gist what you're seeing and the various combinations? This whole construction worries me a bit...

alexcrichton · 2017-05-23T14:15:36Z

src/bootstrap/check.rs

+    build.add_rustc_lib_path(compiler, &mut cargo);
+
+    // Avoid using the rustc shim.
+    cargo.env_remove("RUSTC");


Er wait hang on, I think I may have spoken too soon!

So Cargo's a pretty special flower here. I'm remembering now that we do use the shim rustc to compile Cargo itself. This correctly accounts for things like optimization flags, debuginfo flags, etc. When we run tests, however, Cargo would use the RUSTC env var configured but all of Cargo's tests actually explicitly ignore this env var relying on rustc instead. That means that the PATH modification is purely done for tests and RUSTC is purely done for compiling Cargo itself. Weird, no?

So now we've got a question for the rls itself. I believe the rls should be compiling with RUSTC (the shim) actually to get the right flags, so I believe I was mistaken to request this env_remove. Down below though it still seems that the PATH modification should not be necessary. Does the test suite for the rls tamper with RUSTC at all? If it's invoking Cargo and/or respecting RUSTC then I'd actually expect the rls to basically ignore the PATH below, always relying on RUSTC itself (a different compiler).

You mentioned errors with the bitflags crate, but some of those may have actually been very recently fixed, so maybe a rebase, remove env_remove, remove PATH changes, and see if it works?

So, baseline is with the env.remove and setting the PATH, and tests pass. If I remove both of them, I get:

thread '<unnamed>' panicked at 'could not run cargo: ChainedError { error: failed to run `rustc` to learn about target-specific information, cause: process didn't exit successfully: `/home/ncameron/rust2/build/bootstrap/debug/rustc - --crate-name ___ --print=file-names -Zunstable-options -Zcontinue-parse-after-error --error-format=json -Zsave-analysis --crate-type bin --crate-type rlib --target x86_64-unknown-linux-gnu` (exit code: 101) --- stderr error: Option 'error-format' given more than once.

For each test. I think because the rustc shim adds --error-format even though the RLS has added it already.

Although, it must do so indirectly since it is not in the shim

The exact change:

diff --git a/src/bootstrap/check.rs b/src/bootstrap/check.rs index 6da19e8..095fe08 100644 --- a/src/bootstrap/check.rs +++ b/src/bootstrap/check.rs @@ -123,9 +123,9 @@ pub fn rls(build: &Build, stage: u32, host: &str) { build.add_rustc_lib_path(compiler, &mut cargo); // Avoid using the rustc shim. - cargo.env_remove("RUSTC"); + //cargo.env_remove("RUSTC"); - build.run(cargo.env("PATH", &path_for_cargo(build, compiler))); + build.run(&mut cargo); }

I think because the rustc shim adds --error-format even though the RLS has added it already.

But error-format as a string shows up nowhere inside the src/bootstrap directory?

Something seems really fishy here?

I don't recall where, but I thought the test infrastructure sets it somewhere? IIRC, compiletest or something intercepts error messages so it can do compile-fail etc and uses json errors for that

alexcrichton · 2017-05-23T14:15:51Z

src/bootstrap/check.rs

+    let path = build.sysroot(compiler).join("bin");
+    let old_path = ::std::env::var("PATH").expect("");
+    let sep = if cfg!(windows) { ";" } else {":" };
+    format!("{}{}{}", path.display(), sep, old_path)


This should probably os os::split_paths and os::join_paths

ishitatsuyuki · 2017-05-29T04:12:02Z

@nrc mind separate and merge the submodule update first?

alexcrichton · 2017-06-01T19:07:02Z

@nrc I'm trying to check this out locally to test this out and see these errors, but it's not building? Missing commits in rustfmt? (that itself is also something we should be concerned about!)

This was added in rust-lang#38072 but I can't recall why and AFAIK Cargo already handles this. This was discovered through rust-lang#42146 where passing duplicate flags was causing problems.

alexcrichton · 2017-06-02T16:59:11Z

RUSTFLAGS duplication is because of this bug -- #42382

alexcrichton · 2017-06-02T17:06:00Z

Ok there's a lot of other fishy logic in the rls. This is why I'm hesitant to merge workaround as it seems like there's a number of legitimate bugs?

"rustc" is used bare as a program to run, which doesn't account for environment variables like RUSTC or configuration like build.rustc. This should extract the program from the ProgramBuilder? This happens elsewhere too
Environment variables are loaded at compile time instead of runtime, and may also likely be wrong for running in the rustc build environment? I'm not really sure what's going on here but loading at comiple time seems definitely wrong and you may know how this relates to running tests in the build environment as well.

bors · 2017-06-03T01:11:35Z

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

…rk-Simulacrum rustbuild: Remove RUSTFLAGS logic in rustc shim This was added in rust-lang#38072 but I can't recall why and AFAIK Cargo already handles this. This was discovered through rust-lang#42146 where passing duplicate flags was causing problems.

alexcrichton · 2017-06-08T14:59:47Z

ping @nrc, thoughts on my most recent comment?

nrc · 2017-06-15T00:43:48Z

Sorry, been busy with other things. Will get back to this soon...

nrc · 2017-06-16T00:32:42Z

"rustc" is used bare as a program to run,...

This is partly by design and partly just because we haven't got round to supporting this stuff in the RLS. The 'by design' bit is because we use our own (linked) rustc for the regular builds and letting the user specify the rustc used from Cargo seems like a good way to get incompatible crates.

I hope we can work around this by carefully setting up the rustc here in Rustbuild.

Environment variables are loaded at compile time instead of runtime

Not exactly sure what is going on there. I think that this only applies to env vars for setting up the tool chain and that this is ok because we don't support cross-compilation. However, that seems wrong because one could cross-compile the RLS.

This looks like it needs fixing in the RLS.

nrc · 2017-06-16T00:33:04Z

Oh, and sorry for taking so long to respond.

nrc · 2017-06-16T03:37:16Z

This commit fixes the compile time/runtime env vars issue: rust-lang/rls@c750c66

nrc · 2017-06-16T03:37:38Z

Updating the RLS is blocked on rust-lang/cargo#4178

ishitatsuyuki · 2017-06-16T03:46:38Z

You can merge the branch beforehand and git will handle the duplicates afterward.

bors · 2017-07-13T00:46:58Z

📌 Commit d023e75 has been approved by @alexcrichton

Mark-Simulacrum · 2017-07-13T00:48:00Z

@bors r=alexcrichton

bors · 2017-07-13T00:48:01Z

💡 This pull request was already approved, no need to approve it again.

There's another pull request that is currently being tested, blocking this pull request: Change language in Travis configuration to shell #43198

bors · 2017-07-13T00:48:01Z

📌 Commit d023e75 has been approved by alexcrichton

bors · 2017-07-13T07:24:23Z

⌛ Testing commit d023e7508e977b5f28bdf90b665eb6782a65d92c with merge 62d527953e7cb73db46385257636f52c0ae55f22...

bors · 2017-07-13T08:06:08Z

💔 Test failed - status-travis

alexcrichton · 2017-07-13T14:26:06Z

@bors: retry

…

On Thu, Jul 13, 2017 at 3:06 AM, bors ***@***.***> wrote: 💔 Test failed - status-travis <https://travis-ci.org/rust-lang/rust/builds/253095953?utm_source=github_status&utm_medium=notification> — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#42146 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AAD95DaSww1ksDftL3EYCrZwW-iowMSoks5sNdARgaJpZM4Nhz21> .

bors · 2017-07-14T07:36:15Z

⌛ Testing commit d023e7508e977b5f28bdf90b665eb6782a65d92c with merge 75042aaf587e2ac3245b9221df111719ea45f91b...

bors · 2017-07-14T08:52:08Z

💔 Test failed - status-travis

kennytm · 2017-07-14T09:34:30Z

x86_64-gnu-aux jobserver test failed?

[01:09:56]      Running build/x86_64-unknown-linux-gnu/stage2-tools/x86_64-unknown-linux-gnu/release/deps/jobserver-5e749bf41aad8cc2
[01:09:56] 
[01:09:56] running 3 tests
[01:09:57] test jobserver_and_j ... ok
[01:09:57] test jobserver_exists ... FAILED
[01:09:58] test makes_jobserver_used ... ok
[01:09:58] 
[01:09:58] failures:
[01:09:58] 
[01:09:58] ---- jobserver_exists stdout ----
[01:09:58] 	running `/checkout/obj/build/x86_64-unknown-linux-gnu/stage2-tools/x86_64-unknown-linux-gnu/release/cargo build`
[01:09:58] thread 'jobserver_exists' panicked at '
[01:09:58] Expected: execs
[01:09:58]     but: exited with exit code: 101
[01:09:58] --- stdout
[01:09:58] 
[01:09:58] --- stderr
[01:09:58]    Compiling foo v0.0.1 (file:///checkout/obj/build/x86_64-unknown-linux-gnu/stage2-tools/x86_64-unknown-linux-gnu/cit/t0/foo)
[01:09:58] error: failed to run custom build command for `foo v0.0.1 (file:///checkout/obj/build/x86_64-unknown-linux-gnu/stage2-tools/x86_64-unknown-linux-gnu/cit/t0/foo)`
[01:09:58] process didn't exit successfully: `/checkout/obj/build/x86_64-unknown-linux-gnu/stage2-tools/x86_64-unknown-linux-gnu/cit/t0/foo/target/debug/build/foo-fc5bedfb6ea40096/build-script-build` (exit code: 101)
[01:09:58] --- stderr
[01:09:58] thread 'main' panicked at 'called `Result::unwrap()` on an `Err` value: NotPresent', /checkout/src/libcore/result.rs:860:4
[01:09:58] note: Run with `RUST_BACKTRACE=1` for a backtrace.
[01:09:58] 
[01:09:58] ', /cargo/registry/src/github.com-1ecc6299db9ec823/hamcrest-0.1.1/src/core.rs:31:12
[01:09:58] note: Run with `RUST_BACKTRACE=1` for a backtrace.
[01:09:58] 
[01:09:58] 
[01:09:58] failures:
[01:09:58]     jobserver_exists
[01:09:58] 
[01:09:58] test result: FAILED. 2 passed; 1 failed; 0 ignored; 0 measured; 0 filtered out
[01:09:58] 
[01:09:58] error: test failed, to rerun pass '--test jobserver'

bors · 2017-07-15T13:30:05Z

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

nrc · 2017-07-17T05:24:25Z

@bors r=alexcrichton

I'm not sure if the jobserver test failure is real, but I can't repro locally, it looks vaguely suspicious to me and no-one was around on irc to help, so I figured it was worth trying again.

bors · 2017-07-17T05:24:26Z

📌 Commit 04415dc has been approved by alexcrichton

bors · 2017-07-17T05:25:54Z

⌛ Testing commit 04415dc with merge 88cf76a...

@alexcrichton

More Rust/RLS integration r? @alexcrichton cc rust-lang/rls#310 closes #41199 closes #41197

bors · 2017-07-17T07:59:48Z

☀️ Test successful - status-appveyor, status-travis
Approved by: alexcrichton
Pushing 88cf76a to master...

cc rust-lang/rust#42146

rust-highfive assigned alexcrichton May 22, 2017

alexcrichton reviewed May 22, 2017

View reviewed changes

carols10cents added the S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. label May 22, 2017

alexcrichton reviewed May 23, 2017

View reviewed changes

nrc mentioned this pull request May 24, 2017

workspaces: exclude not working in Rustbuild rust-lang/cargo#4089

Closed

nrc force-pushed the rls-rust branch from cde147f to 13ba2cf Compare May 30, 2017 03:17

nrc force-pushed the rls-rust branch from 13ba2cf to 86e5e3e Compare June 1, 2017 23:00

alexcrichton mentioned this pull request Jun 2, 2017

rustbuild: Remove RUSTFLAGS logic in rustc shim #42382

Merged

nrc mentioned this pull request Jul 13, 2017

Stability tracking - RLS rust-dev-tools/dev-tools-team#16

Open

11 tasks

nrc added 2 commits July 17, 2017 17:21

Remove exception from license check for strings.rs

2579793

Run RLS tests

04415dc

nrc force-pushed the rls-rust branch from d023e75 to 04415dc Compare July 17, 2017 05:23

bors added a commit that referenced this pull request Jul 17, 2017

Auto merge of #42146 - nrc:rls-rust, r=alexcrichton

88cf76a

More Rust/RLS integration r? @alexcrichton cc rust-lang/rls#310 closes #41199 closes #41197

bors merged commit 04415dc into rust-lang:master Jul 17, 2017

This was referenced Jul 17, 2017

Add warnings when rustdoc html rendering differs #41991

Merged

Rework Rustbuild to an eagerly compiling approach #43059

Merged

matklad added a commit to matklad/rls that referenced this pull request Jul 20, 2018

Make the workspace hack local to RLS

c61d1ce

cc rust-lang/rust#42146

matklad added a commit to matklad/rls that referenced this pull request Jul 20, 2018

Make the workspace hack local to RLS

20a1e26

cc rust-lang/rust#42146

matklad added a commit to matklad/rls that referenced this pull request Jul 20, 2018

Make the workspace hack local to RLS

b622868

cc rust-lang/rust#42146

matklad added a commit to matklad/rls that referenced this pull request Jul 20, 2018

Make the workspace hack local to RLS

d0c3e3a

cc rust-lang/rust#42146

matklad mentioned this pull request Jul 20, 2018

Make the workspace hack local to RLS rust-lang/rls#952

Merged

	src/tools/cargotest \
	cargo \
	src/test/pretty \
	src/test/run-pass/pretty \
	src/test/run-fail/pretty \
	src/test/run-pass-valgrind/pretty \
	src/test/run-pass-fulldeps/pretty \
	src/test/run-fail-fulldeps/pretty \
	$(BOOTSTRAP_ARGS)

More Rust/RLS integration #42146

More Rust/RLS integration #42146

Conversation

nrc commented May 22, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nrc commented May 23, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ishitatsuyuki commented May 29, 2017

alexcrichton commented Jun 1, 2017

alexcrichton commented Jun 2, 2017

alexcrichton commented Jun 2, 2017

bors commented Jun 3, 2017

alexcrichton commented Jun 8, 2017

nrc commented Jun 15, 2017

nrc commented Jun 16, 2017

nrc commented Jun 16, 2017

nrc commented Jun 16, 2017

nrc commented Jun 16, 2017

ishitatsuyuki commented Jun 16, 2017

bors commented Jul 13, 2017

Mark-Simulacrum commented Jul 13, 2017

bors commented Jul 13, 2017

bors commented Jul 13, 2017

bors commented Jul 13, 2017

bors commented Jul 13, 2017

alexcrichton commented Jul 13, 2017 via email

bors commented Jul 14, 2017

bors commented Jul 14, 2017

kennytm commented Jul 14, 2017

bors commented Jul 15, 2017

nrc commented Jul 17, 2017

bors commented Jul 17, 2017

bors commented Jul 17, 2017

bors commented Jul 17, 2017