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

Fix Windows builds #256

Merged
merged 4 commits into from
Oct 3, 2016
Merged

Fix Windows builds #256

merged 4 commits into from
Oct 3, 2016

Conversation

andreastt
Copy link
Contributor

@andreastt andreastt commented Oct 3, 2016

Fixes #246.


This change is Reviewable

We were using Rust beta as some features we were relying on were only
available there, most notably cross-compilation.  Now that beta has been
rolled into stable, we are able to rely on the stable channel for most
of our targets.
As of 2016-09-21 Travis started serving Ubuntu precise containers when
trusty was requested. This caused the x86_64-pc-windows-gnu build to
break, but we only noticed when building on custom Travis branches
because the geckodriver master branch used a cached Rust installation.

This change reverts the 64-bit Windows builds to run outside of the
container infrastructure, in which we can guarantee we are served Ubuntu
trusty with a sufficiently modern gcc to cross-compile.
gcc-mingw-w64 is implied by gcc-mingw-w64-x86-64.
@andreastt andreastt added this to the 0.11 milestone Oct 3, 2016
@andreastt
Copy link
Contributor Author

I think a future improvement to this would be to provide our own Dockerfile descriptions of what we want to build as they are no slower than bare metal builds. This would allow us to move some of the dependency installation out of .travis.yml and build.sh, into the Dockerfiles.

This would particularly benefit one-off components such as the custom mingw thing we need to link for 32-bit Windows.

@andreastt
Copy link
Contributor Author

Anyway, despite the above, I think this solves our immediate problems in no worse way than before.

r? @jgraham

@jgraham
Copy link
Member

jgraham commented Oct 3, 2016

Reviewed 1 of 1 files at r1, 1 of 1 files at r2, 1 of 1 files at r3, 4 of 4 files at r4.
Review status: all files reviewed at latest revision, 1 unresolved discussion.


build.sh, line 28 at r4 (raw file):

mingw_i686_install() {
    curl https://static.rust-lang.org/dist/rust-mingw-nightly-i686-pc-windows-gnu.tar.gz \

It's not really clear what this is doing, or why it's only doing it if !DOCKER && i686 (for example it seems like linux32 would not want it, if we added that).


Comments from Reviewable

@andreastt
Copy link
Contributor Author

Review status: all files reviewed at latest revision, 1 unresolved discussion.


build.sh, line 28 at r4 (raw file):

Previously, jgraham wrote…

It's not really clear what this is doing, or why it's only doing it if !DOCKER && i686 (for example it seems like linux32 would not want it, if we added that).

It’s doing it _if_ Docker is being used (`! -z` means _if not undefined_) _and_ i686 is being installed.

But it’s basically what alexcrichton is talking about as the ‘custom mingw component’ here: #138 (comment)


Comments from Reviewable

@jgraham
Copy link
Member

jgraham commented Oct 3, 2016

Review status: all files reviewed at latest revision, 1 unresolved discussion.


build.sh, line 28 at r4 (raw file):

Previously, andreastt (Andreas Tolfsen) wrote…

It’s doing it if Docker is being used (! -z means if not undefined) and i686 is being installed.

But it’s basically what alexcrichton is talking about as the ‘custom mingw component’ here: #138 (comment)

OK, but it seems it should check more of the target to include Windows, and a comment is required.

Comments from Reviewable

By setting up our own container we can share the build.sh file (almost)
fully between containerised- and bare metal builds.  The apt dependencies
needed are listed in i686-trusty/Dockerfile.

We build i686-pc-windows-gnu in a custom container because the Ubuntu
trusty VM provided by Travis does not have a sufficiently modern gcc
compiler.
@andreastt
Copy link
Contributor Author

Review status: 3 of 4 files reviewed at latest revision, 1 unresolved discussion.


build.sh, line 28 at r4 (raw file):

Previously, jgraham wrote…

OK, but it seems it should check more of the target to include Windows, and a comment is required.

Alright, sorted.

As I said in an earlier comment on the PR, I don’t think this design is optimal. I think we should consider building more (if not everything) in containers and define dependencies, such as the mingw component, in Dockerfiles.


Comments from Reviewable

@jgraham
Copy link
Member

jgraham commented Oct 3, 2016

Reviewed 1 of 1 files at r5.
Review status: all files reviewed at latest revision, all discussions resolved.


Comments from Reviewable

@jgraham jgraham merged commit b2274b8 into mozilla:master Oct 3, 2016
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.

2 participants