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

use shared scripts for init and sccache in cross image #42218

Merged
merged 2 commits into from
May 29, 2017

Conversation

venkatagiri
Copy link
Contributor

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

@alexcrichton
Copy link
Member

@bors: r+

Thanks @venkatagiri!

@bors
Copy link
Contributor

bors commented May 25, 2017

📌 Commit 5ce2eb1 has been approved by alexcrichton

@malbarbo
Copy link
Contributor

malbarbo commented May 25, 2017

@venkatagiri LGTM, thanks!

If your are interested, you could extract this and put in a script install-mips-musl.sh, and this in a script install-mipsel-musl.sh. Otherwise, I think we are ready to go. @alexcrichton?

Edit: I saw @alexcrichton r+ after i wrote this comment

@alexcrichton
Copy link
Member

Eh the URLs there are different enough it may not be worth to do so, but I'd be fine either way!

@malbarbo
Copy link
Contributor

Could you clarify what you mean by "Eh the URLs there are different enough it may not be worth to do so"?

My suggestion was to create two scripts, so that following the image build is easy because the reader is not distract by details. It would read, build-rumprun, build-arm-musl, install-mips-musl, install-mipsel-musl, etc.

@venkatagiri
Copy link
Contributor Author

venkatagiri commented May 25, 2017 via email

@alexcrichton
Copy link
Member

Oh I'd be fine either way. I personally prefer for one-off scripts to just be in the Dockerfile, but I don't mind doing otherwise!

@alexcrichton alexcrichton added the S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. label May 25, 2017
@venkatagiri
Copy link
Contributor Author

@alexcrichton I have extracted the musl installation from Dockerfile into scripts.

@venkatagiri
Copy link
Contributor Author

Fixed the tidy errors by splitting the URL into variables.

@alexcrichton
Copy link
Member

@bors: r+

Looks great!

@bors
Copy link
Contributor

bors commented May 25, 2017

📌 Commit cb7dbd5 has been approved by alexcrichton

Mark-Simulacrum added a commit to Mark-Simulacrum/rust that referenced this pull request May 26, 2017
…chton

use shared scripts for init and sccache in cross image

cc rust-lang#42201
cc @malbarbo
@Mark-Simulacrum
Copy link
Member

@bors r-

[00:05:23] Step 11 : RUN ./install-mips-musl.sh
[00:05:23]  ---> Running in e58d0caf3c29
[00:05:23] /bin/sh: 1: ./install-mips-musl.sh: Permission denied
[00:05:23] The command '/bin/sh -c ./install-mips-musl.sh' returned a non-zero code: 126
[00:05:23] The command has failed after 5 attempts.

@venkatagiri
Copy link
Contributor Author

@Mark-Simulacrum Fixed the file permissions.

@Mark-Simulacrum
Copy link
Member

I seem to recall a chmod +x being necessary to do so -- but I may be wrong. I'm going to holdoff on re- r+ until someone more familiar comes along to re-review.

@Mark-Simulacrum Mark-Simulacrum added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. labels May 26, 2017
@venkatagiri
Copy link
Contributor Author

Yes. I ran chmod +x on the files and merged them into last commit.
The file shows as "Executable file" now. https://github.com/venkatagiri/rust/blob/7acc999b4249a774dd617153524e3819b2ab9da7/src/ci/docker/cross/install-mips-musl.sh

@Mark-Simulacrum
Copy link
Member

Ah, okay, I looked in the scripts and I think I was wrong -- we don't actually need to add +x beyond what we store in git.

@bors r=alexchrichton

@bors
Copy link
Contributor

bors commented May 26, 2017

📌 Commit 7acc999 has been approved by alexchrichton

Mark-Simulacrum added a commit to Mark-Simulacrum/rust that referenced this pull request May 27, 2017
…ichton

use shared scripts for init and sccache in cross image

cc rust-lang#42201
cc @malbarbo
@Mark-Simulacrum Mark-Simulacrum added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels May 28, 2017
@bors
Copy link
Contributor

bors commented May 28, 2017

⌛ Testing commit 7acc999 with merge 920c479...

bors added a commit that referenced this pull request May 28, 2017
use shared scripts for init and sccache in cross image

cc #42201
cc @malbarbo
@bors
Copy link
Contributor

bors commented May 29, 2017

☀️ Test successful - status-appveyor, status-travis
Approved by: alexchrichton
Pushing 920c479 to master...

@bors bors merged commit 7acc999 into rust-lang:master May 29, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants