Skip to content

Conversation

@dnephin
Copy link
Member

@dnephin dnephin commented Sep 7, 2016

Instead of attempting to use one monolithic docker image for all the build tasks we should aim to have images for each logical group of tasks. This PR creates a new Dockerfile for running the cross build.

cc @justincormack - we talked about this earlier this week

Copy link
Member Author

Choose a reason for hiding this comment

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

I suspect a bunch of these aren't necessary, but I haven't gone through each yet.

Does anyone know offhand of any of these are runtime dependencies and aren't required for compiling?

@duglin
Copy link
Contributor

duglin commented Sep 7, 2016

microservices!

@vdemeester
Copy link
Member

/me likes it 👼

@dnephin dnephin force-pushed the separate_cross_build branch from 604670d to a196b3d Compare September 7, 2016 21:09
@aaronlehmann
Copy link

I really like this. This will reduce bloat by so much for people who aren't routinely doing cross-compiles.

@justincormack
Copy link
Contributor

Yes, definitely an improvement. Will try it out shortly.

Dockerfile Outdated
Copy link
Member

Choose a reason for hiding this comment

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

Is is valid to test the existence of /.dockerenv instead of introducing this environment variable?

I'm not sure /.dockerenv will continue to exist in future releases, but many people have been already using that file for checking whether running in Docker:
https://github.com/search?utf8=%E2%9C%93&q=if+-f+%2F.dockerenv&type=Code&ref=searchresults

Copy link
Contributor

Choose a reason for hiding this comment

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

I am inclined to remove .dockerenv when we get introspection, so I wouldnt use it here.

@ehazlett
Copy link
Contributor

ehazlett commented Sep 8, 2016

Looks like janky and experimental are failing with this:

22:20:45 ---> Making bundle: tgz (in bundles/1.13.0-dev/tgz)
22:20:45 error: binary and cross must be run before tgz

@ehazlett ehazlett added the status/failing-ci Indicates that the PR in its current state fails the test suite label Sep 8, 2016
@dnephin
Copy link
Member Author

dnephin commented Sep 8, 2016

I think to make this work with CI we are going to need a separate jenkins build for cross+tgz.

Signed-off-by: Daniel Nephin <dnephin@docker.com>
@vieux
Copy link
Contributor

vieux commented Oct 5, 2016

good idea! needs a rebase @dnephin

@thaJeztah
Copy link
Member

ping @dnephin can you rebase your PR? Or is #27359 meant to replace this?

@dnephin
Copy link
Member Author

dnephin commented Oct 18, 2016

It's blocked on #27359 , but it doesn't replace it

@thaJeztah
Copy link
Member

@dnephin we're looking at this one, but it looks like we should be able to merge this without the other PR, correct?

@dnephin
Copy link
Member Author

dnephin commented Oct 20, 2016

Not really. It breaks janky because we run builds in a single image. We need a way to tell it to use different images for different builds.

Making that change now is difficult because it would break any branches that don't include this change. The Jenkinsfile makes this a much easier operation, because the build config is stored in the branch.

-t ${DOCKER_CROSS_IMAGE} \
-f dockerfiles/Dockerfile.cross dockerfiles/

cross: bundles build-cross ## cross build the binaries for darwin, freebsd and\nwindows
Copy link
Contributor

Choose a reason for hiding this comment

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

stale \n


tgz: build ## build the archives (.zip on windows and .tgz\notherwise) containing the binaries
$(DOCKER_RUN_DOCKER) hack/make.sh dynbinary binary cross tgz
tgz: build cross ## build the archives (.zip on windows and .tgz\notherwise) containing the binaries
Copy link
Contributor

Choose a reason for hiding this comment

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

why there are \n in comments?

@dnephin
Copy link
Member Author

dnephin commented Nov 29, 2016

I opened #28952 to replace this

@dnephin dnephin closed this Nov 29, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

status/failing-ci Indicates that the PR in its current state fails the test suite status/2-code-review

Projects

None yet

Development

Successfully merging this pull request may close these issues.

10 participants