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

Prep miri repository for rustc merger #258

Merged
merged 1 commit into from
Jul 21, 2017
Merged

Conversation

oli-obk
Copy link
Contributor

@oli-obk oli-obk commented Jul 19, 2017

Just an initial step, nothing removed yet, even the miri executable is moved along

r? @eddyb

can I have libmiri or does it really need to be librustc_miri?

@eddyb
Copy link
Member

eddyb commented Jul 19, 2017

I'd expect it to be a subdirectory of librustc_mir because of possible circular dependencies. There's no reason to have a million crates.

@RalfJung
Copy link
Member

Now I hope git will be able to do a rebase of my validation branch across these renames properly... rebasing around memory.rs changes has already been enough "fun"^^

@oli-obk
Copy link
Contributor Author

oli-obk commented Jul 19, 2017

Now I hope git will be able to do a rebase of my validation branch across these renames properly... rebasing around memory.rs changes has already been enough "fun"^^

git should normally simply skip over such things.

@oli-obk
Copy link
Contributor Author

oli-obk commented Jul 19, 2017

I'd expect it to be a subdirectory of librustc_mir because of possible circular dependencies. There's no reason to have a million crates.

sgtm

How are we going to run miri's tests? Without fullmir, most tests will fail. But last time I tried to get full mir into nightly that got turned down due to a 5MB increase in binary size.

For testing I though we could introduce a new (obviously unstable) attribute, which triggers miri to try to run the main function in the file. This should allow easy integration into the existing run-pass and compile-fail tests.

@RalfJung
Copy link
Member

Is there a description of the high-level plan somewhere? Right now I have no idea how things are supposed to be organized after the merge.
Also: How will "cargo miri" work? Will it still be possible to run miri on a given .rs file?

@oli-obk
Copy link
Contributor Author

oli-obk commented Jul 19, 2017

There's some info here: #105

Also: How will "cargo miri" work? Will it still be possible to run miri on a given .rs file?

If I get my attribute, you can just trigger the attribute via some -Z flag, acting just like cargo miri (which I think we need for testing in rustc anyway, otherwise miri will quickly become buggy if we can't properly test all the paths that const eval doesn't need right now)

Otherwise, we keep cargo miri here and access the miri library via librustc_mir::miri.

@oli-obk
Copy link
Contributor Author

oli-obk commented Jul 19, 2017

also: is it fine for a module of a rustc crate to be Apache/MIT dual licensed?

@RalfJung
Copy link
Member

There's some info here: #105

I read this a while ago, but it doesn't actually make any concrete plans, does it? The "rls vs clippy way" is not answered, for example.
So, will miri just wholesale move to rustc and PRs will have to be sent there? (That would probably slow down things considerably, but maybe that's okay.) Will the history be preserved? Will there just be a submodule?

Otherwise, we keep cargo miri here and access the miri library via librustc_mir::miri.

That would however not execute these tests when the in-tree miri gets changed...

@oli-obk
Copy link
Contributor Author

oli-obk commented Jul 19, 2017

So, will miri just wholesale move to rustc and PRs will have to be sent there?

yes see rust-lang/rust#43340

(That would probably slow down things considerably, but maybe that's okay.)

you'll have to rebase one repository less ;)

Will the history be preserved?

yes, has been approved in #105

Will there just be a submodule?

I simply moved miri to librustc_mir::miri and fixed all the import errors.

@oli-obk
Copy link
Contributor Author

oli-obk commented Jul 19, 2017

That would however not execute these tests when the in-tree miri gets changed...

i know, might be necessary at first. But if MIR-only libs come, everything should work inside rustc again.

@RalfJung
Copy link
Member

i know, might be necessary at first. But if MIR-only libs come, everything should work inside rustc again.

I'd prefer if we could avoid that. However, it seems that would require building libstd twice as part of Rust testing? Once for the release artifact, once with MIR? Hm. :/

@oli-obk
Copy link
Contributor Author

oli-obk commented Jul 19, 2017

I'd prefer if we could avoid that. However, it seems that would require building libstd twice as part of Rust testing? Once for the release artifact, once with MIR? Hm. :/

we already do that for the stages... maybe we could generate stage 1 with full MIR and stage 2 without. And run miri tests only in stage 1.

@RalfJung
Copy link
Member

Hm, interesting. Depending on how my mir-validation stuff gets merged, that may also be another flag that at least some miri tests want to have turned on -- to run the "types as contracts" test suite. (I planned for that to be a thing in miri only, and just use xargo to build libstd with this flag. This merger makings things more... interesting.^^)

@RalfJung
Copy link
Member

In principle another option would be to have the rust test suite "trigger" another test suite running elsewhere, which the fetches the newly produced release artifacts, uses xargo to build libstd, and runs the tests. Not sure if that can be integrated into rust's test suite though, to make it so that bors fails if this external test suite fails.

@oli-obk
Copy link
Contributor Author

oli-obk commented Jul 19, 2017

In principle another option would be to have the rust test suite "trigger" another test suite running elsewhere, which the fetches the newly produced release artifacts, uses xargo to build libstd, and runs the tests. Not sure if that can be integrated into rust's test suite though, to make it so that bors fails if this external test suite fails.

That sounds very complex and prone to failure.

@oli-obk
Copy link
Contributor Author

oli-obk commented Jul 20, 2017

Now I hope git will be able to do a rebase of my validation branch across these renames properly... rebasing around memory.rs changes has already been enough "fun"^^

I just rebased this PR onto the current master. git resolved everything (correctly!) without requiring intervention by me. I'd wager that this'll also work the other way around.

@oli-obk oli-obk force-pushed the upstream branch 2 times, most recently from c1ee928 to f27f749 Compare July 20, 2017 14:15
@oli-obk
Copy link
Contributor Author

oli-obk commented Jul 20, 2017

everything's now in librustc_mir/interpret

@oli-obk oli-obk force-pushed the upstream branch 2 times, most recently from 15ec7da to 3112c99 Compare July 20, 2017 18:42
@oli-obk oli-obk merged commit 6143ef0 into rust-lang:master Jul 21, 2017
@oli-obk oli-obk deleted the upstream branch July 21, 2017 10:32
@RalfJung
Copy link
Member

This pretty much entirely broke using vscode with miri :(

"cargo build --all" and "cargo test --all" in the root folder really should work.

@eddyb
Copy link
Member

eddyb commented Jul 22, 2017

@RalfJung I'd expect that'd be fixed after the partial move to rust-lang/rust.

@RalfJung RalfJung mentioned this pull request Jul 22, 2017
@oli-obk
Copy link
Contributor Author

oli-obk commented Jul 22, 2017

I have a branch ready that should fix all such problems, since it does the partial move within this repo

@RalfJung
Copy link
Member

You have a link? I was about to prep a PR that makes miri a workspace. Building already works, but I am still working on getting the tests passing again.

(Btw, there is a stale Cargo.lock in the root, and there is no Cargo.lock in the actual sources any more.)

@oli-obk
Copy link
Contributor Author

oli-obk commented Jul 22, 2017

@oli-obk
Copy link
Contributor Author

oli-obk commented Jul 22, 2017

Just run vscode from the miri directory

@RalfJung
Copy link
Member

The trouble is, then it does not show the tests in the left.

@oli-obk
Copy link
Contributor Author

oli-obk commented Jul 23, 2017

Not sure what you mean. I moved the tests into miri/tests, so they're in a subdir now.

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.

3 participants