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

Improve Travis CI log (travis_fold, colors) #42128

Merged
merged 5 commits into from
Jun 2, 2017

Conversation

kennytm
Copy link
Member

@kennytm kennytm commented May 20, 2017

Result looks like (https://travis-ci.org/rust-lang/rust/jobs/234614611):

Full page screenshot

1-fullpage


  • Bring back colors on Travis, which was disabled since Remove strictly-unnecessary flags for docker #39036. Append --color=always to cargo when running in CI environment.
  • Removed set -x from the shell scripts. The retry function already prints which command it is running, adding -x just adds noise to the output. Interesting information can be manually echoed.
  • Support travis_fold/travis_time. Matching pairs of these allow Travis CI to collapse the output in between. This greatly cut down the unnecessary "successful" output one need to scroll through before finding the failed statement.
  • Passed --quiet to all tests, so tests not failing will not occupy a line of log, reducing bloat in the report.

Also include some minor changes, like changing the script of .travis.yml to execute a single-line command, so the log won't write the extremely long multi-line
The command "if [ "$ALLOW_PR" = "" ] && [ "$TRAVIS_BRANCH" != "auto" ]; then … " exited with 0 at the end.

@rust-highfive
Copy link
Collaborator

r? @brson

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

@kennytm
Copy link
Member Author

kennytm commented May 20, 2017

Travis CI still cuts the pretty log after 10000 lines even with folding. Currently Rust generates ~23000 lines. Viewing the raw logs pretty much negates all the benefits here.

  • This PR simply remove set -x which reduces ~140 lines.
  • Skipping the features table from make tidy can remove ~1100 lines (tidy is run twice).
  • Given that names and details of failed tests will be printed again, perhaps the compiletests should be run with the --quiet flag, which should be able to purge ~7000 lines. But non-failing status for each individual test will be lost this way.

AppVeyor currently picks up almost no improvement. Coloring is not enabled even with explicit --color always because AppVeyor's console is not a real Windows console, yet a term::WinConsole can be allocated (Stebalien/term#62). AppVeyor supports coloring via ANSI code (like xterm) but it has no terminfo, so a term::TerminfoTerminal can't be created either.

@Mark-Simulacrum Mark-Simulacrum added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label May 20, 2017
.gitignore Outdated
@@ -101,3 +101,5 @@ version.ml
version.texi
.cargo
!src/vendor/**
**/target/rls/debug/
/src/target/
Copy link
Contributor

Choose a reason for hiding this comment

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

Note to self: These directories probably shouldn't exist, I should investigate why they show up before this gets merged and I stop seeing them in git status, because then I'll never remember.

Copy link
Member Author

@kennytm kennytm May 21, 2017

Choose a reason for hiding this comment

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

@rkruppe

/src/target/ appears when manually running cargo build inside the crates like /src/libsyntax/.

**/target/rls/debug/ appears when opening any folder in VSCode with kalitaalexey's vscode-rust extension, configured to use RLS.

Both of these are not really caused by rustbuild.

Copy link
Contributor

Choose a reason for hiding this comment

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

I could have sworn I've seen src/target show up without ever running cargo in there. But I just realized that the "Rust Enhanced" Sublime Text plugin runs cargo when saving. So you're probably right that it's not related to the build system.

@Mark-Simulacrum
Copy link
Member

Wait, so we've been needlessly downloading ruby artifacts this whole time? Or at least initializing with them? I am somewhat surprised, but also very happy we noticed.

@kennytm
Copy link
Member Author

kennytm commented May 21, 2017

@Mark-Simulacrum AFAIK nothing is actually downloaded when using the "ruby" language. Travis CI's containers always include Ruby + rvm by default (as well as clang/gcc, nvm, PostgreSQL client, etc), no matter which language we choose. The only difference to us by switching to generic, besides pedantry, is these 10 lines

$ rvm use default
Using /home/travis/.rvm/gems/ruby-2.3.1
...
$ ruby --version
ruby 2.3.1p112 (2016-04-26 revision 54768) [x86_64-linux]
$ rvm --version
rvm 1.27.0 (latest) by Wayne E. Seguin <wayneeseguin@gmail.com>, Michal Papis <mpapis@gmail.com> [https://rvm.io/]
$ bundle --version
Bundler version 1.13.6
$ gem --version
2.5.1

become these two lines

$ bash -c 'echo $BASH_VERSION'
4.3.11(1)-release

(compare:

)

@kennytm
Copy link
Member Author

kennytm commented May 21, 2017

I've implemented the --quiet thing mentioned in #42128 (comment) in 594d855eadebcc885fe1dbb5c4dff0c13d2a3168 0195e3fcaa18043c295e3796db3caf33dce4dd00. This will reduce the number of lines well below 10000. I can revert it if this change makes the logs too shallow :).

Update: Changed to pass in --quiet via ./configure instead of checking environment.

@arielb1
Copy link
Contributor

arielb1 commented May 23, 2017

@brson had just came back from PTO. He'll probably look at this PR soon-ish.

@kennytm kennytm force-pushed the travis-folder branch 2 times, most recently from f1d7684 to 0195e3f Compare May 24, 2017 03:34
@carols10cents
Copy link
Member

ping @brson to get this on your review queue after the holiday!

@alexcrichton alexcrichton assigned alexcrichton and unassigned brson Jun 1, 2017
@alexcrichton
Copy link
Member

Thanks for the PR @kennytm! I'll take a look at the changes here

.gitignore Outdated
@@ -101,3 +101,5 @@ version.ml
version.texi
.cargo
!src/vendor/**
**/target/rls/debug/
Copy link
Member

Choose a reason for hiding this comment

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

I think with rls in the same workspace as everything else wnow (a relatively recent change) this may no longer be necessary?

Copy link
Member Author

@kennytm kennytm Jun 1, 2017

Choose a reason for hiding this comment

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

I just tried and RLS won't even start on VSCode 😂 Manually running rls (rls 0.1.0 (38ca9b7 2017-05-14)) still places the target/rls folder next to Cargo.toml.

$ cd src/bootstrap
$ rls sfjsdjfl
^C
$ ls target
rls/

If RLS respect workspace settings, this line can indeed be removed as it is covered by the next line too.

Update: Removed in 57356b3.

@@ -1,4 +1,4 @@
language: minimal
language: generic
Copy link
Member

Choose a reason for hiding this comment

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

Historically minimal was required to get more disk space on the builders to avoid blowing the disk space limits on the Android builder, but it looks like generic only has 1GB less of space (24 vs 25 GB) and I think we've mitigated this through other means. Getting out that ruby business sounds good!

Copy link
Member

Choose a reason for hiding this comment

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

If this is really the case, that's quite surprising. Is travis really making people specify the wrong language to get more disk space?

/// default. This reduces visibility of unnecessary logs, allowing developers to
/// quickly identify the error.
pub struct OutputFolder<D: Display> {
name: D,
Copy link
Member

Choose a reason for hiding this comment

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

We can probably just make this String,no?

Copy link
Member Author

Choose a reason for hiding this comment

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

Converted to String in e6e5dc0.

impl<D: Display> OutputFolder<D> {
/// Creates a new output folder with the given group name.
pub fn new(name: D) -> OutputFolder<D> {
print!("travis_fold:start:{0}\ntravis_time:start:{0}\r\x1b[0K", name);
Copy link
Member

Choose a reason for hiding this comment

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

Are there docs for this that we can link to?

Copy link
Member

Choose a reason for hiding this comment

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

Also should this flush after printing?

Copy link
Member Author

Choose a reason for hiding this comment

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

docs: Unfortunately I can't find any official doc. The closest official thing would be https://github.com/travis-ci/travis-build/blob/e634cd1c4/lib/travis/build/templates/header.sh#L250-L254

Copy link
Member Author

Choose a reason for hiding this comment

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

flush: We could flush, but it is unnecessary. Travis will not fold until the "end" is seen, so putting a flush at the "end" should be good enough.

Copy link
Member Author

Choose a reason for hiding this comment

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

Added more explanation in e6e5dc0.

src/ci/run.sh Outdated
@@ -28,6 +28,7 @@ RUST_CONFIGURE_ARGS="$RUST_CONFIGURE_ARGS --disable-manage-submodules"
RUST_CONFIGURE_ARGS="$RUST_CONFIGURE_ARGS --enable-locked-deps"
RUST_CONFIGURE_ARGS="$RUST_CONFIGURE_ARGS --enable-cargo-openssl-static"
RUST_CONFIGURE_ARGS="$RUST_CONFIGURE_ARGS --enable-llvm-clean-rebuild"
RUST_CONFIGURE_ARGS="$RUST_CONFIGURE_ARGS --enable-quiet-tests"
Copy link
Member

Choose a reason for hiding this comment

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

Could we hold off on passing this just yet? Non-quiet tests are invaluable for debugging spurious failures.

Perhaps PRs could pass this by default but auto builds wouldn't?

Copy link
Member

Choose a reason for hiding this comment

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

I have this in #42354

Copy link
Member

Choose a reason for hiding this comment

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

Excellent! @kennytm want to just remove this piece from this PR for now then?

Copy link
Member Author

@kennytm kennytm Jun 1, 2017

Choose a reason for hiding this comment

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

@Mark-Simulacrum @alexcrichton Addressed this in c90c87b. But I'll just remove it then 😉

Update: Removed in 6ac0787.

- /src/target -- created when trying to directly `cargo build` on a single
  package.
* Bring back colors on Travis, which was disabled since rust-lang#39036.
  Append --color=always to cargo when running in CI environment.
* Removed `set -x` in the shell scripts. The `retry` function already
  prints which command it is running, add `-x` just add noise to the
  output.
* Support travis_fold/travis_time. Matching pairs of these allow Travis CI
  to collapse the output in between. This greatly cut down the unnecessary
  "successful" output one need to scroll through before finding the failed
  statement.
There is no `minimal` language. Due to travis-ci/travis-ci#4895, it will
fallback to `ruby`, which certainly isn't what we want. `generic` is an
undocumented (travis-ci/docs-travis-ci-com#910) language that serves the
desired purpose.
When `--quiet` is passed to rustbuild, suppress rustdoc test output unless
failure.

Added a `--quiet` flag to `tidy`, which suppresses the features table.

The actual `--quiet` flag is enabled in rust-lang#42354.

Since details of failed tests will still be printed, and the name of slow
tests taking >60 to runtime will also be printed, the debugging difficulty
caused by information loss should be minimal; but it is very worthwhile to
keep the log under 10000 lines on Travis CI so that common errors can be
spotted without reading the raw log.
@alexcrichton
Copy link
Member

Looks fantastic, thanks @kennytm!

@alexcrichton
Copy link
Member

@bors: r+

@bors
Copy link
Contributor

bors commented Jun 1, 2017

📌 Commit 6ac0787 has been approved by alexcrichton

@bors
Copy link
Contributor

bors commented Jun 2, 2017

⌛ Testing commit 6ac0787 with merge d7798c3...

bors added a commit that referenced this pull request Jun 2, 2017
Improve Travis CI log (travis_fold, colors)

Result looks like (https://travis-ci.org/rust-lang/rust/jobs/234614611):

<details><summary>Full page screenshot</summary>

![1-fullpage](https://cloud.githubusercontent.com/assets/103023/26302841/2627c354-3f18-11e7-9299-52a2088639af.jpg)

</details>

---

* Bring back colors on Travis, which was disabled since #39036. Append `--color=always` to cargo when running in CI environment.
* Removed `set -x` from the shell scripts. The `retry` function already prints which command it is running, adding `-x` just adds noise to the output. Interesting information can be manually `echo`ed.
* Support `travis_fold`/`travis_time`. Matching pairs of these allow Travis CI to collapse the output in between. This greatly cut down the unnecessary "successful" output one need to scroll through before finding the failed statement.
* Passed `--quiet` to all tests, so tests not failing will not occupy a line of log, reducing bloat in the report.

Also include some minor changes, like changing the `script` of `.travis.yml` to execute a single-line command, so the log won't write the extremely long multi-line
`The command "if [ "$ALLOW_PR" = "" ] && [ "$TRAVIS_BRANCH" != "auto" ]; then … " exited with 0` at the end.
@bors
Copy link
Contributor

bors commented Jun 2, 2017

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-waiting-on-review Status: Awaiting review from the assignee but also interested parties.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

9 participants