Support projects with both bin and lib crate types#363
Conversation
|
@nrc As a side note, those two tests that I added cause an ICE related to save-analysis, might be worth looking into. |
I try to track these things in the rls-analysis repo. They typically need fixing in rustc though. It is helpful to have a minimal test case, the easiest way to do so is to just use rustc on a file with |
|
The test problem is |
nrc
left a comment
There was a problem hiding this comment.
I think this is basically doing the right thing, a few comments inline for things that need fixing
src/build.rs
Outdated
| if let (Ok(home), Ok(toolchain)) = (home, toolchain) { | ||
| Some(format!("{}/toolchains/{}", home, toolchain)) | ||
| } else { | ||
| return env::var("SYSROOT") |
src/build.rs
Outdated
| // Finally store the modified cargo-generated args/envs for future rustc calls | ||
| args.insert(0, rustc_exe.clone()); | ||
| *self.cmd_line_args.lock().unwrap() = args; | ||
| *self.cmd_line_envs.lock().unwrap() = cmd.get_envs().clone(); |
There was a problem hiding this comment.
It makes me a little nervous to lock these two without releasing the other - there is a small risk of deadlock. I think the best solution is actually to have the two fields combined under a single Arc<Mutex<_>>
There was a problem hiding this comment.
I'm not 100% sure how rvalues work in Rust, but I assumed that when we don't bind a value, its scope is maximally limited, so here the MutexGuard would only be limited to the scope of the statement and released before the next lock. Even so, we still could theoretically, unfortunately, interleave updates here. I'll try changing that to a single Mutex, as you're suggesting
| // so the dep-info is ready by the time we return from this callback. | ||
| // Since ProcessBuilder doesn't allow to modify args, we need to create | ||
| // our own command here from scratch here. | ||
| let rustc_exe = env::var("RUSTC").unwrap_or("rustc".to_owned()); |
There was a problem hiding this comment.
It makes me a bit sad to duplicate this code - given the string literals it feels like an easy error for someone to modify one instance without modifying the other.
There was a problem hiding this comment.
Do you mean duplicating it here and here? I could extract it, but hopefully getting rustc executable name here is only temporary.
src/build.rs
Outdated
| // Since ProcessBuilder doesn't allow to modify args, we need to create | ||
| // our own command here from scratch here. | ||
| let rustc_exe = env::var("RUSTC").unwrap_or("rustc".to_owned()); | ||
| let mut cmd_dep_info = Command::new(&rustc_exe); |
There was a problem hiding this comment.
Given that we run this command to get more than just dep info, it should be renamed
src/build.rs
Outdated
| // Becase we only try to emulate `cargo test` using `cargo check`, so for now | ||
| // assume crate_type arg (i.e. in `cargo test` it isn't specified for --test targets) | ||
| // and build test harness only for final crate type | ||
| let crate_type = crate_type.expect("no crate-name in rustc command line"); |
There was a problem hiding this comment.
should "crate-name" be "crate-type"?
|
Rebased and force-pushed. Chose |
|
Requires #371 for i686 target on AppVeyor. |
nrc
left a comment
There was a problem hiding this comment.
Looks good! I had one minor nit inline, and it needs a rebase, sorry
src/build.rs
Outdated
| let cmd_line_envs = self.cmd_line_envs.lock().unwrap(); | ||
| self.rustc(&*cmd_line_args, &*cmd_line_envs, build_dir) | ||
| let compile_cx = self.compilation_cx.lock().unwrap(); | ||
| let (args, envs) = (&(*compile_cx).args, &(*compile_cx).envs); |
There was a problem hiding this comment.
This line looks a bit weird - I would make it two lets without a tuple. I'm surprised you'd need the explicit &* too
|
Split (args, envs) as you asked, but didn't toy with &* yet, as with recent submodule change I'm having trouble compiling and will be able to look later at it. |
Another attempt at #208.
This took way longer than I'd like to admit. 😢
The main change here is that instead of doing bare dep-info call (except for build scripts, which we don't intercept at all) for every crate type in our current package, we add our own flags based on server analysis configuration. Since we try to emulate cargo test mode while we're basically running cargo check, I'm passing --cfg test here for anything that's pulled by our final crate type (currently lib if build_lib is set, default cargo check target otherwise) and --test for final crate type. This means we check our primary target as if we were compiling with test harness, in cfg_test mode. We still cache args/envs only for our primary target.
Changed detecting build script target from checking if arguments contain --emit=dep-info,link to checking if crate name is build_script_build, because I think it's more descriptive and it'll allow us to intercept more calls (like separate test harnesses in CompileMode::Test etc.) later on.