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

Move rustc_back modules where they belong. #46305

Merged
merged 3 commits into from
Dec 5, 2017
Merged

Move rustc_back modules where they belong. #46305

merged 3 commits into from
Dec 5, 2017

Conversation

irinagpopa
Copy link
Contributor

No description provided.

@rust-highfive
Copy link
Collaborator

Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @eddyb (or someone else) soon.

If any changes to this PR are deemed necessary, please add them as extra commits. This ensures that the reviewer can see what has changed since they last reviewed the code. Due to the way GitHub handles out-of-date commits, this should also make it reasonably obvious what issues have or haven't been addressed. Large or tricky changes may require several passes of review and changes.

Please see the contribution instructions for more information.

@eddyb
Copy link
Member

eddyb commented Nov 27, 2017

r? @alexcrichton What should be done about the tests that use tempdir?

@rust-highfive rust-highfive assigned alexcrichton and unassigned eddyb Nov 27, 2017
@carols10cents carols10cents added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Nov 27, 2017
@@ -66,6 +66,7 @@ extern crate rustc_errors as errors;
extern crate serialize;
#[cfg(windows)]
extern crate cc; // Used to locate MSVC
pub extern crate tempdir;
Copy link
Contributor

@arielb1 arielb1 Nov 27, 2017

Choose a reason for hiding this comment

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

why "pub extern crate"? Shoulldn't this import be private?

Copy link
Contributor

Choose a reason for hiding this comment

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

Ah ok you are exporting it for tests so that they don't need to use Cargo. Could you add a comment to that effect?

@@ -12,11 +12,11 @@

#![feature(rustc_private)]
Copy link
Contributor

Choose a reason for hiding this comment

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

I suppose you can remove this test if we're moving to tempdir from crates.io

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The reason as to why we don't change or remove the tests it's because the tests just happen to use tempdir, these don't test tempdir itself.

@bors
Copy link
Contributor

bors commented Nov 27, 2017

☔ The latest upstream changes (presumably #44884) made this pull request unmergeable. Please resolve the merge conflicts.

@petrochenkov
Copy link
Contributor

The slice stuff is available in libcore now (#45306).
Not sure if it landed in the bootstrap compiler yet, but at least the rustc_back/rustc_data_structures version can be cfg(stage0)-ed easily.

@alexcrichton
Copy link
Member

Looks pretty good to me, thanks!

You can also import tempdir directly in tests I think:

#![feature(rustc_private)]

extern crate tempdir;

// ...

@eddyb
Copy link
Member

eddyb commented Nov 27, 2017

@petrochenkov Oh, great, I didn't notice. If the libcore functions aren't #[cfg(stage0)] we can just use them directly, the bootstrap compiler's libstd doesn't really matter.

@alexcrichton We tried that and it wasn't found, unlike the rustc_* crates.

@alexcrichton
Copy link
Member

@eddyb that means that the test wasn't set up correctly or something like that, the crate exists and it can be linked to.

@shepmaster
Copy link
Member

@alexcrichton it looks like new commits have been pushed!

@eddyb
Copy link
Member

eddyb commented Dec 1, 2017

@shepmaster There's still more we want to do here (at the very least #46305 (comment)), but @irinagpopa won't be able to get back to it until next week.

@shepmaster shepmaster added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Dec 1, 2017
@eddyb
Copy link
Member

eddyb commented Dec 4, 2017

Ah, no point in waiting for more, at least it's self-contained and slice is gone now.

@bors r=alexcrichton,eddyb

@bors
Copy link
Contributor

bors commented Dec 4, 2017

📌 Commit 2c175df has been approved by alexcrichton,eddyb

bors added a commit that referenced this pull request Dec 5, 2017
Move rustc_back modules where they belong.
@bors
Copy link
Contributor

bors commented Dec 5, 2017

⌛ Testing commit 2c175df with merge a4fa23a...

@bors
Copy link
Contributor

bors commented Dec 5, 2017

☀️ Test successful - status-appveyor, status-travis
Approved by: alexcrichton,eddyb
Pushing a4fa23a to master...

@bors bors merged commit 2c175df into rust-lang:master Dec 5, 2017
@irinagpopa irinagpopa deleted the backstory branch December 5, 2017 13:40
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

9 participants