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

Fixes other targets rustlibs installation #40479

Merged
merged 1 commit into from
Apr 6, 2017
Merged

Conversation

sezaru
Copy link

@sezaru sezaru commented Mar 13, 2017

When the user select more than one target to generate rustlibs for, rustbuild will only install the host one.

This patch fixes it, more info in #39235 (comment)

@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 @alexcrichton (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.

@@ -49,6 +49,12 @@ pub fn install(build: &Build, stage: u32, host: &str) {
install_sh(&build, "docs", "rust-docs", stage, host, &prefix,
&docdir, &libdir, &mandir, &empty_dir);
}

for target in build.config.target.iter().filter(|target| *target != host) {
Copy link
Member

Choose a reason for hiding this comment

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

Perhaps this filter could be dropped and the rust-std install statement below could get folded into this one?

Copy link
Author

Choose a reason for hiding this comment

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

I added it because I'm not sure if build.config.target will always have the host target in it, if that is true, then sure, we can safely remove the filter.

Copy link
Member

Choose a reason for hiding this comment

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

Ah yeah that's always true, config.target is a superset of config.host which always contains config.build

@bors
Copy link
Contributor

bors commented Mar 15, 2017

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

@alexcrichton
Copy link
Member

Thanks! Could you also squash the commits together?

@sezaru
Copy link
Author

sezaru commented Mar 16, 2017

Sorry for the dumb question, but I'm not sure how I can do it correctly, When I try to do a git rebase -i HEAD~3, it will bring all the commits between mines, should I squash then all?

@sezaru
Copy link
Author

sezaru commented Mar 16, 2017

Also, it seems some checks failed on travis-ci, but there is no log on the ones that fail.

@alexcrichton
Copy link
Member

Oh you may wish to execute git rebase origin/master first to remove the merge commit, and then -i HEAD~2 should do the trick (just your commits) I believe.

Also yeah don't worry too much about the CI, the relevant builder passed.

@sezaru
Copy link
Author

sezaru commented Mar 16, 2017

Hey Alex, I just did the squash, the last commit contains all the changes, but for some reason all the other commits still exists behind it, is that the desired result? Maybe that happened because I made the changes directly in my rust fork master instead of a branch.

@alexcrichton
Copy link
Member

Hm yeah I've seen oddness before with PRs coming from a master branch, can you perhaps switch it to a different branch?

When the user select more than one target to generate rustlibs for, rustbuild will only install the host one.

This patch fixes it, more info in rust-lang#39235 (comment)
@alexcrichton
Copy link
Member

@bors: r+

@bors
Copy link
Contributor

bors commented Apr 5, 2017

📌 Commit 56902fb has been approved by alexcrichton

@bors
Copy link
Contributor

bors commented Apr 5, 2017

⌛ Testing commit 56902fb with merge 66e2b6c...

@bors
Copy link
Contributor

bors commented Apr 5, 2017

💔 Test failed - status-appveyor

@alexcrichton
Copy link
Member

alexcrichton commented Apr 5, 2017 via email

frewsxcv added a commit to frewsxcv/rust that referenced this pull request Apr 5, 2017
Fixes other targets rustlibs installation

When the user select more than one target to generate rustlibs for, rustbuild will only install the host one.

This patch fixes it, more info in rust-lang#39235 (comment)
arielb1 pushed a commit to arielb1/rust that referenced this pull request Apr 5, 2017
Fixes other targets rustlibs installation

When the user select more than one target to generate rustlibs for, rustbuild will only install the host one.

This patch fixes it, more info in rust-lang#39235 (comment)
@bors
Copy link
Contributor

bors commented Apr 5, 2017

⌛ Testing commit 56902fb with merge 09deb66...

@arielb1
Copy link
Contributor

arielb1 commented Apr 5, 2017

@bors retry - prioritizing rollup

bors added a commit that referenced this pull request Apr 6, 2017
Rollup of 12 pull requests

- Successful merges: #40479, #40561, #40709, #40815, #40909, #40927, #40943, #41015, #41028, #41052, #41054, #41065
- Failed merges:
@bors bors merged commit 56902fb into rust-lang:master Apr 6, 2017
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.

5 participants