Include more info the rustc invocation/compilation #19
Conversation
src/lib.rs
Outdated
| pub struct CrateSource { | ||
| pub dylib: Option<String>, | ||
| pub rlib: Option<String>, | ||
| pub rmeta: Option<String>, |
There was a problem hiding this comment.
What exactly goes in these paths? Having the full path doesn't seem useful since we'd need to relocate, so I guess just the filenames. Will these always be the same when built on a different machine?
Why do we need to know what kind of crate was used?
Can there ever be more than one of these per crate (i.e., could this be an enum)?
src/lib.rs
Outdated
| #[derive(Debug, Clone)] | ||
| pub struct CompilationOptions { | ||
| pub invocation: String, | ||
| pub crate_name: Option<String>, |
src/lib.rs
Outdated
| pub invocation: String, | ||
| pub crate_name: Option<String>, | ||
| pub test: bool, | ||
| pub sysroot: Option<PathBuf>, |
src/lib.rs
Outdated
| pub struct CompilationOptions { | ||
| pub invocation: String, | ||
| pub crate_name: Option<String>, | ||
| pub test: bool, |
There was a problem hiding this comment.
This seems a bit ad hoc. Would it be better to have a 'target' field or something more generic?
src/lib.rs
Outdated
| /// must be non-None. | ||
| /// | ||
| /// Mimicks CrateSource from src/librustc/middle/cstore.rs (only paths) | ||
| /// TODO: Uses String to be easily debuggable, change to PathBuf |
There was a problem hiding this comment.
Presumably this should be addressed.
src/lib.rs
Outdated
| pub dylib: Option<PathBuf>, | ||
| pub rlib: Option<PathBuf>, | ||
| pub rmeta: Option<PathBuf>, | ||
| } |
There was a problem hiding this comment.
I know we discussed this, but I'm not sure if we got to any kind of conclusion. IIRC, there's only ever multiple sources for std and we only want this info to avoid parsing the command line, but even then it would be better to use the crate name rather than the path. Is that right? Is there anything we need to make the disctinction between these kinds of crate sources?
There was a problem hiding this comment.
The main use for this was to figure out the dependencies between rustc compilations, combined with the new output field, so we can reconstruct file-based input-output 'white boxes' in the build plan.
However, we might just discard this information completely since crate disambiguator (included for the local crate and deps) information is sufficient to figure out the dependency relation between crates and by adding the output field we also get the implicit file-based dependencies.
I'll remove the struct in the next commit.
Xanewok
force-pushed
the
compilation-options
branch
from
0b9dbe5 to
4c44768
Compare
Xanewok
force-pushed
the
compilation-options
branch
from
4c44768 to
5d2c63f
Compare
|
Changed to include only pub struct CompilationOptions {
pub directory: PathBuf,
pub program: String,
pub arguments: Vec<String>,
pub output: PathBuf,
}Also split |
|
Great, thanks for the changes! |
|
Published as 0.18.1 |
Emit used rustc invocation in the save-analysis file Blocked on rust-dev-tools/rls-data#19. (I'm guessing it won't pass CI due to an out-of-tree git dependency) This should allow RLS to recreate a Rust compilation build plan from the save-analysis files alone, which should be useful when fetching those from external build systems, most notably Buck now. Also this includes some more potentially useful compilation-specific options (e.g. sysroot or the actual path to extern crates) but that's not required for the build plan bits. cc @jsgf @alexcrichton r? @nrc
This is a proposal for what we might include about a given crate compilation. The most important one is the invocation, which can be used to possibly recreate a build plan of sorts from a set of save analysis JSON files. However, there are also other options included that might be handy, such as info whether this is a unit test harness mode, used sysroot, target triple and also where are the actual external crate files.
Additionally, I think it might also be useful to include
cfgoptions (which I did at first, but decided that it's not as crucial now and can possibly be added later), which might be useful for the RLS and other consumers.I'll also push a PR against Rust repo shortly, referencing this branch.
@nrc is this okay for now?