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

Makefile: go build instead of install (solves cross compile issues) #2520

Merged
merged 1 commit into from
May 9, 2016

Conversation

karalabe
Copy link
Member

@karalabe karalabe commented May 6, 2016

Go doesn't like the GOBIN env var being set simultaneously with cross-go install-ing. This PR works around that issue by using go build -i instead.

@robotally
Copy link

robotally commented May 6, 2016

Vote Count Reviewers
👍 1 @caktux
👎 0

Updated: Sun May 8 14:36:10 UTC 2016

@caktux
Copy link
Contributor

caktux commented May 6, 2016

👍

@fjl
Copy link
Contributor

fjl commented May 6, 2016

I don't like this change because it disables incremental builds for everyone. That feels like too high a price to pay for a small workaround.

Wouldn't it work to

  • not set GOBIN in build/env.sh
  • add cp build/_workspace/bin/* build/bin as a step in the Makefile

@karalabe
Copy link
Member Author

karalabe commented May 6, 2016

Why would it disable incremental builds? Note: go build -i. Emphasis on
the -i.

On May 6, 2016 23:24, "Felix Lange" [email protected] wrote:

I don't like this change because it disables incremental builds for
everyone. That feels like too high a price to pay for a small workaround.

Wouldn't it work to

  • not set GOBIN in build/env.sh
  • add cp build/_workspace/bin/* build/bin as a step in the Makefile


You are receiving this because you authored the thread.
Reply to this email directly or view it on GitHub
#2520 (comment)

@fjl
Copy link
Contributor

fjl commented May 6, 2016

Ugh, didn't see the -i. Sorry.

@fjl
Copy link
Contributor

fjl commented May 6, 2016

I have tried it now and the disadvantage is that all commands need to be re-linked individually even if there are no source changes.

@karalabe
Copy link
Member Author

karalabe commented May 8, 2016

Yup, though I think that's a smaller price to pay for now. Since this is blocking the release, I'd suggest we get this in now and perhaps fix it another way when we figure one out. I'm open to suggestion though if you don't like it.

@fjl
Copy link
Contributor

fjl commented May 8, 2016

Yes, it's ok to get this onto master.

@karalabe
Copy link
Member Author

karalabe commented May 8, 2016

@fjl So, thumbs up? Mergable? :D

@karalabe
Copy link
Member Author

karalabe commented May 8, 2016

@caktux Btw, thanks a lot for the quick fixes to the build servers!

@karalabe karalabe merged commit 8aa4597 into ethereum:develop May 9, 2016
@obscuren obscuren removed the review label May 9, 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.

5 participants