improve README instructions for using `rustup` and for compiling separate crates #483

apoelstra · 2018-10-15T18:46:54Z

I found that with the existing instructions I would get errors along the lines of "libstd not found", or "dependency was built against possibly newer libstd", and it was very difficult to tell what was going on because there are many moving parts and actually multiple things that can go wrong.

Update the README to hold the user's hand more and give troubleshooting help.

RalfJung · 2018-10-16T06:43:45Z

README.md

-project through Miri.
+Compile your project and its dependencies against a MIR-enabled libstd as described
+above. The easiest way to do this is to copy `xargo/build.sh` and `xargo/Xargo.toml`
+into your own project's root, and to re-run `build.sh`. This ensures that `libstd`


I am a bit surprised by this. build.sh installs the new libstd globally, so you can use MIRI_SYSROOT=~/.xargo/HOST cargo +nightly miri even without copying it. If you are using the same compiler, this locally built libstd should be compatible with the one shipped with rustc.

Ah! If you run cargo clean first you are correct. But cargo will use cached dependency compilations even if you change MIRI_SYSROOT. (Maybe this is a bug in cargo?)

I don't see how your instructions can make any difference there. If you used xargo build later, that would take the new libstd into account, but cargo build for sure doesn't care about ~/.xargo/HOST.

Something doesn't add up here.

I think there are actually some bugs here, and I opened #486 to track that.

But none of that should require a copy of Xargo.toml, and I have yet to see a situation where that helps.

In my project (rust-bitcoin), if I run

cargo clean MIRI_SYSROOT=~/.xargo/HOST cargo +nightly-2018-10-14 build MIRI_SYSROOT=~/.xargo/HOST cargo +nightly-2018-10-14 miri ## does not error, seems to do nothing MIRI_SYSROOT=~/.xargo/HOST cargo +nightly-2018-10-14 miri test

I will reliably get the error

error[E0460]: found possibly newer version of crate `std` which `bitcoin_bech32` depends on

My fix was to bring Xargo.toml into the crate root and run the commands in build.sh there. But now I see that a more straightforward thing to do is to just run cargo clean before MIRI_SYSROOT=~/.xargo/HOST cargo miri test and it'll work.

This definitely seems like some sort of bug rather than something we should update the README for.

I think the following requirements are important to get cargo miri to work with a local libstd:

xargo/build.sh must be executed with the same toolchain as cargo miri.

MIRI_SYSROOT must be set when the project is built.

Can you write the instructions in a way that makes that as clear as possible?

README.md

RalfJung · 2018-10-16T06:44:51Z

README.md

+
+1. Install Miri using `rustup run nightly-2018-10-15 cargo install --all-features --path .`,
+with the date replaced as appropriate.
+2. Edit `build.sh` to replace `xargo` with `rustup run nightly-2018-10-15 xargo`.


Instead, I think we can tell people to run rustup run nightly-2018-10-15 xargo/build.sh

Ah, nice, I'll fix this.

RalfJung · 2018-10-17T08:05:19Z

README.md

+
+1. Install Miri using `cargo +nightly-2018-10-15 install --all-features --path .`,
+with the date replaced as appropriate.
+2. Run `build.sh` as `rustup run nightly-2018-10-15 build.sh`.


Should be xargo/build.sh, shouldn't it?

apoelstra · 2018-10-19T15:04:35Z

Rebased, addressed nits, and replaced the "copy build.sh" instructions with the much simpler "Run cargo clean before building your project".

RalfJung · 2018-10-20T10:05:32Z

README.md

+1. Run `cargo clean` to eliminate any cached dependencies that were built against
+the non-MIR `libstd`.
+2. For a binary project, run `MIRI_SYSROOT=~/.xargo/HOST cargo +nightly miri` to
+build and run your project; for a binary or library, use `MIRI_SYSROOT=~/.xargo/HOST cargo +nightly miri test`


I'd rearrange the sentence a bit here: The prat after the semicolon I'd start with what this is good for ("to run all tests"). Reading this I wondered "wait the first part is for a binary, the second part is for a binary or library, that's odd".

RalfJung · 2018-10-20T10:07:19Z

README.md

+
+If you forget to set `MIRI_SYSROOT`, be sure to run `cargo clean` again before
+correcting it. Otherwise you are likely to get "dependency was built against possibly
+newer std" errors.


Maybe add a remark here saying if this throws strange errors about crate mismatches, probably the xargo sysroot was built with a different nightly than the current project and one should run rustup run nightly xargo/build.sh.

RalfJung · 2018-10-20T10:08:40Z

README.md

+
+You may prefer to do this rather than depending on the rustup default toolchain,
+if you routinely update the default, since **it is essential that `xargo/build.sh`
+is run with the same toolchain as `cargo miri`.**


I find this somewhat redundant. If we make the original instructions specify a toolchain everywhere, it seems clear that we can replace nightly by another toolchain name, doesn't it? It may be worth stating that explicitly, but I am not a fan of repeating instructions.

README.md

bjorn3 · 2018-10-21T16:40:46Z

rust-toolchain

@@ -1 +0,0 @@
-nightly-2018-10-14


Could you instead just replace it with nightly so that people don't have to switch to nightly manually, while at the same time not pinning to a specific nightly?

They will still have to switch to nightly manually when doing the other steps; this override only controls compilation of Miri. This will be especially confusing if they have their default compiler set to a specific nightly, which is common for people using unstable compiler tools.

apoelstra · 2018-10-21T16:50:47Z

README.md

