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

Adding support for multi-architecture images and binaries #184

Merged
merged 2 commits into from
May 29, 2020
Merged

Adding support for multi-architecture images and binaries #184

merged 2 commits into from
May 29, 2020

Conversation

xunholy
Copy link
Contributor

@xunholy xunholy commented Mar 30, 2020

Fixes:

Note: Should also consider consolidating the following pull request if you wish to support releasing the binary.

@unicomp21
Copy link

Will this work on the newly released m6g/graviton2 processors from AWS as well?

@xunholy
Copy link
Contributor Author

xunholy commented May 27, 2020

Bump**
@sssilver would you know who is the best person to review this pull request?

@sssilver
Copy link
Contributor

@xunholy Hi! Sorry this took a while. We'll get this reviewed in a day or so and get it merged! Thank you so much for your contribution~

@sssilver sssilver requested a review from acmacalister May 27, 2020 04:03
@xunholy
Copy link
Contributor Author

xunholy commented May 27, 2020

Thanks @sssilver & @acmacalister - I just wasn't sure what the process was to have this reviewed. Appreciate your time 👍

Copy link

@Michael9127 Michael9127 left a comment

Choose a reason for hiding this comment

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

It seems that this doesnt support windows builds. GOOS=windows fails.

Also, could you please squash these commits into one.

Copy link
Contributor

@acmacalister acmacalister left a comment

Choose a reason for hiding this comment

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

@xunholy looks good, thanks for your PR! As @Michael9127 noted, can we add Windows as build arch?

Allow Dockerfile --build-args to override GOOS and GOARCH defaults

Support building multi architecture binaries

remove default OS and ARCH to avoid tag confusion when compiling image through Makefile

Tag image with corrosponding OS and ARCH build variables

updating Makefile

Signed-off-by: Michael Fornaro <[email protected]>
@xunholy
Copy link
Contributor Author

xunholy commented May 28, 2020

I've squashed commits as requested, usually, I find it easier to just squash and merge but I'm easy either way.

@xunholy
Copy link
Contributor Author

xunholy commented May 29, 2020

Just used the logic TARGET_OS=windows make cloudflared which works when building until it hits a failure for the following reason:

cmd/cloudflared/windows_service.go:19:2: errors redeclared as imported package name
	previous declaration at cmd/cloudflared/windows_service.go:16:2
make: *** [cloudflared] Error 2

Not sure when this was last tested however, I could see "github.com/pkg/errors" was being imported twice.

Once I removed the duplicate windows build works fine. @Michael9127 @acmacalister

@acmacalister
Copy link
Contributor

Thanks @xunholy. Surprised it has a build error, as we distribute Windows builds on every release

@xunholy
Copy link
Contributor Author

xunholy commented May 29, 2020

No worries, using the command I mentioned and with the small fix I can build windows fine with these changes.

Please let me know if there is anything else you'd like from my end.

@sssilver sssilver merged commit be0514c into cloudflare:master May 29, 2020
sssilver pushed a commit that referenced this pull request Feb 1, 2021
* Allow Dockerfile --build-args to override GOOS and GOARCH defaults

Allow Dockerfile --build-args to override GOOS and GOARCH defaults

Support building multi architecture binaries

remove default OS and ARCH to avoid tag confusion when compiling image through Makefile

Tag image with corrosponding OS and ARCH build variables

updating Makefile

Signed-off-by: Michael Fornaro <[email protected]>

* remove duplicate import on windows_service.go

Signed-off-by: Michael Fornaro <[email protected]>
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