Skip to content

fix makefile and log - #1714

Merged
AlexStocks merged 3 commits into
OpenAtomFoundation:unstablefrom
luky116:fixExporter
Jul 11, 2023
Merged

fix makefile and log#1714
AlexStocks merged 3 commits into
OpenAtomFoundation:unstablefrom
luky116:fixExporter

Conversation

@luky116

@luky116 luky116 commented Jul 11, 2023

Copy link
Copy Markdown
Collaborator

No description provided.

CGO_ENABLED=0 GOOS=darwin GOARCH=amd64 go build -o bin/$(PROJNAME)
endif

deps: generateVer

Copy link
Copy Markdown

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.

Comment thread tools/pika_exporter/Makefile Outdated
@@ -65,8 +66,10 @@ all: build
build: deps
ifeq ($(UNAME), Linux)

Copy link
Copy Markdown
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
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

done

endif
endif

deps: generateVer

Copy link
Copy Markdown

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.

@yaoyinnan yaoyinnan left a comment

Copy link
Copy Markdown
Contributor

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