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

Update Makefile build-cli target #46

Closed
wants to merge 4 commits into from

Conversation

anthonywendt
Copy link
Contributor

Updated to have a build-cli target that will automatically build the appropriate binary based on your system and arch.

Updated to have a build-cli target that will automatically build
the appropriate binary based on your system and arch.
@anthonywendt anthonywendt self-assigned this Sep 1, 2023
@anthonywendt
Copy link
Contributor Author

This may not be necessary. Just started playing in this repo. Feel free to ignore.

Copy link
Contributor

@Noxsios Noxsios left a comment

Choose a reason for hiding this comment

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

I saw the above comment after I made this review, but still submitting as if this was a more serious PR (but not requesting changes). As this PR stands, don't think the benefits outweigh the costs of this change. I leave it up to @UncleGedd .

@@ -1,7 +1,38 @@
# SPDX-License-Identifier: Apache-2.0
# SPDX-FileCopyrightText: 2023-Present The UDS Authors

ARCH ?= amd64
# Figure what operating system and arch we are on.
# Set some variables used to build the apporpriate binary
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
# Set some variables used to build the apporpriate binary
# Set some variables used to build the appropriate binary

make build-cli-linux-amd
make build-cli
Copy link
Contributor

Choose a reason for hiding this comment

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

Are there plans to support other arches for UDS CLI releases? As it stands right now only linux/amd64 will be supported.

Copy link
Contributor

Choose a reason for hiding this comment

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

We should at least be supporting darwin/arm64. Honestly we should be supporting linux/amd64, linux/arm64 and darwin/arm64. I have nothing to say about windows. 😏

But it makes no sense to released this tool that we have to develop with without supporting the platforms we develop on (primarily linux/amd64, darwin/arm64).

I can build from source every time but that's kind of annoying. If for some reason we only "support" releases of linux/amd64, then i should still be able to run make build from my m2 mac and be able to build the binary for my correct platform without me having to fuss with it.

Copy link
Collaborator

Choose a reason for hiding this comment

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

+1 haven't read through this PR yet but UDS CLI in its current form should run on Linux or Mac arm/amd architectures

Comment on lines -14 to +48
build-cli-linux-amd: ## Build the CLI for Linux AMD64
CGO_ENABLED=0 GOOS=linux GOARCH=amd64 go build -ldflags="$(BUILD_ARGS)" -o build/uds main.go

build-cli-linux-arm: ## Build the CLI for Linux ARM64
CGO_ENABLED=0 GOOS=linux GOARCH=arm64 go build -ldflags="$(BUILD_ARGS)" -o build/uds-arm main.go

build-cli-mac-intel: ## Build the CLI for Mac Intel
GOOS=darwin GOARCH=amd64 go build -ldflags="$(BUILD_ARGS)" -o build/uds-mac-intel main.go

build-cli-mac-apple: ## Build the CLI for Mac Apple
GOOS=darwin GOARCH=arm64 go build -ldflags="$(BUILD_ARGS)" -o build/uds-mac-apple main.go
build-cli: ## Build the CLI
CGO_ENABLED="${CGO}" GOOS="${UNAME_S}" GOARCH="${ARCH}" go build -ldflags="$(BUILD_ARGS)" -o "${OUTPUT}" main.go
Copy link
Contributor

Choose a reason for hiding this comment

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

If you want to cross compile, you now need to do make build-cli ARCH=arm64 UNAME_S=darwin OUTPUT=build/uds-mac-apple instead of make build-cli-mac-apple. I would reflect this change in the README or somewhere discoverable, as it would be very easy to mix up these args and build a binary for the wrong arch / system that gets outputted to the wrong name.

@anthonywendt
Copy link
Contributor Author

anthonywendt commented Sep 7, 2023

Do you want me to put any more effort into this? If so, I can update based on all these comments. I can also close it if not so its not just sitting here. @UncleGedd

@UncleGedd
Copy link
Collaborator

UncleGedd commented Sep 7, 2023

Do you want me to put any more effort into this? If so, I can update based on all these comments. I can also close it if not so its not just sitting here. @UncleGedd

Really appreciate you showing me how this works after you guys mentioned it last week. Personally I'm not a fan of all the new Makefile logic introduced, while it results in a simpler make cmd, overall I think it's adding more complexity than necessary. Feel free to close for now and we can re-attack sometime in the future if it makes sense

@anthonywendt anthonywendt deleted the dev/anthony/makefile-update branch September 13, 2023 14:40
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