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

ratchet up travis to build stage1 #27028

Merged
merged 1 commit into from Jul 17, 2015

Conversation

Projects
None yet
5 participants
@Gankro
Copy link
Contributor

Gankro commented Jul 14, 2015

This has travis build LLVM and rustc up to stage1, but not run any tests. It seems wasteful to have the ultimate might of travis running on every PR just to check for whitespace errors. This is a pure subset of the bootstrap, so it shouldn't ever spuriously break.

make tidy still runs first, so we still get "fast errors" on bad style. However once make tidy passes, the build will simply keep running to try to make rustc. tidy takes ~3 mins to complete, so if your build runs longer you can be confident we've gone on to build LLVM/rustc. In principle this is configured to use ccache (it shows up in the build logs as uploaded/downloaded), but I found no actual performance changes in using it.

Maybe someone at @travis-ci knows what's up with that.

For reference, here is a successful build with ccache enabled: https://travis-ci.org/Gankro/rust/builds/70821237

and one without: https://travis-ci.org/Gankro/rust/builds/70812814

Builds seem to take about 41mins regardless.

r? @alexcrichton

@oli-obk

This comment has been minimized.

Copy link
Contributor

oli-obk commented Jul 14, 2015

is there any way to turn on deny(warnings) for just this stage1 build, not stage1 in general?

.travis.yml Outdated
script:
- make tidy
- make rustc-stage1 -j8

This comment has been minimized.

@tamird

tamird Jul 14, 2015

Contributor

travis build machines have 2 cores, i think you want -j2 here

This comment has been minimized.

@Gankro

Gankro Jul 14, 2015

Author Contributor

Is there a negative impact of giving too big of a number? In my experience you just don't gain anything.

This comment has been minimized.

@tamird

tamird Jul 14, 2015

Contributor

I'd expect the normal context switching cost. Maybe worth testing?
On Jul 14, 2015 11:19 AM, "Alexis Beingessner" notifications@github.com
wrote:

In .travis.yml
#27028 (comment):

script:

  • make tidy
      • make rustc-stage1 -j8

Is there a negative impact of giving too big of a number? In my experience
you just don't gain anything.


Reply to this email directly or view it on GitHub
https://github.com/rust-lang/rust/pull/27028/files#r34579851.

This comment has been minimized.

@alexcrichton

alexcrichton Jul 14, 2015

Member

Typically too much saturation can lead to a decrease in build times, this may want to be ratcheted down to -j4 instead of 8

.travis.yml Outdated
@@ -2,6 +2,8 @@
# RVM/bundler/ruby and whatnot. Right now 'rust' as a language actually
# downloads a rust/cargo snapshot, which we don't really want for building rust.
language: c
compiler: /usr/bin/gcc-4.7

This comment has been minimized.

@alexcrichton

alexcrichton Jul 14, 2015

Member

How come this was necessary? Does it just set the CC environment variable?

This comment has been minimized.

@Gankro

Gankro Jul 14, 2015

Author Contributor

Yeah, if we don't set this it will inject export CC = gcc after our matrix is injected.

This comment has been minimized.

@alexcrichton

alexcrichton Jul 14, 2015

Member

Could this set the CC env var explicitly? I think it better connects the meaning of setting this option.

This comment has been minimized.

@Gankro

Gankro Jul 14, 2015

Author Contributor

@alexcrichton I'm not sure what you mean; that's exactly what this does.

This comment has been minimized.

@alexcrichton

alexcrichton Jul 14, 2015

Member

Right but the intention is not clear. This is setting a pretty arbitrary key in this configuration file, "compiler", which has no connection to the CC environment variable and the only reason we want to do this is to set the CC environment variable. I'd prefer to explicitly do so in the env array.

This comment has been minimized.

@Gankro

Gankro Jul 14, 2015

Author Contributor

The problem is that compiler gets injected after the environment variables get set:

https://travis-ci.org/rust-lang/rust/builds/70970991#L228-L231

This comment has been minimized.

@tamird

tamird Jul 14, 2015

Contributor

another nit: can the treatment of CC and CXX be identical? might require setting the language to c++

This comment has been minimized.

@Gankro

Gankro Jul 14, 2015

Author Contributor

