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

don't download std-docs on CI #7513

Merged
merged 7 commits into from
Nov 4, 2019
Merged

don't download std-docs on CI #7513

merged 7 commits into from
Nov 4, 2019

Conversation

Eh2406
Copy link
Contributor

@Eh2406 Eh2406 commented Oct 15, 2019

Now that we have a way to do it, lets not download std-docs on CI. Based on the blog post:
https://blog.rust-lang.org/2019/10/15/Rustup-1.20.0.html

@rust-highfive
Copy link

r? @ehuss

(rust_highfive has picked a reviewer for you, use r? to override)

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Oct 15, 2019
@Eh2406 Eh2406 closed this Oct 15, 2019
@Eh2406 Eh2406 reopened this Oct 15, 2019
@Eh2406
Copy link
Contributor Author

Eh2406 commented Oct 15, 2019

Windows strikes again. I think: rustup self update has changed the exe that rustup points to. So windows won't let us use rustup until we reopen powershell. Not sure the cleanest way to proceed.

@alexcrichton
Copy link
Member

It looks like it works now though, right? The changes here look good to me!

@ehuss
Copy link
Contributor

ehuss commented Oct 16, 2019

This looks good to me, too.

However, the performance improvement is a little disappointing. The Windows builder went from 2m 33s to 2m 7s. The linux builders have very little impact (dropping from about 28s to 25s). I don't have any ideas why the Windows is still so slow (does maybe deleting the component take a long time?). Anyway, I'd be fine to merge this.

Copy link
Contributor

@tesuji tesuji left a comment

Choose a reason for hiding this comment

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

The curl command works on all OSes.
See example here: https://github.com/lzutao/home/runs/261056912#step:3:1

echo "##vso[task.prependpath]$HOME/.cargo/bin"
fi
displayName: Install rustup

- bash: |
set -e
rustup set profile minimal
rustup component remove --toolchain=$TOOLCHAIN rust-docs || echo "already removed"
rustup update $TOOLCHAIN
Copy link
Contributor

@tesuji tesuji Oct 17, 2019

Choose a reason for hiding this comment

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

Suggested change
rustup update $TOOLCHAIN
rustup update --no-self-update $TOOLCHAIN

@@ -6,16 +6,18 @@ steps:
rustup self update
elif [ "$AGENT_OS" = "Windows_NT" ]; then
curl -sSf -o rustup-init.exe https://win.rustup.rs
rustup-init.exe -y --default-toolchain $TOOLCHAIN
rustup-init.exe -y --default-toolchain $TOOLCHAIN --profile=minimal
Copy link
Contributor

Choose a reason for hiding this comment

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

This works on all OSes

Suggested change
rustup-init.exe -y --default-toolchain $TOOLCHAIN --profile=minimal
curl -sSL https://sh.rustup.rs | sh -s -- -y --default-toolchain=$TOOLCHAIN --profile=minimal

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sorry for the delay. I am now back from vacation. How do we best combine the branches?

Copy link
Contributor

Choose a reason for hiding this comment

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

@ehuss
Copy link
Contributor

ehuss commented Oct 17, 2019

Hm, it really does take 70s to delete the rust-docs component. Well, hopefully some day in the future the default install will use the minimal profile so it won't be necessary.

@alexcrichton
Copy link
Member

That may be a bug in rustup perhaps that it takes so long to delete docs? I suspect that actually deleting the component shouldn't take 70 seconds

@ehuss
Copy link
Contributor

ehuss commented Oct 23, 2019

I don't think it is an issue with rustup. I see similar times on other CI systems. On my very fast local system with a very fast SSD, it takes about 13s to delete them. Doing the same thing inside a docker container on the same system takes over 35s. Using rm -r takes the same amount of time as rustup remove. It is not too surprising that the virtual images on azure are a little slower. I think Windows file IO is just really slow.

@alexcrichton
Copy link
Member

r=me on the changes whenever this is ready, but unfortunately I think our nightly CI has regressed

@ehuss
Copy link
Contributor

ehuss commented Nov 4, 2019

Yea, we need to add the rustc-dev component in rustup, due to rust-lang/rust#65474.

rustup update $TOOLCHAIN
rustup set profile minimal
rustup component remove --toolchain=$TOOLCHAIN rust-docs || echo "already removed"
rustup component add --toolchain=$TOOLCHAIN rustc-dev || echo "already installed"
Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe move it after the call to rustup update? I think it will fail otherwise, because nightly is not already installed, and "component add" won't be remembered. Also, the echo "already installed" might be a little misleading, since it gobbles any error (such as doesn't exist, or download failure).

Also, maybe it should only install rustc-dev when using the nightly toolchain? It won't be used for any of the others. I'm also not sure if will be published for the stable toolchains?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ok. Switched the order and removed the echo. Now everything brakes. How can we install it only on nightly, I is not so experienced with azure?

