Skip to content
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

[stable] Backports for 1.12.1 #37173

Merged
merged 10 commits into from
Oct 19, 2016
Merged

[stable] Backports for 1.12.1 #37173

merged 10 commits into from
Oct 19, 2016

Conversation

brson
Copy link
Contributor

@brson brson commented Oct 14, 2016

This bumps the stable version to 1.12.1 and backports fixes for the
following regressions, all of which have additionally been backported
to beta:

It does not include fixes for these regressions, either because they
are unfixed or the fixes aren't merged into master:

If you can, please see if there's anything you do to advance the above unfixed
regressions.

Two things to be aware of. The fix for #36875 is due to unknown changes in the
docker container used by the builders. So there's some risk that there are other
changes in that container that could introduce further regressions. The fix for
#36924 and #36925 also picks up the LLVM JS target definitions, and while they

will mostly be inert on the stable branch, it does contain two minor
changes
to common transformations, in ConstantMerge.cpp and
InstCombineCompares.cpp.

I'm not sure yet whether we should wait for further fixes before issuing a point
release. I'm inclined not to wait further, except to pick up #37109 since it
isn't blocked on anything.

I'll run this branch through the test suite on various platforms and against
crater before doing a build.

I think the procedure here is that at least a few members of the compiler team
need to sign off on these, then we can merge into stable. At that point we're
still not committed to anything yet. Then I'll do a build, and probably ask
people to manually verify some of the fixes. Then we'll issue the release like
any other.

cc @rust-lang/compiler

nikomatsakis and others added 3 commits October 14, 2016 11:49
The collector was asserting a total absence of projections, but some
projections are expected, even in trans: in particular, projections
containing higher-ranked regions, which we don't currently normalize.
@rust-highfive
Copy link
Collaborator

r? @Aatch

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

@brson
Copy link
Contributor Author

brson commented Oct 14, 2016

r? @alexcrichton

@rust-highfive rust-highfive assigned alexcrichton and unassigned Aatch Oct 14, 2016
@michaelwoerister
Copy link
Member

Won't this cause stable to use the same LLVM version that we use for nightly? Is that what we want?

@brson
Copy link
Contributor Author

brson commented Oct 14, 2016

@michaelwoerister Yes it will. LLVM on stable/beta/nightly are all only a few commits apart right now, so it doesn't seem all that risky to me.

@michaelwoerister
Copy link
Member

@brson OK, just wanted to make sure this wasn't an unintended side effect.

@cuviper
Copy link
Member

cuviper commented Oct 14, 2016

Hmm, is this the first time we've had a stable bump?

How will stage0 bootstrap work -- can this use 1.11.0, 1.12.0, and 1.12.1? That probably needs some key-munging to let 1.12.0 through...

@cuviper
Copy link
Member

cuviper commented Oct 14, 2016

I would like to have #37153 if possible. The backport is simple, as I commented there.

@steveklabnik
Copy link
Member

Hmm, is this the first time we've had a stable bump?

yes.

@alexcrichton
Copy link
Member

Just wanted to explicitly highlight the LLVM diff as well. (pretty small)

@cuviper

How will stage0 bootstrap work -- can this use 1.11.0, 1.12.0, and 1.12.1? That probably needs some key-munging to let 1.12.0 through...

That's a good question! I would personally assume that 1.13.0 will bootstrap from 1.12.0 (as the beta branch does today). That is, I don't think we'll update that to bootstrap from 1.12.1.

@eddyb
Copy link
Member

eddyb commented Oct 14, 2016

@alexcrichton But doesn't that cause problems for distros which need to choose one version?
Should we guarantee that all stable point releases are compatible at least in their bootstrap abilities?
Compiled crates are not compatible because metadata contains the version but that's less relevant.

@cuviper
Copy link
Member

cuviper commented Oct 14, 2016

@alexcrichton Well now you've revealed that there are two parts to this question!

  1. What will 1.12.1 use for stage0? Part of the reason for using "prior release" was so distros can easily step from release to release. If I've already shipped 1.12.0, can I use that for 1.12.1?
  2. What will 1.13.0 use for stage0? Similarly, if I've already shipped 1.12.1, I would like to use that next.

For the purposes of bootstrapping, these point-releases shouldn't make any difference to the build system. 1.12.0 building 1.12.1 is basically a "local-rebuild" case, and any 1.12.x is a "prior-release" case for 1.130. But their bootstrap keys are different (#36548), and the current local-rebuild autodetection probably won't work with this nuance.

@alexcrichton
Copy link
Member

@cuviper

1.12.1 must bootstrap from 1.11.0 (like 1.12.0), unfortunately. If we updated 1.12.1 to bootstrap from 1.12.0 it would be a much more invasive change (moving a whole stage forward).

In the limit the purpose of the bootstraping-from-previous is to make distro's lives easier. So in that sense we can likely accommodate whatever you need. The easiest thing for us would be to say that 1.x.0 bootstraps 1.x+1.* unconditionally, but would that cause problems?

@cuviper
Copy link
Member

cuviper commented Oct 14, 2016

Right now, 1.12.0 can bootstrap from either 1.11.0 or 1.12.0 (local-rebuild). I think it should be possible for 1.12.1 to bootstrap from 1.11.0 as normal, or use either 1.12.0 or 1.12.1 as a local-rebuild. But I'm sure the build system isn't set up for this, if only due to their different bootstrap keys.

The easiest thing for us would be to say that 1.x.0 bootstraps 1.x+1.* unconditionally, but would that cause problems?

That definitely causes problems, because the only distro rustc I have access to is the latest build we've shipped. If I keep up with everything, 1.11.0 -> 1.12.0 -> 1.12.1 -> 1.13.0 all need to be viable, with the additional possibility of self-rebuilds at any step.