Because we have to hardcode the path to the specific version (travis doesn't support updating default versions, and it requires sudo to do manually), neither C nor C++ will give us the desired behaviour. That is, C only sets CC and C++ does:

For each row, the Travis CI C++ builder will export the CXX env variable to point to either g++ or clang++ and CC to either gcc or clang.

This comment has been minimized.

@alexcrichton

alexcrichton Jul 15, 2015

Member

The problem is that compiler gets injected after the environment variables get set:

I'm.. not sure I understand why this is a problem. What's the failure mode if you move CC next to CXX below?

@alexcrichton

This comment has been minimized.

Copy link
Member

alexcrichton commented Jul 14, 2015

I'm also somewhat dubious that ccache is being used, and it would probably be very nice to enable it. I think a 42 minute build with ccache enabled is definitely indicative of something going wrong. The logs for this PR show a failure to upload an archive about the cache, so maybe that helps diagnose?

@Gankro

This comment has been minimized.

Copy link
Contributor Author

Gankro commented Jul 14, 2015

Yeah sometimes it just fails. The logs I linked at the top of the PR show a completely successful load/save with no difference.

@alexcrichton

This comment has been minimized.

Copy link
Member

alexcrichton commented Jul 14, 2015

The logs I linked at the top of the PR show a completely successful load/save with no difference.

This worries me that we may not be actually using ccache at all. If it's correctly saving/restoring and actually using ccache the build times should be a lot lower...

Perhaps try adding ccache -s (show stats for ccache) at the end of the travis build to see if they're all hits or misses?

@Gankro

This comment has been minimized.

Copy link
Contributor Author

Gankro commented Jul 14, 2015

I ran ccache -s on the last run and can confirm ccache wasn't even touched. Also, upon closer inspection it looks like the cache is PR-local regardless:

fetching PR.27028/cache--compiler-usrbingcc-4.7.tgz

So every PR will take 40 minutes regardless. On subsequent pushes it can potentially go faster, though this is clearly not happening.

@Gankro Gankro force-pushed the Gankro:travis branch from 96200d5 to 0448a8f Jul 15, 2015

@Gankro

This comment has been minimized.

Copy link
Contributor Author

Gankro commented Jul 15, 2015

It works!!!!!!!!!!!!

LLVM is built before travis makes it to 6 minutes now. I had to hack up configure a bit to properly forward the ccache argument even if CC and CXX are defined (for whatever reason it was specifically short-circuiting that case to not use ccache).

In addition, I was mistaken about how the cache works. The docs seem to suggest that any PR posted against master will try to grab master's cache on the first build.

.travis.yml Outdated
- ./configure --llvm-root=path/to/nowhere
- ./configure --enable-ccache
- cat config.mk
- which ccache

This comment has been minimized.

@alexcrichton

alexcrichton Jul 15, 2015

Member

This, cat config.mk, and ccache -s can probably be removed

@Gankro Gankro force-pushed the Gankro:travis branch from 43c03ec to 8475012 Jul 15, 2015

@Gankro

This comment has been minimized.

Copy link
Contributor Author

Gankro commented Jul 15, 2015

Looks like bash is a valid nullary language. Everything seems good to go.

@tamird

This comment has been minimized.

Copy link
Contributor

tamird commented Jul 15, 2015

This is a huge win

@Gankro Gankro force-pushed the Gankro:travis branch from 8475012 to 40da53d Jul 15, 2015

@Gankro

This comment has been minimized.

Copy link
Contributor Author

Gankro commented Jul 15, 2015

Setting language: bash prevents cache: ccache from working. Need to do things the awkward lang: C way.

Ratchet up travis to build stage1 and our own LLVM.
Tidy is still run first for failing fast on the easy stuff.

To accomplish this we have travis actually persist ccache across builds. This
has LLVM built within 6 minutes, and all of stage1 built within 18.
Caching should work on fresh PRs (cache acquired from the master branch).
Because all we persist is ccache, there is minimal danger of persisting corrupt
build state.

I had to mangle `configure` a bit to make --enable-ccache work when custom
compilers are provide via CC and CXX.

@Gankro Gankro force-pushed the Gankro:travis branch from 40da53d to e8a0328 Jul 15, 2015

language: c
compiler: /usr/bin/gcc-4.7

This comment has been minimized.

@tamird

tamird Jul 15, 2015

Contributor

as an alternative to this, is travis' clang recent enough to compile llvm?

This comment has been minimized.

@Gankro

Gankro Jul 15, 2015

Author Contributor

I am a big fan of trying to make this as vanilla as possible. Historically anything really custom has caused travis to spuriously break (e.g. check-stage-1)

This comment has been minimized.

@tamird

tamird Jul 15, 2015

Contributor

I don't follow. I only meant to ask: would the following work (out of the box)?

language: c++
compiler: clang

This comment has been minimized.

@Gankro

Gankro Jul 15, 2015

Author Contributor

Ah, I had been scared away from trying clang due to existence of --enable-clang flags and previous nasty hacks surrounding clang and travis in the past. However it's worth a try.

This comment has been minimized.

@tamird

tamird Jul 15, 2015

Contributor

😿 looks like it's clang 3.4 which also doesn't support c++11. thanks for trying it.

@Gankro Gankro force-pushed the Gankro:travis branch from 30c8a38 to e8a0328 Jul 15, 2015

@alexcrichton

This comment has been minimized.

Copy link
Member

alexcrichton commented Jul 16, 2015

@alexcrichton

This comment has been minimized.

Copy link
Member

alexcrichton commented Jul 16, 2015

@bors: rollup

Manishearth added a commit to Manishearth/rust that referenced this pull request Jul 16, 2015

Rollup merge of rust-lang#27028 - Gankro:travis, r=alexcrichton
 This has travis build LLVM and rustc up to stage1, but not run any tests. It seems wasteful to have the ultimate might of travis running on every PR just to check for whitespace errors. This is a pure subset of the bootstrap, so it shouldn't ever spuriously break.

`make tidy` still runs first, so we still get \"fast errors\" on bad style. However once make tidy passes, the build will simply keep running to try to make rustc. `tidy` takes ~3 mins to complete, so if your build runs longer you can be confident we've gone on to build LLVM/rustc. In principle this is configured to use ccache (it shows up in the build logs as uploaded/downloaded), but I found no actual performance changes in using it.

Maybe someone at @travis-ci knows what's up with that.

For reference, here is a successful build with ccache enabled: https://travis-ci.org/Gankro/rust/builds/70821237

and one without: https://travis-ci.org/Gankro/rust/builds/70812814

Builds seem to take about 41mins regardless.

r? @alexcrichton

@bors bors merged commit e8a0328 into rust-lang:master Jul 17, 2015

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details

@frewsxcv frewsxcv referenced this pull request Aug 4, 2015

Closed

Build servo on travis #6962

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.