Copy link
Contributor

Choose a reason for hiding this comment

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

Oh, I was just thinking adding a conditional to the bash script, maybe something like:

if [ "$TOOLCHAIN" = "nightly" ]; then
  rustup component add --toolchain=$TOOLCHAIN rustc-dev
fi

just after rustup update.

You can also add it as a separate step with a condition, but I don't think it is necessary.

@Eh2406
Copy link
Contributor Author

Eh2406 commented Nov 4, 2019

CI is green!

@ehuss
Copy link
Contributor

ehuss commented Nov 4, 2019

Awesome, thanks!

@bors r+

@bors
Copy link
Contributor

bors commented Nov 4, 2019

📌 Commit e028b87 has been approved by ehuss

@bors bors 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 Nov 4, 2019
@bors
Copy link
Contributor

bors commented Nov 4, 2019

⌛ Testing commit e028b87 with merge ba5cca2...

bors added a commit that referenced this pull request Nov 4, 2019
don't download std-docs on CI

Now that we have a way to do it, lets not download std-docs on CI. Based on the blog post:
https://blog.rust-lang.org/2019/10/15/Rustup-1.20.0.html
@bors
Copy link
Contributor

bors commented Nov 4, 2019

☀️ Test successful - checks-azure
Approved by: ehuss
Pushing ba5cca2 to master...

@bors bors merged commit e028b87 into rust-lang:master Nov 4, 2019
bors added a commit to rust-lang/rust that referenced this pull request Nov 17, 2019
Update Cargo, books

## cargo

12 commits in 5da4b4d47963868d9878480197581ccbbdaece74..8280633db680dec5bfe1de25156d1a1d53e6d190
2019-10-28 21:53:41 +0000 to 2019-11-11 23:17:05 +0000
- Don't panic when parsing `/proc/stat` (rust-lang/cargo#7580)
- Fix unused configuration key warning for a few keys under `build`. (rust-lang/cargo#7575)
- Add back support for `BROWSER` envvar in `cargo doc --open`. (rust-lang/cargo#7576)
- Only include "already existing ..." comment in gitignore on conflict (rust-lang/cargo#7570)
- Add VS Code user dir to .gitignore (rust-lang/cargo#7578)
- Added aliases to subcommand typo suggestions. (rust-lang/cargo#7486)
- Use multiple requirement syntax consistently (rust-lang/cargo#7573)
- Update verison to 0.42 (rust-lang/cargo#7568)
- Expand documentation on build scripts. (rust-lang/cargo#7565)
- Update crossbeam-utils requirement from 0.6 to 0.7 (rust-lang/cargo#7566)
- don't download std-docs on CI (rust-lang/cargo#7513)
- Change my-buddy to github-handle (rust-lang/cargo#7553)

## nomicon

2 commits in 5004ad30d69f93553ceef74439fea2159d1f769e..58e36e0e08dec5a379ac568827c058e25990d6cd
2019-10-12 19:52:40 +0200 to 2019-10-30 08:14:24 -0500
- remove references to the nursery
- Add github action to replace Travis.yml (rust-lang/nomicon#172)

## reference

7 commits in 4b21b64..45558c4
2019-10-27 22:33:11 +0100 to 2019-11-08 14:47:35 +0100
- Audit code blocks. (rust-lang/reference#715)
- Update coherence and orphan rules documentation to match RFC 2451 (rust-lang/reference#703)
- Update organization name (rust-lang/reference#713)
- State that no_implicit_prelude also applies to nested modules (rust-lang/reference#707)
- expand Copy docs (rust-lang/reference#711)
- github action doesn't use the nursery (rust-lang/reference#706)
- Migrate to GitHub Actions. (rust-lang/reference#705)

## book

1 commits in 28fa3d15b0bc67ea5e79eeff2198e4277fc61baf..e79dd62aa63396714278d484d91d48826737f47f
2019-10-29 07:16:09 -0500 to 2019-10-30 07:33:12 -0500
- No need for an iterator here to fetch values (rust-lang/book#1957)

## rust-by-example

1 commits in f3197ddf2abab9abdbc029def8164f4a748b0d91..dcee312c66267eb5a2f6f1561354003950e29105
2019-10-29 10:17:40 -0300 to 2019-10-31 11:26:53 -0300
- refactor: simplify extracting Result from Option (rust-lang/rust-by-example#1283)

## edition-guide

2 commits in e58bc4ca104e890ac56af846877c874c432a64b5..f553fb26c60c4623ea88a1cfe731eafe0643ce34
2019-07-31 20:14:12 +0200 to 2019-10-30 08:27:42 -0500
- remove old references to the nursery
- Port from Travis to GitHub Actions (rust-lang/edition-guide#192)
@ehuss ehuss added this to the 1.41.0 milestone Feb 6, 2022
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.

6 participants