Use linked compiler during Cargo `exec()` callback by Xanewok · Pull Request #424 · rust-lang/rls

Xanewok · 2017-07-25T22:44:42Z

This is a first step to not be as reliant on Cargo and makes it so RLS uses linked compiler instead of rustc or $RUSTC process.
Passing analysis data from the compiler should be faster in few cases since analysis data will be passed in-memory, rather than unconditionally via file on disk. Additionally cargo routine can now collect diagnostics messages and display them until subsequent bare rustc build routine is performed.

Xanewok · 2017-07-25T22:54:48Z

src/actions/mod.rs

-                        if let Some(new_analysis) = new_analysis {
-                            analysis.reload_from_analysis(new_analysis, &project_path_clone, &cwd, false).unwrap();
-                        } else {
+                        if new_analysis.is_empty() {


After each crate compilation rustc can return an Option with analysis data and we collect unwrapped payloads into a vector, which we then load from file. For a single crate the behaviour is the same, since it's a change None => vec![] and Some(analysis) => vec![analysis]. However with this, now Cargo routine can collect and load the analysis data. There's one what-if: if a compiled indirect dependency will return None for analysis but there will be some collected into a vector, then a missing one won't be loaded from disk, since it'll only try to load the provided analysis in-memory, and possible cause subtle problems resolving the data? @nrc is such a case possible?

It is rare for there to be no save-analysis info, even if we fail to compile, so I expect this will not be a problem. In any case, it should be resolved once we build successfully, so it should only be temporary.

nrc · 2017-07-26T06:11:08Z

src/build/cargo.rs

+                _ => {}
+            }
+        } else {
+            cmd.status().expect("Couldn't execute rustc");


I think we must do this before storing the args/envs into the compialtion_cx

I see that I made it so the lock on compilation_cx is unnecessarily held for the whole duration of compilation, I'll fix that.

Besides that, should we set it after in case cmd.status() or rustc::rustc() panics and we don't store bogus values from faulty compilation? If not, then I think this may not be a problem, since we feed the values directly to rustc::rustc and it doesn't touch it nor does cmd.status() require it.

nrc · 2017-07-26T06:13:06Z

src/build/cargo.rs


+        if self.workspace_mode {
+            let args = &compilation_cx.args;
+            let envs = &compilation_cx.envs;


I don't see how these ever get populated since we never do a Cargo run without running rustc directly in workspace mode? How does this work?

Only cargo routine populates the args and envs, rustc consumes them externally. In workspace mode currently only ever cargo routine is called, which then prepares RLS-specific args/envs from those generated by Cargo itself as usual. Then it feeds them directly to, introduced in this change, rustc::rustc (unlike being passed via Command API and actually executing the command). In practice it doesn't use use compilation_cx.{args,envs} at all, however it doesn't prevent it from being set here

ok, going to take another look, I think I misunderstood how it all works

nrc · 2017-07-26T06:16:51Z

src/test/mod.rs

+                                       // however order of received messages is non-deterministic and this
+                                       // would require implementing something like `or_expect_contains`
+                                       ExpectedMessage::new(None).expect_contains("publishDiagnostics"),
+                                       ExpectedMessage::new(None).expect_contains("publishDiagnostics"),


Ideally, I think we should only send a single publish/end pair, rather than one per crate. But that can be fixed later. You should check for a second diagnosticsEnd though

Are you sure? There is only one requested build here, so there's only one pair of diagnosticsBegin/diagnosticsEnd and multiple publishDiagnostics in between because there are multiple files (issued here while iterating every file => diagnostics results pair), confirmed with output now

oh sorry, I was confused - I thought they were multiple diagnosticsBegin, not publishDiagnostics. Then this is doing what I expect.

Xanewok · 2017-07-26T17:16:00Z

src/build/cargo.rs

-        }
-
        // Finally, store the modified cargo-generated args/envs for future rustc calls
        args.insert(0, rustc_exe);


@nrc while we're at it, do you know why we need to pass a rustc exe as a first argument to the linked compiler?

we don't need to pass it, but it is so that the args look like what would happen if you took the args from inside the program (there you get the executable name as the first arg). We could not insert it and strip that arg if we ever still take the args from inside the program (I'm not sure we do).

Oh, okay, I thought run_compiler accepts args with the first program argument omitted, nevermind then 👍

Xanewok · 2017-07-26T20:33:02Z

As a note, test_simple_workspace is failing right now because of a env-related deadlock caused by locking in run_cargo followed by running rustc::rustc since 79d659e. Environment should be handled in a more principled way.

nrc · 2017-07-27T02:24:55Z

