Workspaces #409
Workspaces #409
Conversation
src/build.rs
Outdated
| trace!("manifest_path: {:?}", manifest_path); | ||
| //let root = find_root_manifest_for_wd(options.flag_manifest_path, config.cwd())?; |
src/build.rs
Outdated
| // TODO: Keep this structure cached and regenerate it on every relevant configuration change | ||
| let rls_config = rls_config.lock().unwrap().clone(); | ||
|
|
||
| let opts = CargoOptions::new(&rls_config); |
There was a problem hiding this comment.
I don't really get why you use the CargoOptions struct? It seems to just be adding boilerplate
src/build.rs
Outdated
| target_string = target.clone(); | ||
| opts.target = Some(&target_string); | ||
| } | ||
| opts |
There was a problem hiding this comment.
We seem to lose a lot of logic here, is that intentional?
src/build.rs
Outdated
| } else { | ||
| for package in &opts.package { | ||
| if let None = ws.members().find(|x| x.name() == package) { | ||
| warn!("cargo - couldn't find member package `{}` specified in \"analyze_package\" configuration", package); |
There was a problem hiding this comment.
nit: I would use backticks rather than quotes
src/build.rs
Outdated
| if let Some(ref build_bin) = rls_config.build_bin { | ||
| let mut bins = cur_pkg_targets.iter().filter(|x| x.is_bin()); | ||
| if let None = bins.find(|x| x.name() == build_bin) { | ||
| warn!("cargo - couldn't find binary `{}` specified in \"build_bin\" configuration", build_bin); |
|
@nrc some of your comments are for the old version, where the points raised are addressed/more clear in the final version. Should I address those anyway? |
src/build.rs
Outdated
| let mut flags = "-Zunstable-options -Zsave-analysis --error-format=json \ | ||
| -Zcontinue-parse-after-error".to_owned(); | ||
| #[derive(Debug)] | ||
| struct CargoOptions { |
There was a problem hiding this comment.
Cargo in its API expects references to values that will live at least as long as compile_with_exec, so I just wanted to logically bundle them together
Only if they're still relevant. I worked out about halfway through review that some were addressed later. Just ignore anything that is already addressed. You should probably squash your commits here. |
src/build.rs
Outdated
| } | ||
| } | ||
|
|
||
| fn new(config: &Config) -> CargoOptions { |
There was a problem hiding this comment.
Initializing those vars inline was quite awkward, since var declaration had to be in a right order to satisfy lifetimes/drop order and was initially not fully defined here, also the logic in itself was non-trivial, so with addition of a new mode and conditional initialization of the those config variables, I just went with new while filling the blanks with default values, structuring it a little bit more and trying to convey the intent more
|
@nrc addressed the point about |
Yep, sgtm |
|
Rebased and pushed the final version |
|
|
||
| use build::{Internals, BufWriter, BuildResult, CompilationContext}; | ||
| use config::Config; | ||
| use super::rustc::convert_message_to_json_strings; |
There was a problem hiding this comment.
If this function is going to be shared, it would be better to move it to mod.rs
| /// analysis and diagnostics will be provided | ||
| member_packages: Mutex<HashSet<PackageId>>, | ||
| /// JSON compiler messages emitted for each primary compiled crate | ||
| compiler_messages: Arc<Mutex<Vec<String>>>, |
There was a problem hiding this comment.
Given these fields are only used in workspace mode, we should use an enum for the mode rather than a bool and have these as fields on an enum variant. (We can do this later, doesn't need to block merging now since these fields will probably change as you work on this feature).
|
Great, thanks for the feedback and merging! |
This is a prototype implementation for workspace support (#132). It introduces these new options:
workspace_mode- new behaviour is opt-in via this flaganalyze_package- specified which package will be analyzed in workspace modeThe only thing that's not behind a
workspace_modecheck of some sorts is detection of package root manifest path here It uses only .json file save-analysis data and uses Cargo exclusively, so there's a lot of room for possible optimizations.Tested on webrender workspace and simple_workspace, which is added as a simple test case.
Known issues