Expose Target and Unit params to appropriate Executor callbacks#4416
Merged
bors merged 1 commit intorust-lang:masterfrom Aug 20, 2017
Merged
Expose Target and Unit params to appropriate Executor callbacks#4416bors merged 1 commit intorust-lang:masterfrom
Target and Unit params to appropriate Executor callbacks#4416bors merged 1 commit intorust-lang:masterfrom
Conversation
Xanewok
added a commit
to Xanewok/rls
that referenced
this pull request
Aug 20, 2017
Until rust-lang/cargo#4416 is merged, this depends on a `more-executor-params` of the Xanewok/cargo fork. That PR provides additional, necessary information passed via `Executor` callbacks used to query `Context` and create the target dependency graph.
Xanewok
added a commit
to Xanewok/rls
that referenced
this pull request
Aug 20, 2017
Until rust-lang/cargo#4416 is merged, this depends on a `more-executor-params` of the Xanewok/cargo fork. That PR provides additional, necessary information passed via `Executor` callbacks used to query `Context` and create the target dependency graph.
Member
|
@bors: r+ Looks great to me, thanks! |
Contributor
|
📌 Commit 2fd78db has been approved by |
Contributor
bors
added a commit
that referenced
this pull request
Aug 20, 2017
Expose `Target` and `Unit` params to appropriate `Executor` callbacks This effectively does only two things: * Pass existing `&Unit` to `init()` * Pass existing `&Target` to `exec()`s Also updated doc comments slightly. The `init()` is called for every `Work` preparation by `rustc()` and since it's not called once (not sure if intended?), it'd be good to include more information for which `Unit` the `rustc` process invocation has been initially prepared (I think more importantly it shows that `init()` is called many times for many targets). Additionally, it'd be good to know about `Unit` compiled in the `exec()` callbacks in more structured manner (so the user doesn't have to parse the `ProcessBuilder` arguments and/or make certain assumptions) and since `target` was already cloned and moved into the closure, it was only a matter of exposing it. I think it'd be ideal to provide all the necessary information to recreate a `Unit` but with a static lifetime needed for the closure, however this would mean cloning more data per-target ([`Kind`](https://github.com/rust-lang/cargo/blob/master/src/cargo/ops/cargo_rustc/mod.rs#L41) should be basically free, however not sure if cloning [`Profile`](https://github.com/rust-lang/cargo/blob/master/src/cargo/core/manifest.rs#L151) is acceptable). With this, `Executor` users could query `Context` more easily (e.g. to get a unit dep graph! 😄), as the API accepts `Unit`s in many places. r? @alexcrichton cc @nrc EDIT: This changes the public API, not sure if there's something else I should do about it, only ran `cargo test` locally.
Contributor
|
☀️ Test successful - status-appveyor, status-travis |
Contributor
Author
|
Thank you! 🎉 |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
This effectively does only two things:
&Unittoinit()&Targettoexec()sAlso updated doc comments slightly.
The
init()is called for everyWorkpreparation byrustc()and since it's not called once (not sure if intended?), it'd be good to include more information for whichUnittherustcprocess invocation has been initially prepared (I think more importantly it shows thatinit()is called many times for many targets).Additionally, it'd be good to know about
Unitcompiled in theexec()callbacks in more structured manner (so the user doesn't have to parse theProcessBuilderarguments and/or make certain assumptions) and sincetargetwas already cloned and moved into the closure, it was only a matter of exposing it.I think it'd be ideal to provide all the necessary information to recreate a
Unitbut with a static lifetime needed for the closure, however this would mean cloning more data per-target (Kindshould be basically free, however not sure if cloningProfileis acceptable). With this,Executorusers could queryContextmore easily (e.g. to get a unit dep graph! 😄), as the API acceptsUnits in many places.r? @alexcrichton
cc @nrc
EDIT:
This changes the public API, not sure if there's something else I should do about it, only ran
cargo testlocally.