-
Notifications
You must be signed in to change notification settings - Fork 1.2k
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
fix makefile and log #1714
fix makefile and log #1714
Conversation
CGO_ENABLED=0 GOOS=darwin GOARCH=arm64 go build -o bin/$(PROJNAME) | ||
else ifeq ($(UNAME_M), arm64) | ||
CGO_ENABLED=0 GOOS=darwin GOARCH=amd64 go build -o bin/$(PROJNAME) | ||
endif | ||
|
||
deps: generateVer |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The code patch you provided seems to handle the build process based on the operating system and architecture. Here's a brief review with some suggestions:
-
Variable Naming: It's generally recommended to use lowercase letters for variable names in Makefiles. For example, consider changing
UNAME
touname
andUNAME_M
touname_m
. -
Comment Clarity: The comment indicating the command for Linux in the
build
target can be clarified by mentioning that it's specifically for x86_64 architecture ("Linux x86_64"). -
Error Handling: It's a good practice to include error handling in your code. In this case, if the operating system is neither Linux nor macOS, the code doesn't handle the situation. You might want to add an error message or fallback mechanism when the conditions are not met.
Apart from these suggestions, the code patch looks fine and should work as intended.
tools/pika_exporter/Makefile
Outdated
@@ -65,8 +66,10 @@ all: build | |||
build: deps | |||
ifeq ($(UNAME), Linux) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do you need to consider the situation of Linux and arm architecture?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done
CGO_ENABLED=0 GOOS=darwin GOARCH=amd64 go build -o bin/$(PROJNAME) | ||
else ifeq ($(ARCH), arm64) | ||
CGO_ENABLED=0 GOOS=darwin GOARCH=arm64 go build -o bin/$(PROJNAME) | ||
endif | ||
endif | ||
|
||
deps: generateVer |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The code patch includes a build target in a Makefile to handle cross-compilation for different operating systems and architectures. Here are some bug risks and improvement suggestions:
- Typo: In line 11,
ARCH
should be assigned the output ofuname -m
, not"uname -m"
. - Typo: In line 14,
BRANCH
should use double backticks (``) instead of single quotes ('') as the sed command requires backticks. - In line 65, there's a typo (
arm6411
) instead ofarm64
. Fixing that will ensure the correct architecture is used for building on Linux. - Consider adding error handling for the shell commands. If any of the
uname
orgit branch
commands fail, it would be good to display an error message.
Apart from these minor issues, the code looks fine in terms of handling different operating systems (Linux and Darwin) and architectures (x86_64 and arm64). It sets the appropriate values for GOOS
and GOARCH
environment variables and executes the go build
command accordingly.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
* fix makefile and log * fix makefile and log * fix makefile
* fix makefile and log * fix makefile and log * fix makefile
No description provided.