OK, I think I understand better what you're doing and it looks good. Just need to fix up the locking on the env and it should be good to go.

Xanewok · 2017-07-28T12:38:55Z

Force-pushed a version with two locks (one for Cargo, one for rustc scopes) that fixes the workspace deadlock. Unfortunately right now this still can fail (what's interesting that it only seems to affect find_all_refs tests?). I don't think it's good to try and fix that and devise a more complex and foolproof lock in the code that will be practically only needed by the tests, so I pushed the current version for now.

Since more work in general and the more complete workspace support will probably mean more work involving env vars, I'll try to move relevant tests into separate processes so we don't have to work around env var as much and unnecessarily.

Xanewok · 2017-07-28T22:59:56Z

I'll get working on more foolproof locking for the tests during the weekend and hopefully will get back whenever I manage to do something with it.

Xanewok · 2017-07-31T18:16:24Z

Pushed a prototype version of EnvironmentLock, which is supposed to be double-layered lock that ensures consistent env vars during the build and testing.
The lock has to be double-layered, since we'll begin to call linked rustc during cargo, and not only do we need to provide consistent and mutually-exclusive access to env vars during cargo routine, but also during it, we need to do the same for multiple rustc calls while holding the outer/parent lock.

Naive approach of doing two separate locks for each procedure won't work, because our builds has 2 modes: bare rustc and cargo (which can now run nested rustc as well). There can be a situation where we start a cargo build in one RLS server instance (multiple instances run during testing) and before we get to the nested rustc lock, some other instance can start building while acquiring only rustc lock. That's why any env var locking required for the build must go through the same lock (regardless of whether it's cargo or bare rustc build routine) and only then, while holding it, another one (inner) can be acquired if needed for additional calls.

I'm currently using EnvironmentLockFacade to pass appropriate outer/inner mutex access to underlying rustc and cargo procedures, while the actual Mutexes (in EnvironmentLock as ENV_LOCK) are initialized statically, ensuring single access across many spawned language services.

It's only a prototype and requires more work, like organizing code better in general, encapsulating the logic a bit more, so we can ensure to the best of our ablities that the locks will be acquired in correct order and it needs proper comments when the design will be set in stone.

Xanewok · 2017-07-31T20:35:17Z

src/build/cargo.rs

-    let mut restore_env = Environment::push(&env);
+    // There can be at most one Cargo instance ran, so don't guard env vars with a lock in case
+    // unerlying rustc calls will require mutually exclusive access to this process' env vars
+    // FIXME: Don't lock me. This is only required for tests and shouldn't modify program's behaviour


To be removed, forgot about this comment

Xanewok · 2017-08-01T21:41:56Z

Pushed a reorganized and documented version of the double mutex. For this, I separated this into util::environment module to hide the implementation details and clean up in general. More implementation details are in the description of 5bd732f (the last commit with the lock).

@nrc does this version look better? I tried to encode the none -> outer -> inner lock access order via types, but locking order is still not guaranteed. However, I think it's a lot more clear and more ergonomical (?) now, for what it's worth 🙂

nrc

I need to come back and do a proper review, but I'm feeling fuzzy. Here are some initial comments

nrc · 2017-08-02T02:37:14Z

src/util/environment.rs

@@ -0,0 +1,141 @@
+use std::env;


lets keep this in the build module, rather than util (and the file needs the copyright header)

I wanted to separate it into a different module to hide the impl details and this seemed abstract enough to put under util module. Can I put it in the build module but under inline module to achieve the same visibility semantics (so the build/cargo modules can't access private fields)?

yes, that sounds perfect

nrc · 2017-08-02T02:38:05Z

src/util/mod.rs

@@ -0,0 +1 @@
+pub mod environment;


Although we don't need this with the previous comment, for future reference, you don't need a separate file here, you could use an inline module in the parent.

nrc · 2017-08-02T02:41:11Z

src/build/cargo.rs

+    let outer_lock = env_lock.as_outer();
+    let (outer, inner) = outer_lock.get_lock();
+    // Lock early to guarantee synchronized access to read RUSTFLAGS env var
+    let env_lock_guard = outer.lock().unwrap();


Could we encapsulate this stuff in the EnvironmentLock class and expose descriptively named API instead of low-level locking primitives?

@nrc what should the API return? MutexGuard<'a, ()> (newtyped?) instead of &Mutex<()>? I just realized that returning a scoped value in this case does not require providing explicit lifetime to the lock structs, so this will be an improvement.
In general, though, I'd love to limit the lifetime of InnerLock to the one of the guard that's returned alongside of it, but since it's passed to Arc<Executor>, I'm not sure that's possible.

I imagine you would have your own guard struct that you would return, rather than returning either the mutex itself or a guard.

Xanewok · 2017-08-02T09:14:44Z

Pushed updated and rebased version, hope I addressed some points you wanted.
The lock etc. are still under different module to hide private data and such, but the module is in build now.
I know the lock implementation probably still leaves much to be desired, but if it's acceptable for now in the current form, I'd love for the PR to be merged, mainly because of the linked compiler changes in workspace mode. If needed, I can improve the lock or work on a way to handle scoped, synchronized environment properly later.
However, if necessary, I'll obviously address further issues if there will be any with the current implementation.

Xanewok · 2017-08-03T09:16:07Z

macOS build got stuck, retrying

nrc · 2017-08-04T06:05:12Z

I've read through and I think this looks OK. I'd be interested in trying to iterate on the env lock a bit more, but I can't imagine how to do that without playing with the code myself. So, lets land this as is. Could you rebase please?

This introduces a new `build::environment` module with moved `Environment` RAII guard and new global `EnvironmentLock` and related. The lock has to be a double one, because we might need doubly scoped env vars (outer one for Cargo routine and inner one for underlying rustc calls across different threads, contained within Cargo routine invocation scope). While the lock itself and provided types don't enforce a strict lock ordering, the order of retrieved locks and types is still encoded in the type system. This might not be a complete and fully abstracted away solution to N-valued mutex and guarded environment, but works good enough and seems to cover enough cases in both production and testing scenarios.

Xanewok · 2017-08-05T15:25:33Z

Rebased now

nrc · 2017-08-05T23:25:14Z

Thanks for the rebase!

Xanewok commented Jul 25, 2017

View reviewed changes

Xanewok force-pushed the linked-compiler branch from c65df46 to a97fd89 Compare July 25, 2017 22:56

nrc reviewed Jul 26, 2017

View reviewed changes

Xanewok commented Jul 26, 2017

View reviewed changes

Xanewok force-pushed the linked-compiler branch from a97fd89 to 1aa9f3a Compare July 26, 2017 20:31

Xanewok force-pushed the linked-compiler branch from 1aa9f3a to 505276c Compare July 28, 2017 12:29

Xanewok mentioned this pull request Jul 28, 2017

Split into lib/bin and prepare for integration tests under tests/ dir #426

Closed

Xanewok force-pushed the linked-compiler branch from 505276c to 157ad60 Compare July 31, 2017 18:07

Xanewok commented Jul 31, 2017

View reviewed changes

Xanewok force-pushed the linked-compiler branch from 157ad60 to 5bd732f Compare August 1, 2017 21:37

nrc reviewed Aug 2, 2017

View reviewed changes

Xanewok force-pushed the linked-compiler branch from 5bd732f to 6d30069 Compare August 2, 2017 09:10

Xanewok force-pushed the linked-compiler branch from 6d30069 to 7790a96 Compare August 3, 2017 07:53

Xanewok closed this Aug 3, 2017

Xanewok reopened this Aug 3, 2017

Xanewok added 3 commits August 4, 2017 10:05

Use linked compiler during Cargo exec() callback

b5a1fb6

Check for multiple target diagnostics in workspace test

02f8744

Xanewok force-pushed the linked-compiler branch from 7790a96 to a9f5f25 Compare August 4, 2017 08:12

nrc merged commit c4dac14 into rust-lang:master Aug 5, 2017

Xanewok deleted the linked-compiler branch August 6, 2017 17:57

Conversation

Xanewok commented Jul 25, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Xanewok Jul 26, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Xanewok Jul 26, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Xanewok commented Jul 26, 2017

Uh oh!

nrc commented Jul 27, 2017

Uh oh!

Xanewok commented Jul 28, 2017

Uh oh!

Xanewok commented Jul 28, 2017

Uh oh!

Xanewok commented Jul 31, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Xanewok commented Aug 1, 2017

Uh oh!

nrc left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Xanewok commented Aug 2, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Xanewok commented Aug 3, 2017

Uh oh!

nrc commented Aug 4, 2017

Uh oh!

Xanewok commented Aug 5, 2017

Uh oh!

nrc commented Aug 5, 2017

Uh oh!

Reviewers

Assignees

Labels

Projects

Xanewok Jul 26, 2017 •

edited

Loading

Xanewok Jul 26, 2017 •

edited

Loading

Xanewok commented Jul 31, 2017 •

edited

Loading

Xanewok commented Aug 2, 2017 •

edited

Loading