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

fix makefile and log #1714

Merged
merged 3 commits into from
Jul 11, 2023
Merged

Conversation

luky116
Copy link
Collaborator

@luky116 luky116 commented Jul 11, 2023

No description provided.

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
Copy link

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:

  1. Variable Naming: It's generally recommended to use lowercase letters for variable names in Makefiles. For example, consider changing UNAME to uname and UNAME_M to uname_m.

  2. 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").

  3. 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.

@@ -65,8 +66,10 @@ all: build
build: deps
ifeq ($(UNAME), Linux)
Copy link
Contributor

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?

Copy link
Collaborator Author

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
Copy link

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:

  1. Typo: In line 11, ARCH should be assigned the output of uname -m, not "uname -m".
  2. Typo: In line 14, BRANCH should use double backticks (``) instead of single quotes ('') as the sed command requires backticks.
  3. In line 65, there's a typo (arm6411) instead of arm64. Fixing that will ensure the correct architecture is used for building on Linux.
  4. Consider adding error handling for the shell commands. If any of the uname or git 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.

Copy link
Contributor

@yaoyinnan yaoyinnan left a comment

Choose a reason for hiding this comment

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

LGTM

@AlexStocks AlexStocks merged commit a27896d into OpenAtomFoundation:unstable Jul 11, 2023
bigdaronlee163 pushed a commit to bigdaronlee163/pika that referenced this pull request Jun 8, 2024
* fix makefile and log

* fix makefile and log

* fix makefile
cheniujh pushed a commit to cheniujh/pika that referenced this pull request Sep 24, 2024
* fix makefile and log

* fix makefile and log

* fix makefile
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.

3 participants