Skip to content

Fix make check-stage1 #27417

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

Closed
wants to merge 1 commit into from
Closed

Conversation

eefriedman
Copy link
Contributor

stage1 rustc doesn't support catching panics, so we have to just kill
the whole process on a fatal error. This is a little hacky... but it's not
very much code, and we only use it for stage1 compilers.

Also, use stage2 compiletest to run tests.

stage1 rustc doesn't support catching panics, so we have to just kill
the whole process on a fatal error.  This is a little hacky... but it's not
very much code, and we only use it for stage1 compilers.

Also, use stage2 compiletest to run tests.
@rust-highfive
Copy link
Contributor

r? @pcwalton

(rust_highfive has picked a reviewer for you, use r? to override)

@Gankra
Copy link
Contributor

Gankra commented Jul 31, 2015

:O

Fantastic!

💯

@alexcrichton
Copy link
Member

Out of curiosity, could you measure the different in compile time for just not passing -Z no-landing-pads at stage0? The benefit may not outweigh the cost any more.

@jonas-schievink
Copy link
Contributor

I hope I did this right, but this is my result (on 64-bit Win10):

With no-landing-pads:

$ ../configure && time make -j4 rustc-stage1

real    11m20.300s
user    0m13.254s
sys     0m23.901s

With landing pads (enabled by commenting out this line):

$ ../configure && time make -j4 rustc-stage1

real    13m10.085s
user    0m13.870s
sys     0m29.292s

@bluss
Copy link
Member

bluss commented Aug 3, 2015

Does this allow running the testsuite at stage0 or stage1? The testsuite is essential for developing & contributing to libstd etc.

My patience with rust's long develop-compile-test cycles is already stretched pretty thin, if we go ahead to speed up full bootstrap at the cost of using tests at all, then something is missing. Something like ./configure --i-actually-work-here so that you can use the stage0 and/or stage1 testsuite for development.

@jonas-schievink
Copy link
Contributor

make check-stage1 works, check-stage0 doesn't because make can't build compiletest (I believe this PR contains a fix for that)

@alexcrichton
Copy link
Member

I think I'd be in favor of just removing -Z no-landing-pads from the stage0 build. It was always a bit of hack and it's something that should be supported in a more official capacity anyway.

Thoughts @brson?

@alexcrichton alexcrichton assigned alexcrichton and unassigned pcwalton Aug 3, 2015
@tamird
Copy link
Contributor

tamird commented Aug 3, 2015

OTOH, 2 minutes is not nothing.

@bluss
Copy link
Member

bluss commented Aug 3, 2015

2 minutes are great savings, but maybe we can have a debug or developer switch that toggles it so that we can work productively on libs, for example using the stage0 tests?

@alexcrichton
Copy link
Member

Sure, I agree that 2 minutes is nice to save, but there are downsides:

  • We already have a bajillion options in our configure script, and I'd rather not add yet-another-obscure-option.
  • Seems like there's quite a desire to get tests working at stage0/1, and the only way to robustly do so is to enable landing pads right now.

@jonas-schievink
Copy link
Contributor

This also wasn't done on the fastest machine. What are the specs of the build bots? (alternatively someone could test/extrapolate the percentage bootstrap time saved by this to get a better metric, I didn't do that because my total time actually decreased which I suspect was due to noise)

@Gankra
Copy link
Contributor

Gankra commented Aug 3, 2015

@jonas-schievink Note that buildbot is massively bottlenecked on Windows running an entire extra stage anyway. Most of the bots idle long before windows finishes up.

disregard I need sleep

@eefriedman
Copy link
Contributor Author

Ping. I've seen three possibilities proposed: this PR, a patch to remove the -Z no-landing-pads flag, and doing nothing because nobody cares enough.

@bluss
Copy link
Member

bluss commented Aug 19, 2015

I'd prefer a configure option if we don't outright remove this bootstrap optimization. I don't think this is just for me, those actually working on rust need unwinding while testing their contributions and the test suite.

I saw @nikomatsakis wanted to know how to turn this off too.

@nikomatsakis
Copy link
Contributor

Yes, I second the request for a configure flag. Perhaps tied to enable debug (which in my ideal world would not disable optimizations, but I guess that is orthogonal)

-------- Original message --------
From: bluss [email protected]
Date:08/19/2015 08:10 (GMT-05:00)
To: rust-lang/rust [email protected]
Cc: Niko Matsakis [email protected]
Subject: Re: [rust] Fix make check-stage1 (#27417)
I'd prefer a configure option if we don't outright remove this bootstrap optimization. I don't think this is just for me, those actually working on rust need unwinding while testing their contributions and the test suite.

I saw @nikomatsakis wanted to know how to turn this off too.


Reply to this email directly or view it on GitHub.

@alexcrichton
Copy link
Member

For now let's take the route of just tying -Z no-landing-pads in stage0 to --enable-debug, I'm still worried about this patch as-is with respect to the output on panics because we do somewhat rely on output being captured and then emitted on the other side of the panic (e.g. what's read in the monitor function).

Just tying to --enable-debug would be nice and simple and keep things generally working as they currently are.

@arielb1
Copy link
Contributor

arielb1 commented Aug 22, 2015

@alexcrichton

Well, it USED to WORK with -Z no-landing-pads before you BROKE it. I guess this could be fixed when we get a panic_handler API.

@alexcrichton
Copy link
Member

Closing due to inactivity, and given the discussion on the discuss thread I've opened #28710 to track this.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

10 participants