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

Make unconteinerized build work on OSX #1028

Merged
merged 10 commits into from
Mar 15, 2016
Merged

Make unconteinerized build work on OSX #1028

merged 10 commits into from
Mar 15, 2016

Conversation

2opremio
Copy link
Contributor

@davkal PTAL

@tomwilkie
Copy link
Contributor

I don't understand how this even works... surely when entering the container it tries to build something that it doesn't know about?

@2opremio
Copy link
Contributor Author

What don't you understand? It simply names the binary based on the host
architecture/OS (for which it's also compiled).

It fixes trying to use a binary from the wrong architecture when mixing
builds in and out of the container (e.g. in an OSX host) which was being
experienced by @davkal.

From a conversation in Slack it seems @davkal may be having other problems
too, let's see if this helps.

On Wednesday, February 24, 2016, Tom Wilkie [email protected]
wrote:

I don't understand how this even works... surely when entering the
container it tries to build something that it doesn't know about?


Reply to this email directly or view it on GitHub
#1028 (comment).

@2opremio 2opremio force-pushed the codecgen-multiarch branch 2 times, most recently from db62f8c to 6b17a23 Compare February 25, 2016 09:48
@2opremio 2opremio changed the title Create codecgen binary based on the host's architecture Create codecgen binary based on the host's architecture (and invoke go uniformly) Feb 25, 2016
@davkal
Copy link
Contributor

davkal commented Feb 25, 2016

Builds in container, and on Darwin. LGTM

@tomwilkie
Copy link
Contributor

Pls hold off on this until I've had a chance to look at it, I have some concerns.

@tomwilkie
Copy link
Contributor

I get this when trying to build using docker-machine on my mac:

docker run --rm -ti \
        -v /Users/twilkie/Documents/src/github.com/weaveworks/scope:/go/src/github.com/weaveworks/scope \
        -v /Users/twilkie/Documents/src/github.com/weaveworks/scope/.pkg:/go/pkg \
        --net=host \
        -e GOARCH -e GOOS -e CIRCLECI -e CIRCLE_BUILD_NUM -e CIRCLE_NODE_TOTAL \
        -e CIRCLE_NODE_INDEX -e COVERDIR -e SLOW \
        weaveworks/scope-backend-build SCOPE_VERSION=18964aa GO_BUILD_INSTALL_DEPS=-i vendor/github.com/ugorji/go/codec/codecgen/bin/codecgen_darwin_amd64
make: Entering directory '/go/src/github.com/weaveworks/scope'
make: *** No rule to make target 'vendor/github.com/ugorji/go/codec/codecgen/bin/codecgen_darwin_amd64'.  Stop.
make: Leaving directory '/go/src/github.com/weaveworks/scope'
make: *** [vendor/github.com/ugorji/go/codec/codecgen/bin/codecgen_darwin_amd64] Error 2

@2opremio
Copy link
Contributor Author

@tomwilkie please take another look. I fixed it by moving all the codecgen dependencies to the non-containerized part (which is here they should be since codecgen depends on the host of whatever runs the non-containerized part)

@tomwilkie
Copy link
Contributor

This now requires I have go installed, which I thought we'd decided against...

@2opremio
Copy link
Contributor Author

This now requires I have go installed, which I thought we'd decided against...

No it doesn't. Have you actually tested it?

As I already said, the makefile variables are used lazily unless assigned with :=, although I can happily define the variable in the non-containerized-block if that's confusing.

vagrant@vagrant-ubuntu-vivid-64:~/scope$ echo $PATH
/home/vagrant/bin:/usr/local/go/bin:/home/vagrant/bin:/usr/local/sbin:/usr/local/bin:/usr/sbin:/usr/bin:/sbin:/bin:/usr/games:/usr/local/games
vagrant@vagrant-ubuntu-vivid-64:~/scope$ export PATH=/home/vagrant/bin:/usr/local/sbin:/usr/local/bin:/usr/sbin:/usr/bin:/sbin:/bin:/usr/games:/usr/local/games
vagrant@vagrant-ubuntu-vivid-64:~/scope$ which go
vagrant@vagrant-ubuntu-vivid-64:~/scope$ make

@davkal
Copy link
Contributor

davkal commented Feb 25, 2016

@2opremio how did you test the darwin build? Which command do you run?

@2opremio
Copy link
Contributor Author

@davkal I have been relying on you to test the build in Darwin (I use a Linux VM). The purpose of this PR is to make you comfortable with the build, hence the PTAL :)

@@ -1,5 +1,4 @@
FROM golang:1.5.3
ENV GO15VENDOREXPERIMENT 1

