-
Notifications
You must be signed in to change notification settings - Fork 12.7k
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
Rework Rustbuild to an eagerly compiling approach #43059
Conversation
I see large amounts of code being moved around in the first commit. Is this
the only effect (and motivation) for the first commit? Are the changes purely mechanical? |
I hope that's the only effect; unfortunately, I don't see of any easy way to split the work up into multiple commits at this point. Or, at least, multiple commits that work. I can spend some time to add a commit that just moves all code without changing it at all to use Step, I think, though it itself won't build. Edit: To be clear, I will spend some time soon splitting up the code into a move-everything commit first. |
ba0ade8
to
6a9ef25
Compare
Okay, I've pushed up a new set of commits. This implementation strives to be the minimal possible to make things work, i.e., things aren't cleaned up pretty much at all. 18ed842 is the primary commit. The new commit chain currently doesn't attempt to keep the code buildable up until the last commit -- this is so that the diff is better. My current plan is to wait for us to review this, merge it, and then in future PRs clean the code up. I think that's the most efficient way to do this. There are a few FIXMEs in the code that I'm not entirely sure how to fix, and they'll need to be handled before we merge -- I'll be investigating options later on. |
☔ The latest upstream changes (presumably #40939) made this pull request unmergeable. Please resolve the merge conflicts. |
6a9ef25
to
7bac3ef
Compare
Rebased. |
7bac3ef
to
5148363
Compare
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.
Looks like a great direction to go in to me! I haven't exhaustively reviewed yet but opted to look at some of the new abstractions followed by a skim of check.rs
. The overall framework looks pretty good to me, albeit not quite as ergonomic as I'd hoped :(. I think the handling of "default rules" is a little unfortunate but certainly not a dealbreaker.
I wonder if it'd be worth getting some more 'flavorful' suites running before doing a more in-depth review? WDYT?
src/bootstrap/Cargo.toml
Outdated
@@ -38,3 +38,6 @@ getopts = "0.2" | |||
rustc-serialize = "0.3" |
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.
If we take a dependency on serde I think we should be able to remove rustc-serialize and update toml, right?
src/bootstrap/lib.rs
Outdated
@@ -65,9 +65,15 @@ | |||
//! also check out the `src/bootstrap/README.md` file for more information. | |||
|
|||
#![deny(warnings)] | |||
#![feature(associated_consts)] | |||
#![feature(core_intrinsics)] |
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.
I'm not personally a huge fan of using unstable code in the bootstrap process, is there a way we could avoid this? Are these features required?
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.
So associated constants should be stabilized soon I believe -- there is a PR I think already?
The core intrinsics here is just used for sanity checking in the cache -- once we get non 'static
TypeId it should also be able to go away if we can serialize that, which should be possible.
I think it's fine to keep these for now, since the first is soon to be stabilized and the latter needs to be implemented.
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.
I'm sort of ok with the associated consts, but I'd personally really prefer to remove core_intrinsics
. I don't think a non-'static
TypeId
will exist any time soon due to soundness issues.
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.
non-'static
type id is tracked here: #41875.
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.
Note that it's just a change to the intrinsic, not TypeId
itself. The TypeId
itself will still require 'static
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.
Well, I'm probably in favor of not landing that change. It seems that even though the intrinsic is unsafe, from what I gather (not really well-versed in this subject) it's not possible to use the relaxed bound in any safe way anyway so it doesn't seem all that useful, I guess.
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.
The intrinsic only gives you an integer. It can be safe but it's also useless, unless combined with unsafe
code to build an Any
-like abstraction.
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.
Ah I see, yes I believe this was @Mark-Simulacrum's original suggested approach. On reflection, this is probably doable with no more boilerplate than wrapping the struct definition in a macro.
(also, for some reason I previously thought you couldn't have per-function static variables which didn't help my thinking)
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.
You can put any module-level item (including whole modules!) inside any block of statements, but they are not parametrized by the surrounding function's generics or anything like that, it's just for name scoping. This is sometimes used inside the initializer of a static to declare more statics it refers to, or entire types & their trait impls, especially from macro-generated code (e.g. thread_local!
).
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.
Ah, that was it! I wanted statics per instance of a generic function which doesn't work (as you note), but generating each function individually by a macro is fine. That explains my confusion, thanks.
src/bootstrap/builder.rs
Outdated
|
||
fn run(self, builder: &'a Builder) -> Self::Output; | ||
|
||
fn should_run(_builder: &'a Builder, _path: &Path) -> bool { false } |
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.
Could you add some doctests for this trait and the methods here? I'm just starting but I don't quite know what should_run
and make_run
to yet
src/bootstrap/builder.rs
Outdated
} | ||
} | ||
|
||
pub fn ensure<S: Step<'a> + Serialize>(&'a self, step: S) -> S::Output |
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.
Should Serialize
be a supertrait of Step
?
src/bootstrap/check.rs
Outdated
const ONLY_HOSTS: bool = true; | ||
|
||
fn should_run(_builder: &Builder, path: &Path) -> bool { | ||
path.ends_with("cargo") // FIXME: Why is this not src/tools/cargo? |
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.
Oh just historical, this should change to src/tools/cargo now
src/bootstrap/check.rs
Outdated
|
||
static COMPILETESTS: &[(bool, &str, &str, &str)] = &[ | ||
// default, path, mode, suite | ||
(true, "src/test/codegen", "codegen", "codegen"), |
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.
Could these switch to structs with field names?
src/bootstrap/check.rs
Outdated
builder.ensure(native::TestHelpers { target }); | ||
|
||
if mode == "debuginfo-gdb" { | ||
builder.ensure(RemoteCopyLibs { compiler, target }); |
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.
Shouldn't this happen for all test suites?
(e.g. run-pass needs libs in an emulator as well)
I plan to test this out and fix related bugs on various platforms this weekend, though I don't have access to windows to test that platform. I've pushed up some more documentation and fixed the comments I believe.
Could you clear up the ergonomics question here a little? Maybe we can change some things to make it better? |
src/bootstrap/lib.rs
Outdated
//! provide those libraries for it; they are mostly equivalent to constructing | ||
//! the stage1/bin compiler so we don't go through them individually. | ||
//! | ||
//! ## Uplifiting stage1 {std,test,rustc} |
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.
typo here: Uplifiting
☔ The latest upstream changes (presumably #43117) made this pull request unmergeable. Please resolve the merge conflicts. |
Hm, looks like this will probably have to wait a bit -- I hope to get back on it next week though. |
Oh I don't think there's any blockers or anything on the ergonomic front, it's not like it's great today defining things in two places. I didn't have any actionable concerns in mind. |
a674f24
to
8ec46cb
Compare
I've made a "everything becomes owned" PR at Mark-Simulacrum#1. It's fairly large unfortunately and adds a lot of |
4684859
to
981235c
Compare
@alexcrichton I'm changing this to waiting-on-review, since I think it's ready for that. There was at least one successful Travis build (8fd7703). As before, please be brutal with review. |
☔ The latest upstream changes (presumably #43246) made this pull request unmergeable. Please resolve the merge conflicts. |
448be7d
to
8972d5f
Compare
Rebased. |
|
||
use builder::Step; | ||
|
||
pub struct Interned<T>(usize, PhantomData<*const T>); |
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.
Why not a T
rather than a *const T
?
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.
We don't own the T here, and I think this is how to indicate that -- the T is owned by the TyInterner
. Correct me if I'm wrong, though.
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's a phantomdata so I don't know that matters? Unless it's a convention thing.
Much more in favour of this method of caching (for other readers: things are 'interned' by just leaking them, eliminating all lifetime complexity). I'll try and take a look through the rest of it later this week. Just so I'm clear, things that have been lost include:
|
⌛ Testing commit 8f2e576 with merge 3e8ee248f77852446e05672da619bd1803b4227c... |
@infinity0 ah yeah to clarify no funcitonal change is intended here (other than making things "better"), it's just a big change and probably a good idea to keep our eyes our for regressions! |
💔 Test failed - status-travis |
@bors r=alexcrichton |
📌 Commit 1c11823 has been approved by |
Rework Rustbuild to an eagerly compiling approach This introduces a new dependency on `serde`; I don't believe that's a problem since bootstrap is compiled with nightly/beta always so proc macros are available. Compile times are slightly longer -- about 2-3x (30 seconds vs. 10 seconds). I don't think this is too big a problem, especially since recompiling bootstrap is somewhat rare. I think we can remove the dependency on Serde if necessary, though, so let me know. r? @alexcrichton
OK, thanks for the explanations! |
☀️ Test successful - status-appveyor, status-travis |
This PR seems to have removed the |
At least, |
let failures = self.build.delayed_failures.get(); | ||
if failures > 0 { | ||
println!("\n{} command(s) did not execute successfully.\n", failures); | ||
process::exit(1); |
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 is the code that @infinity0 is talking about. The delayed_failures
counter still exists, and is incremented, but AFAICS nothing checks it anymore.
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.
Looks like this just needs to go at the end of Build::build
. I'll try that and send a PR.
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.
Yeah, it's quite likely that we're just not checking it anymore. r? me on the PR please!
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.
@infinity0 has a change to also summarize the failed commands, so I think they'll PR it.
Is it really time? Have our months, no, *years* of suffering come to an end? Are we finally able to cast off the pall of Hoedown? The weight which has dragged us down for so long? ----- So, timeline for those who need to catch up: * Way back in December 2016, [we decided we wanted to switch out the markdown renderer](rust-lang#38400). However, this was put on hold because the build system at the time made it difficult to pull in dependencies from crates.io. * A few months later, in March 2017, [the first PR was done, to switch out the renderers entirely](rust-lang#40338). The PR itself was fraught with CI and build system issues, but eventually landed. * However, not all was well in the Rustdoc world. During the PR and shortly after, we noticed [some differences in the way the two parsers handled some things](rust-lang#40912), and some of these differences were major enough to break the docs for some crates. * A couple weeks afterward, [Hoedown was put back in](rust-lang#41290), at this point just to catch tests that Pulldown was "spuriously" running. This would at least provide some warning about spurious tests, rather than just breaking spontaneously. * However, the problems had created enough noise by this point that just a few days after that, [Hoedown was switched back to the default](rust-lang#41431) while we came up with a solution for properly warning about the differences. * That solution came a few weeks later, [as a series of warnings when the HTML emitted by the two parsers was semantically different](rust-lang#41991). But that came at a cost, as now rustdoc needed proc-macro support (the new crate needed some custom derives farther down its dependency tree), and the build system was not equipped to handle it at the time. It was worked on for three months as the issue stumped more and more people. * In that time, [bootstrap was completely reworked](rust-lang#43059) to change how it ordered compilation, and [the method by which it built rustdoc would change](rust-lang#43482), as well. This allowed it to only be built after stage1, when proc-macros would be available, allowing the "rendering differences" PR to finally land. * The warnings were not perfect, and revealed a few [spurious](rust-lang#44368) [differences](rust-lang#45421) between how we handled the renderers. * Once these were handled, [we flipped the switch to turn on the "rendering difference" warnings all the time](rust-lang#45324), in October 2017. This began the "warning cycle" for this change, and landed in stable in 1.23, on 2018-01-04. * Once those warnings hit stable, and after a couple weeks of seeing whether we would get any more reports than what we got from sitting on nightly/beta, [we switched the renderers](rust-lang#47398), making Pulldown the default but still offering the option to use Hoedown. And that brings us to the present. We haven't received more new issues from this in the meantime, and the "switch by default" is now on beta. Our reasoning is that, at this point, anyone who would have been affected by this has run into it already.
This has been present since `builder.ensure` was first added in rust-lang#43059. It's unclear to me why it was added then - I tested these changes locally with `x test compiler/rustc_data_structures --stage 0` and they worked fine. Fixes rust-lang#51748.
Don't build the full compiler before running unit tests This has been present since `builder.ensure` was first added in rust-lang#43059. It's unclear to me why it was added then - I tested these changes locally with `x test compiler/rustc_data_structures --stage 0` and they worked fine. Fixes rust-lang#51748.
This introduces a new dependency on
serde
; I don't believe that's a problem since bootstrap is compiled with nightly/beta always so proc macros are available. Compile times are slightly longer -- about 2-3x (30 seconds vs. 10 seconds). I don't think this is too big a problem, especially since recompiling bootstrap is somewhat rare. I think we can remove the dependency on Serde if necessary, though, so let me know.r? @alexcrichton