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

1.52.1 - Blog post explaining the fingerprint issue. #836

Merged
merged 20 commits into from
May 10, 2021

Conversation

pnkfelix
Copy link
Member

@pnkfelix pnkfelix commented May 7, 2021

No description provided.

@pnkfelix pnkfelix requested a review from a team as a code owner May 7, 2021 19:33
Mark-Simulacrum
Mark-Simulacrum previously approved these changes May 7, 2021
Copy link
Member

@Mark-Simulacrum Mark-Simulacrum left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I will likely aim to merge this in roughly an hour, but it looks good to me.

@Mark-Simulacrum
Copy link
Member

Moving publish date to Monday morning.

pnkfelix and others added 3 commits May 7, 2021 22:51
Co-authored-by: Tyler Mandry <tmandry@gmail.com>
since this post isn't going out today, it should use more abstract language to refer to when 1.52.0 came out.
Copy link
Member

@carols10cents carols10cents left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Haven't finished reading yet but wanted to submit comments so far

posts/2021-05-07-caught-red-handed.md Outdated Show resolved Hide resolved
posts/2021-05-07-caught-red-handed.md Outdated Show resolved Hide resolved
Copy link
Member

@carols10cents carols10cents left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok, thoughts on the post done. I'm happy to read revisions later today or tomorrow if that would be helpful!

posts/2021-05-07-caught-red-handed.md Outdated Show resolved Hide resolved
posts/2021-05-07-caught-red-handed.md Outdated Show resolved Hide resolved
posts/2021-05-07-caught-red-handed.md Outdated Show resolved Hide resolved
posts/2021-05-07-caught-red-handed.md Outdated Show resolved Hide resolved
posts/2021-05-07-caught-red-handed.md Outdated Show resolved Hide resolved
posts/2021-05-07-caught-red-handed.md Outdated Show resolved Hide resolved
posts/2021-05-07-caught-red-handed.md Outdated Show resolved Hide resolved

We recommend that users of 1.52.0 disable incremental compilation, to avoid running into this problem.

We do *not* recommend that users of 1.52.0 downgrade to an earlier version of Rust in response to this problem. As noted above, there is at least one instance of a silent [miscompilation][issue-82920] caused by incremental compilation that was not caught until we added the fingerprint checking.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

And we don't know how far back these bugs are present.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

(I think the text in the summary does a good job of explaining that aspect of things. I don't think we need to reiterate the point here.)

posts/2021-05-07-caught-red-handed.md Outdated Show resolved Hide resolved
posts/2021-05-07-caught-red-handed.md Outdated Show resolved Hide resolved
Just a first draft, need to re-read and adjust the structure a little still.
@Mark-Simulacrum
Copy link
Member

Just pushed an update in light of my expectation that we'll be releasing a point release on Monday - see rust-lang/rust#85097 - but I expect to continue working on the language tomorrow, thank you everyone for the comments so far.

@Mark-Simulacrum
Copy link
Member

Hey @carols10cents I applied a bunch of feedback from your comments I think, though some of them are also a bit stale after some movement - I would really love a review if you have a chance to do so :)

@carols10cents
Copy link
Member

@Mark-Simulacrum looking now!

Copy link
Member

@carols10cents carols10cents left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is lots, lots better!!!

I think the only bits of my first round of feedback that I don't see incorporated are:

  • At least a short statement that we will be investigating and retrospecting on the root causes of how this change got to stable before we expected it to and how to prevent similar situations in the future
  • At least a short definition of what "miscompilation" means in this case to give a sense of what kind of "wrong" the compiler may have been silently generating, because it sounds pretty scary without further elaboration

Without that information, I would bet money that those two topics will be the ones that get the most comments/questions when this is posted.

posts/2021-05-07-caught-red-handed.md Outdated Show resolved Hide resolved
posts/2021-05-07-caught-red-handed.md Outdated Show resolved Hide resolved
posts/2021-05-07-caught-red-handed.md Outdated Show resolved Hide resolved
posts/2021-05-07-caught-red-handed.md Outdated Show resolved Hide resolved
posts/2021-05-07-caught-red-handed.md Outdated Show resolved Hide resolved
Mark-Simulacrum and others added 3 commits May 9, 2021 19:34
Co-authored-by: Carol (Nichols || Goulding) <193874+carols10cents@users.noreply.github.com>
Co-authored-by: Carol (Nichols || Goulding) <193874+carols10cents@users.noreply.github.com>
@Mark-Simulacrum
Copy link
Member

Hopefully addressed all remaining points. Thanks!

posts/2021-05-07-caught-red-handed.md Outdated Show resolved Hide resolved
posts/2021-05-07-caught-red-handed.md Outdated Show resolved Hide resolved
posts/2021-05-07-caught-red-handed.md Outdated Show resolved Hide resolved
carols10cents
carols10cents previously approved these changes May 10, 2021
Copy link
Member

@carols10cents carols10cents left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks great now (with the few small edits @est31 pointed out that I agree with). Thank you so much for working on this ❤️

steveklabnik
steveklabnik previously approved these changes May 10, 2021
Copy link
Member

@steveklabnik steveklabnik left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for doing such a thorough writeup on this. Looks good to me.

wesleywiser
wesleywiser previously approved these changes May 10, 2021
posts/2021-05-07-caught-red-handed.md Outdated Show resolved Hide resolved
our confidence in the fixes, may issue a 1.52.2 point release which backports
those fixes to the stable channel. Users wishing to help us test can use the
nightly channel, and report bugs to rust-lang/rust with any ICEs they are
seeing. We do not expect at this time to disable incremental by default on the
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this raises the question "will incremental be disabled on beta?"

It might be good to preemptively address that question.

@Mark-Simulacrum
Copy link
Member

@wesleywiser I think I'm currently inclined to leave beta and nightly in the non-disabled incremental states, but it's definitely a good idea to call out their status. I think it's not unlikely 1.53 will need to also ship without incremental, though.

Co-authored-by: est31 <est31@users.noreply.github.com>
Mark-Simulacrum and others added 3 commits May 10, 2021 08:51
Co-authored-by: est31 <est31@users.noreply.github.com>
Co-authored-by: est31 <est31@users.noreply.github.com>
@Mark-Simulacrum
Copy link
Member

Okay, merged in @est31's suggestions and a paragraph about the beta channel. @wesleywiser could you check that it seems good to you?

Co-authored-by: Felix S Klock II <pnkfelix@pnkfx.org>
wesleywiser
wesleywiser previously approved these changes May 10, 2021
Co-authored-by: Felix S Klock II <pnkfelix@pnkfx.org>
@pietroalbini pietroalbini changed the title Blog post explaining the fingerprint issue. 1.52.1 - Blog post explaining the fingerprint issue. May 10, 2021
Copy link
Member

@pietroalbini pietroalbini left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good!

We need to rename the file to posts/2021-05-10-Rust-1.52.1.md, as the date is wrong and the blog post slug is not the one we usually use for releases.

posts/2021-05-07-caught-red-handed.md Outdated Show resolved Hide resolved
Copy link
Member

@pietroalbini pietroalbini left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ready to merge as soon as we release!

@Mark-Simulacrum Mark-Simulacrum merged commit 06486a1 into rust-lang:master May 10, 2021
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.

9 participants