This comment was marked as abuse.

This comment was marked as abuse.

@tomwilkie
Copy link
Contributor

This command used to work make report/report.codecgen.go (on my mac, building in a container) but now does not.

Also, if I change the build container the .codecgen.go files are not rebuilt and I kinda think they should be.

In general it makes me uncomfortable to have different dependancy trees inside the container to outside.

@tomwilkie
Copy link
Contributor

Note both of these are minor issues. What precisely is the problem we're trying to solve here?

@tomwilkie
Copy link
Contributor

I cant think of a nicer way of doing this, so assuming we still want to fix the build-in-container-then-build-on-a-mac-without-a-make-clean-inbetween problem, I'm LGTM.

@tomwilkie tomwilkie removed their assignment Mar 8, 2016
@2opremio
Copy link
Contributor Author

2opremio commented Mar 8, 2016

@davkal / @foot You would still like to have this, right?

@foot
Copy link
Contributor

foot commented Mar 8, 2016

Yes please, easier it is to build in and out of container the better!

@2opremio
Copy link
Contributor Author

2opremio commented Mar 8, 2016

Yes please, easier it is to build in and out of container the better!

Alright, then I will rebase the changes and ask you to take another look

@2opremio 2opremio force-pushed the codecgen-multiarch branch from 904d672 to 7d84993 Compare March 8, 2016 17:01
@2opremio
Copy link
Contributor Author

2opremio commented Mar 8, 2016

Rebased.

@foot Can you please test it so that we can merge it? (Ideally before it bit-rots again! :) )

@foot foot force-pushed the codecgen-multiarch branch from 7d84993 to aead6d3 Compare March 10, 2016 10:24
@foot foot force-pushed the codecgen-multiarch branch from aead6d3 to d7e9d13 Compare March 10, 2016 10:25
@foot
Copy link
Contributor

foot commented Mar 10, 2016

env -u doesn't work on osx! grrr.
Linux: env -u GOARCH -u GOOS $(GO)
Darwin equiv: unset GOARCH GOOS; $(GO)

  • Made a quick patch for the above.
  • I also made a mess when rebasing onto master, i think i fixed it though, look okay?
  • I have a new file now: vendor/github.com/ugorji/go/codec/codecgen/codecgen, should this be gitignored?

@foot
Copy link
Contributor

foot commented Mar 10, 2016

But otherwise it works nicely!

@foot
Copy link
Contributor

foot commented Mar 10, 2016

Rebased onto master.

@foot
Copy link
Contributor

foot commented Mar 10, 2016

But circle is giving a funny permission error now so maybe i broke something..

@2opremio
Copy link
Contributor Author

I have a new file now: vendor/github.com/ugorji/go/codec/codecgen/codecgen, should this be gitignored?

That's because we now build vendor/github.com/ugorji/go/codec/codecgen/codecgen_OS_ARCH instead. Just remove the file manually (you will only need to do this once).

* Get rid of GO15VENDOREXPERIMENT since it's enabled by default in Go >=1.6,
  just use go 1.6 if you want to build outside the container.
* Stop using unset/env -u, just avoid adding env variables where needed.
* Pass build flags consistently to codecgen.
* Let the experimental/ directory be.
@2opremio
Copy link
Contributor Author

I think we were overcomplicating it. My last commit makes it much simpler (although it will only work when ugorji/go#148 is merged and we bump the dependency).

  • Get rid of GO15VENDOREXPERIMENT since it's enabled by default in Go >=1.6,
    just use go 1.6 if you want to build outside the container.
  • Stop using unset/env -u, just avoid adding env variables where needed.
  • Pass build flags consistently to codecgen.
  • Let the experimental/ directory be.

Alfonso Acosta added 2 commits March 15, 2016 10:14
To be able to get rid of GO15VENDOREXPERIMENT ugorji/go#147
@2opremio 2opremio changed the title Create codecgen binary based on the host's architecture (and invoke go uniformly) Make unconteinerized work on OSX Mar 15, 2016
@2opremio 2opremio changed the title Make unconteinerized work on OSX Make unconteinerized build work on OSX Mar 15, 2016
@foot
Copy link
Contributor

foot commented Mar 15, 2016

LGTM for me!

2opremio pushed a commit that referenced this pull request Mar 15, 2016
Make unconteinerized build work on OSX
@2opremio 2opremio merged commit fd37851 into master Mar 15, 2016
@2opremio 2opremio deleted the codecgen-multiarch branch March 15, 2016 16:41
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.

4 participants