It's not insurmountable. If need be, I can patch the bootstrap keys to match whatever I have, and it should be functionally the same. But I think we need to make the build system handle this long term.

@cuviper
Copy link
Member

cuviper commented Oct 14, 2016

I'm fine with 1.x.0 being the "official" bootstrap stage0 for 1.(x+1).*. And I hope we continue that 1.x.y can also bootstrap itself, --enable-local-rebuild. It's just interpolating between that we need to figure out.

@brson
Copy link
Contributor Author

brson commented Oct 15, 2016

Once somebody implements #36548 distros can bootstrap from whatever toolchain happens to work. I don't think we can make any changes to the bootstrapping logic in a stable point release though.

@brson
Copy link
Contributor Author

brson commented Oct 15, 2016

Hm, it's probably not difficult to have 1.12.1 bootstrap off of 1.12.0.

I guess this also indicates that 1.13 will need to bootstrap from 1.12.1...

@brson
Copy link
Contributor Author

brson commented Oct 17, 2016

Right now I plan to not change the bootstrap compiler for 1.12.1, but to publish the simple patch that people can use to change it themselves. 1.13 will bootstrap off 1.12.1.

@brson
Copy link
Contributor Author

brson commented Oct 18, 2016

I've added backports of these:

The latter had a merge conflict that I will need to get reviewed. There's also a build error on this branch that I am working on now. And rust-buildbot doesn't build beta/stable correctly right now. Also poking at that.

@brson
Copy link
Contributor Author

brson commented Oct 18, 2016

@arielb1 there was a merge conflict in meth.rs backporting #37122. I'm about to build and test but does 5e6e322#diff-08fcf9f94ac767ddd96da96a1dc23096 look right to you?

@brson brson mentioned this pull request Oct 18, 2016
@brson
Copy link
Contributor Author

brson commented Oct 18, 2016

Yeah, the backport of #37112 was not clean. I'm removing it for now.

@brson
Copy link
Contributor Author

brson commented Oct 18, 2016

I'm still working to get this branch to build cleanly. I need help with the following:

Once I have this branch building I will run it through a variety of tests.

@brson
Copy link
Contributor Author

brson commented Oct 18, 2016

OK, I pushed a fix to make the "Temporary fix for metadata decoding" patch build. Now testing.

@cuviper
Copy link
Member

cuviper commented Oct 18, 2016

@brson did you or the team consider #37153? It's a small patch, with only a small tweak needed for backporting, as I mentioned there. I will probably add that fix to our Fedora builds regardless, otherwise we can't build Cargo opt+debug with MIR.

@brson
Copy link
Contributor Author

brson commented Oct 18, 2016

@cuviper Thanks for reminding me. I'll go look at it.

@brson
Copy link
Contributor Author

brson commented Oct 18, 2016

@cuviper Added.

@brson
Copy link
Contributor Author

brson commented Oct 18, 2016

I'm currently testing this branch against the auto-builders in the dev environment, and against crater.

@nikomatsakis
Copy link
Contributor

@brson r=me for these existing patches; they all look straightforward.

@brson
Copy link
Contributor Author

brson commented Oct 18, 2016

Oh I mispoke about the one that doesn't cherry-pick cleanly, it's:

Note that the first commit there is a revert that doesn't need to be applied. Only the second commit needs to be applied.

@nikomatsakis
Copy link
Contributor

@brson so I backported the branch, but I encounter assertion failures. They seem to be somewhat legit; I am trying to figure out what is happening here. The problem is that there are references to early-bound regions in the fn type passed to declare_fn, and hence the assertion that needs_subst is false fails. Not sure if anyone remembers this bug, I could dig through the git blame on master to try to figure out what changed here...

@brson
Copy link
Contributor Author

brson commented Oct 18, 2016

@nikomatsakis That's pretty scary. Maybe an indication the backport is too risky.

@brson
Copy link
Contributor Author

brson commented Oct 18, 2016

I've fixed a minor test failure bug and am going to start running the current branch through regression testing again.

@nikomatsakis
Copy link
Contributor

@brson yeah, I don't think it has to do w/ a botched merge; I think it's just that the ground shifted a bit in the meantime. Probably a separate bug fix is my guess (since it seems bogus for this assert to be failing to me...), but I'm not sure which.

@brson
Copy link
Contributor Author

brson commented Oct 19, 2016

Here's the crater report: https://gist.github.com/anonymous/288f30714e4bf083519facec7808fbad

All the regressions are false-positives except one: nuclear-backend-gfx. The error is in crate resolution. Suspicious. I'll try to reproduce locally.

The exceptionally high number of "fixed" crates in the report is due to my custom build of this branch having nightly features turned on (I wonder if that is related to the nuklear regression).

The tests in dev failed for infrastructural reasons, and I'm going to get them started again.

I'm going to go ahead and merge this and start the stable build, assuming this revision is going to be acceptable and the nuklear regression has some reasonable explanation.

@brson brson merged commit d4f3940 into rust-lang:stable Oct 19, 2016
@brson
Copy link
Contributor Author

brson commented Oct 19, 2016

The nuklear resolution failure could be explained as a nightly cargo bug. That test run does not use the same build of cargo as stable.

@brson
Copy link
Contributor Author

brson commented Oct 19, 2016

The nuklear failure does seem to be a cargo nightly regression and shouldn't impact 1.12.1

@cuviper
Copy link
Member

cuviper commented Nov 1, 2016

1.13 will bootstrap off 1.12.1.

@brson - Is this still the plan? I don't see this reflected in the beta branch src/stage0.txt.

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.