Let CompileOptions create a BuildConfig directly #5358
Conversation
|
r? @matklad (rust_highfive has picked a reviewer for you, use r? to override) |
djc
force-pushed
the
options-build-config
branch
from
6406794
to
30bfa20
Compare
|
☔ The latest upstream changes (presumably #5359) made this pull request unmergeable. Please resolve the merge conflicts. |
| /// Whether we are running tests | ||
| pub mode: CompileMode, | ||
| /// Whether we are building documentation | ||
| pub doc_all: bool, |
There was a problem hiding this comment.
I wonder how many bugs do we have because we have doc_all here, CompileMode::Doc and profile.doc, which seem to all control the same thing? This is completely unrelated to the PR of course, it's just me rambling around =)
| /// The target platform to compile for (example: `i686-unknown-linux-gnu`). | ||
| pub target: Option<String>, | ||
| /// Configuration information for a rustc build | ||
| pub build_config: BuildConfig, |
There was a problem hiding this comment.
So, at #5351 (comment) @alexcrichton suggested that we don't do too much stuff when parsing command line arguments. I am not sure I entirely agree, to me it seems beneficial to lower stringly-typed command-line flags into specific data structures as early as possible, even if that requires looking at a broader context then just literally flags. However in this case creating a BuildConfig requires running rustc, if only to get host, which seems a bit too heavy weight for command-line parsing.
Perhaps we could use an API like CompileOptions::into_build_config(self) -> BuildConfig, and call that when constructing a context?
There was a problem hiding this comment.
The way I understand @alexcrichton's remarks is that we shouldn't do any input validation beyond upcasting to more specific types, because in many cases we want API users to benefit from the same validation that the CLI needs. So, where we can employ types to do validation, we should (this makes it ~impossible for API users to pass invalid input anyway), but validation of actually string-typed data should be done on the inside of the library API.
I understand and agree with the reluctance to run rustc during the construction of CompileOptions, but I'm not sure I see much value in having a CompileOptions::into_build_config(self) API -- this would still duplicate a bunch of stuff in CompileOptions (which gets deconstructed in cargo::ops::compile()) and the BuildConfig itself.
It still feels to me like the Rustc does not belong in BuildConfig; previously, BuildConfig was a straightforward bag of data about build configuration, but Rustc is not like that -- it's more a source of build configuration instead. I would be more inclined to move the Rustc and things that depend on it (the target configs host and target) into the Context. This has the added advantage that the construction of the TargetInfos gets closer to these other things, so we could eventually merge those calls to rustc. If we do this, BuildConfig can be (cheaply) constructed as part of CompileOptions. What do you think?
There was a problem hiding this comment.
Hm, yeah, moving rustc, host and target out of BuildConfig seems reasonable. On the other after #5384 the build config would look like
/// Configuration information for a rustc build.
pub struct BuildConfig {
pub rustc: Rustc,
/// Build information for the host arch
pub host: TargetConfig,
/// The target arch triple, defaults to host arch
pub requested_target: Option<String>,
/// Build information for the target
pub target: TargetConfig,
/// How many rustc jobs to run in parallel
pub jobs: u32,
/// Whether we are building for release
pub release: bool,
/// Whether to print std output in json format (for machine reading)
pub json_messages: bool,
}
and, if we move host/target out of it, it becomes really thin. Why do we need in the first place? Could we just fold it all into context?
There was a problem hiding this comment.
That would make Context even more of a God object. I've been looking at angles to splitting up Context, maybe in a part that's more stateless (does not depend on the units submitted) and a part that's more stateful. With #5384 pending, I'll probably wait until that settles, but in the meantime I suggest we avoid moving more stuff there unless it improves the design. Worst case, BuildConfig is still a useful grouping of things that live together in CompileOptions.
There was a problem hiding this comment.
@djc agree that it's best to wait for #5384 to settle.
As for the larger design here, I definitely feel that the situation around Context and related structs could and should be improved, but I can't say precisely how that improvement should look like. The idea about spiting Context along stateless/stateful boundary seems promising. It also seems like we perhaps can split context into several layers, because some parts of context can be seen both mutable at one stage and immutable at other stage? For example, initially the context is empty. Then, we fill it with root units and populate it with unit dependencies map. After that, the units are fixed, but we further mutate other parts of the context to get a Compilation.
There was a problem hiding this comment.
@matklad for me it does not make much sense to hold this off until #5384 settles; I merely meant that future exploration of how to split Context should probably hold for now. I still feel there are a number of improvements in this PR that are worth landing, even if they make BuildConfig a bit thin for now.
There was a problem hiding this comment.
I still feel there are a number of improvements in this PR that are worth landing, even if they make BuildConfig a bit thin for now.
Just to make clear, you'd like lift rustc from BuildConfig to Context, so that it is not created during cli parsing, and otherwise leave the rest as it is now? Sounds good to me, I think we can land this before #5384 than.
cc @ehuss
There was a problem hiding this comment.
I don't have any opinion. There will be some merge conflict work, but that's fine with me.
There was a problem hiding this comment.
#5384 came to fruition faster that this PR, so merging that!
|
BTW, I wanted to mention as a point of feedback that I was annoyed by the process around this PR. As far as I can tell, (1) I submitted this PR, (2) @matklad was immediately assigned as the reviewer, meaning I was assuming he was aware of it. Still, when (3) he submitted #5359 a short while later, that PR went on to be merged before this one, leaving me with a substantial amount of rebasing to do (I've spent a bit over an hour). At the least, it would have been nice if there was some acknowledgement of/communication about the merge conflict and who would be responsible for the necessary rebasing/merging work. |
|
Yes, I've definitely should have given at least a heads up when submitting #5359 :( |
|
I've updated this as discussed, please have a look. |
|
☔ The latest upstream changes (presumably #5473) made this pull request unmergeable. Please resolve the merge conflicts. |
This means we can store the cache file name in one place.
|
Changes look good to me! Let's see what @alexcrichton thinks about this as well! |
|
@bors: r=matklad |
|
📌 Commit 072254c has been approved by |
Let CompileOptions create a BuildConfig directly This puts input validation in a more central place and prevents copying/moving stuff around as separate values.
|
☀️ Test successful - status-appveyor, status-travis |
This puts input validation in a more central place and prevents copying/moving stuff around as separate values.