-To target a specific nightly, modify the above instructions as follows.
+To target a specific nightly, modify the above instructions as follows. It is recommended
+to use the nightly specified in the `rust-version` file in this repo, since that is the
+most recent nightly supported by Miri.


Maybe I should bold this? It's pretty far down. Or maybe I should move it to the top of the README?

I would get rid of this entire section, and instead write "Building Miri" and the following sections in a way that explicitly specifies nightly in every step (we have to, with rust-toolchain gone!). Then I think we can avoid repeating these instructions here?

Sounds good.

RalfJung · 2018-10-22T18:37:36Z

Most of the README, starting at "Building Miri", now needs to be adjusted to add the +nightly. It should also explain that if that doesn't work, one can find a toolchain that does work in the rust-version file.

RalfJung · 2018-10-22T18:38:11Z

README.md

+
+### Common Problems
+
+When modifying the above instructions, you may encounter a number of confusing compiler


Why "modifying"?

I was trying to politely say "you typed the instructions wrong".

lol, that subtlety was lost on me.^^ Maybe just say "When using" or "when working with"?

Heh, yeah, I guess that's even more polite :)

RalfJung · 2018-10-22T18:39:21Z

README.md

+have `MIRI_SYSROOT` set. Run `cargo clean` before switching from non-Miri to Miri
+builds and vice-versa.
+
+#### "found crate `std` compiled by an incompatible version of rustc"


I do not understand how/why this is different from the one right before. Seems to me both are caused by "sysroot confusion", and both are solved the same way. Why should we give different advise for these two?

I don't understand. They have different causes and different solutions.

Oh, the difference is "deps were built without MIRI_SYSROOT vs MIRI_SYSROOT was built with the wrong toolchain, I see. Makes sense.

I think what threw me off was specifically mentioning cargo miri test. The same applies to cargo miri, right? So maybe just say cargo miri then.

RalfJung · 2018-10-23T08:58:28Z

README.md


-Install Miri as a cargo subcommand with `cargo install --all-features`, and install
-a full libstd as described above.
+Install Miri as a cargo subcommand with `cargo install --all-features --path .`.


This and everything above it in the README still lacks a +nightly. The instructions as they are now just don't work.

…rate Cargo projects

…toolchain

…sage

…ghout Also replace `cargo miri test` with `cargo miri` in general examples.

apoelstra · 2018-10-23T15:23:19Z

Rebased; added a commit which

deletes the "installing Miri with a specific nightly" instructions and instead uses +nightly throughout
replaces cargo miri test with cargo miri in the troubleshooting cases where the distinction doesn't matter

RalfJung · 2018-10-24T12:48:48Z

I like it. :) Still have some tweaks I want to do but I will just make a PR of my own then and ask you for feedback.

Cycling PR because the Travis state is stuck.

apoelstra force-pushed the 2018-10-readme branch 2 times, most recently from 0c83b2f to 8259670 Compare October 15, 2018 18:53

RalfJung reviewed Oct 16, 2018

View reviewed changes

README.md Outdated Show resolved Hide resolved

RalfJung reviewed Oct 16, 2018

View reviewed changes

apoelstra force-pushed the 2018-10-readme branch 2 times, most recently from 0241f1b to bcf4476 Compare October 16, 2018 18:04

RalfJung reviewed Oct 17, 2018

View reviewed changes

apoelstra force-pushed the 2018-10-readme branch from bcf4476 to 29d1966 Compare October 19, 2018 15:04

RalfJung reviewed Oct 20, 2018

View reviewed changes

README.md Show resolved Hide resolved

apoelstra commented Oct 20, 2018

View reviewed changes

README.md Show resolved Hide resolved

apoelstra force-pushed the 2018-10-readme branch 2 times, most recently from 8f8d65a to e22edfb Compare October 20, 2018 16:31

bjorn3 requested changes Oct 21, 2018

View reviewed changes

apoelstra force-pushed the 2018-10-readme branch from 12ed60a to 67895b7 Compare October 21, 2018 16:47

apoelstra commented Oct 21, 2018

View reviewed changes

RalfJung reviewed Oct 22, 2018

View reviewed changes

RalfJung reviewed Oct 23, 2018

View reviewed changes

apoelstra added 5 commits October 23, 2018 15:22

improve README instructions for using rustup and for compiling sepa…

95f7406

…rate Cargo projects

added line indicating that build.sh and cargo miri need the same …

f77b292

…toolchain

readme: pull "common problems" into their own section

3dcf655

rename rust-toolchain to rust-version; add note to README about u…

abda1a8

…sage

README: remove "specific nightly" instructions and use +nightly throu…

5347411

…ghout Also replace `cargo miri test` with `cargo miri` in general examples.

apoelstra force-pushed the 2018-10-readme branch from 67895b7 to 5347411 Compare October 23, 2018 15:22

RalfJung closed this Oct 24, 2018

RalfJung reopened this Oct 24, 2018

RalfJung merged commit 1cbed5c into rust-lang:master Oct 24, 2018


		### Common Problems

		When modifying the above instructions, you may encounter a number of confusing compiler

		@@ -1 +0,0 @@
		nightly-2018-10-14

improve README instructions for using rustup and for compiling separate crates #483

improve README instructions for using rustup and for compiling separate crates #483

Uh oh!

Conversation

apoelstra commented Oct 15, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

apoelstra commented Oct 19, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

RalfJung commented Oct 22, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

apoelstra commented Oct 23, 2018

Uh oh!

RalfJung commented Oct 24, 2018

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

improve README instructions for using `rustup` and for compiling separate crates #483

improve README instructions for using `rustup` and for compiling separate crates #483

apoelstra commented Oct 15, 2018 •

edited

Loading