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

Fix sysroot handling #772

Merged
merged 2 commits into from
Jun 14, 2019
Merged

Fix sysroot handling #772

merged 2 commits into from
Jun 14, 2019

Conversation

RalfJung
Copy link
Member

No description provided.

@RalfJung RalfJung changed the title change sysroot check to print the output in case of an error Fix sysroot handling Jun 14, 2019
@RalfJung
Copy link
Member Author

Turns out I screwed up in my last sysroot PR, I hope this PR (updated it) now fixes that.

I also changed it so that the panic from compile_time_sysroot surfaces even if MIRI_SYSROOT is set; this means we actually test that that panic dos not happen as part of the test suite.

@RalfJung
Copy link
Member Author

CI looks good. Still, this needs a new review I think.

@oli-obk
Copy link
Contributor

oli-obk commented Jun 14, 2019

lgtm, but I'm confused how this happened. Is it because I created the sysroot hacks in our test suite and thus changes in the miri binary were ignored by our tests since the tests passed their own --sysroot?

@RalfJung RalfJung merged commit 4736692 into rust-lang:master Jun 14, 2019
@RalfJung RalfJung deleted the sysroot branch June 14, 2019 12:28
@RalfJung
Copy link
Member Author

RalfJung commented Jun 14, 2019

What happened is that the test suite in rustc always passes a --sysroot, yes. Same for our test suite here though, and same for cargo miri. So I think it has been the case since forever that the fallback code (with no MIRI_SYSROOT/--sysroot) panicked, it's just that we never ran into that code.

But now I added this sanity check in cargo miri that does exactly that.

@RalfJung
Copy link
Member Author

Oh also we don't test cargo miri at all on the rustc side. Would be great if we could somehow make that happen -- and maybe also recompile libstd there instead of relying on setting --cfg miri in bootstrap, which I feel uneasy about.

@oli-obk
Copy link
Contributor

oli-obk commented Jun 14, 2019

maybe also recompile libstd there instead of relying on setting --cfg miri in bootstrap

You mean via cargo miri setup or just straight up adding an additional libstd-miri thing in bootstrap that Miri depends on?

@RalfJung
Copy link
Member Author

Well ideally the same way users do it, so via cargo miri setup.

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.

2 participants