-
Notifications
You must be signed in to change notification settings - Fork 2.3k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Build a CommandPrototype instead of a ProcessBuilder #1107
Conversation
host: Layout, target: Option<Layout>, | ||
root_pkg: &Package, | ||
build_config: BuildConfig) | ||
-> CargoResult<Context<'a, 'b>> { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This already has an unfortunately large number of arguments, perhaps the execution could be set after a Context
is created?
baa7686
to
bcacded
Compare
use util::{mod, CargoResult, ProcessError, ProcessBuilder}; | ||
|
||
/// Trait for objects that can execute commands. | ||
pub trait ExecEngine { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It may be worth having this trait inherit from Send + Sync
to cut down on writing Send + Sync
all over the place.
021032d
to
44b478d
Compare
Everything should be done except that problem with compilation options. The lazy part inside me suggests that we just file an issue for this, and fix it when the issues in rustc are fixed. The ExecEngine is just an obscure feature that nobody will use in at least the next few weeks anyway. The alternative is to add a template parameter to CompilationOptions, but that would need to be propagated everywhere. |
44b478d
to
252920b
Compare
252920b
to
3656bec
Compare
That all sounds good to me, thanks again for this! I'm excited to see what comes next :) |
With this change, the various commands in Cargo now build a `CommandPrototype` instead of a `ProcessBuilder`. This `CommandPrototype` is then passed to an object that implements the `CommandsExecution` trait, which executes them. Ideally `CommandPrototype` would look like this: ```rust enum CommandPrototype { Rustc { libraries: Vec<String>, opt_level: uint, input: Path, ... etc... (all the options that can be passed to rustc) }, Rustdoc { ... (all the options that can be passed to rustdoc) }, Custom { cmd: ..., env: ..., } } ``` ...but that's a bit too much work for now (maybe in a follow-up). What this enables is using the Cargo library to intercept the build commands and tweak them. I'm planning to write `cargo-emscripten` thanks to this change. With this, one could also imagine embedding rustc and rustdoc directly inside cargo, if that is needed.
☀️ Test successful - cargo-linux-32, cargo-linux-64, cargo-mac-32, cargo-mac-64, cargo-win-32, cargo-win-64 |
With this change, the various commands in Cargo now build a
CommandPrototype
instead of aProcessBuilder
. ThisCommandPrototype
is then passed to an object that implements theCommandsExecution
trait, which executes them.Ideally
CommandPrototype
would look like this:...but that's a bit too much work for now (maybe in a follow-up).
What this enables is using the Cargo library to intercept the build commands and tweak them. I'm planning to write
cargo-emscripten
thanks to this change.With this, one could also imagine embedding rustc and rustdoc directly inside cargo, if that is needed.