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(make): improve error handling and clean target logic in Makefile #713

Merged
merged 1 commit into from
Jan 1, 2025

Conversation

cfc4n
Copy link
Member

@cfc4n cfc4n commented Jan 1, 2025

  1. Fix clean target to properly handle libpcap Makefile existence check
  2. Use if statement instead of && operator for better control flow
  3. Avoid executing unnecessary commands when libpcap is not present

This change makes the build process more robust and prevents false error reports during cleanup operations.

1. Fix clean target to properly handle libpcap Makefile existence check
2. Use if statement instead of && operator for better control flow
3. Avoid executing unnecessary commands when libpcap is not present

This change makes the build process more robust and prevents false error
reports during cleanup operations.
@cfc4n cfc4n added the improve label Jan 1, 2025
@dosubot dosubot bot added the fix bug fix PR label Jan 1, 2025
@cfc4n
Copy link
Member Author

cfc4n commented Jan 1, 2025

/improve

Copy link

github-actions bot commented Jan 1, 2025

PR Code Suggestions ✨

CategorySuggestion                                                                                                                                    Score
Possible issue
Ensure that Golang version variables are defined before performing version checks

Add a check to ensure that GO_VERSION_MAJ and GO_VERSION_MIN are defined before
comparing their values to avoid potential errors.

functions.mk [29-32]

-if [ ${GO_VERSION_MIN} -lt 22 ]; then
+if [ -z "${GO_VERSION_MAJ}" ] || [ -z "${GO_VERSION_MIN}" ]; then
+    echo "Golang version variables are not set"
+    exit 1
+elif [ ${GO_VERSION_MIN} -lt 22 ]; then
     echo -n "you MUST use golang 1.22 or newer, "
     echo "your current golang version is ${GO_VERSION}"
     exit 1
 fi
Suggestion importance[1-10]: 9

Why: This suggestion is highly relevant as it prevents potential errors by ensuring that GO_VERSION_MAJ and GO_VERSION_MIN are defined before performing comparisons. This improves the reliability of the version check logic.

9
Add checks for undefined variables in the $(CMD_CLANG) commands to prevent build errors

Ensure that the $(CMD_CLANG) commands in the $(KERN_OBJECTS) target handle cases
where required variables like LINUX_ARCH or EXTRA_CFLAGS are undefined to prevent
build failures.

Makefile [122-127]

+if [ -z "$(LINUX_ARCH)" ] || [ -z "$(EXTRA_CFLAGS)" ]; then \
+    echo "Required variables for clang command are not set"; \
+    exit 1; \
+fi; \
 $(CMD_CLANG) -D__TARGET_ARCH_$(LINUX_ARCH) \
     $(EXTRA_CFLAGS) \
     $(BPFHEADER) \
     -target bpfel -c $< -o $(subst kern/,user/bytecode/,$(subst .o,_core.o,$@)) \
     -fno-ident -fdebug-compilation-dir . -g -D__BPF_TARGET_MISSING="GCC error \"The eBPF is using target specific macros, please provide -target\"" \
     -MD -MP || exit 1
Suggestion importance[1-10]: 9

Why: The suggestion adds a safeguard to check for undefined variables like LINUX_ARCH and EXTRA_CFLAGS before executing the $(CMD_CLANG) command. This is crucial for preventing build failures and ensures the build process is more robust.

9
Add default values for undefined variables in the apt-get install command to prevent execution errors

Ensure that the sudo apt-get -y install command handles cases where CLANG_NUM or
CROSS_COMPILE_DEB are undefined to prevent potential errors during execution.

builder/init_env.sh [72-74]

-sudo apt-get -y install build-essential pkgconf libelf-dev llvm${CLANG_NUM} \
-    clang${CLANG_NUM} linux-tools-common linux-tools-generic ${CROSS_COMPILE_DEB} \
+sudo apt-get -y install build-essential pkgconf libelf-dev llvm${CLANG_NUM:-default} \
+    clang${CLANG_NUM:-default} linux-tools-common linux-tools-generic ${CROSS_COMPILE_DEB:-default} \
     libssl-dev flex bison bc linux-source || { echo "apt-get install failed"; exit 1; }
Suggestion importance[1-10]: 8

Why: The suggestion addresses a potential issue where undefined variables like CLANG_NUM or CROSS_COMPILE_DEB could cause the apt-get install command to fail. Adding default values ensures robustness and prevents execution errors during the installation process.

8
General
Improve error handling for the make command in the $(TARGET_LIBPCAP) target to provide clear feedback on failure

Add error handling for the make command in the $(TARGET_LIBPCAP) target to ensure
the build process halts on failure.

Makefile [203]

-CC=$(CMD_CC_PREFIX)$(CMD_CC) AR=$(CMD_AR_PREFIX)$(CMD_AR) make || exit 1
+CC=$(CMD_CC_PREFIX)$(CMD_CC) AR=$(CMD_AR_PREFIX)$(CMD_AR) make || { echo "libpcap build failed"; exit 1; }
Suggestion importance[1-10]: 7

Why: The suggestion enhances error handling by providing a clear error message if the make command fails. This improves debugging and ensures the build process halts appropriately on failure.

7

@cfc4n
Copy link
Member Author

cfc4n commented Jan 1, 2025

/review

Copy link

github-actions bot commented Jan 1, 2025

PR Reviewer Guide 🔍

Here are some key observations to aid the review process:

⏱️ Estimated effort to review: 4 🔵🔵🔵🔵⚪
🧪 No relevant tests
🔒 No security concerns identified
⚡ Recommended focus areas for review

Error Handling

The added error handling for apt-get update and apt-get install commands improves robustness but should be reviewed to ensure it doesn't introduce unintended behavior or halt the script unnecessarily.

sudo apt-get update || { echo "apt-get update failed"; exit 1; }
# 环境安装,添加错误检查
sudo apt-get -y install build-essential pkgconf libelf-dev llvm${CLANG_NUM} \
    clang${CLANG_NUM} linux-tools-common linux-tools-generic ${CROSS_COMPILE_DEB} \
    libssl-dev flex bison bc linux-source || { echo "apt-get install failed"; exit 1; }
Golang Version Check Logic

The updated logic for checking Golang version introduces a stricter requirement (1.22 or newer). This change should be validated to ensure compatibility with all intended environments.

if [ ${GO_VERSION_MIN} -lt 22 ]; then
	echo -n "you MUST use golang 1.22 or newer, "
	echo "your current golang version is ${GO_VERSION}"
Build Process Robustness

The added || exit 1 in multiple commands ensures the build process halts on failure, but it should be reviewed to confirm it doesn't unintentionally disrupt workflows or partial builds.

		-MD -MP || exit 1
	$(CMD_CLANG) -D__TARGET_ARCH_$(LINUX_ARCH) \
		$(EXTRA_CFLAGS) \
		$(BPFHEADER) \
		-DKERNEL_LESS_5_2 \
		-target bpfel -c $< -o $(subst kern/,user/bytecode/,$(subst .c,_core$(KERNEL_LESS_5_2_PREFIX),$<)) \
		-fno-ident -fdebug-compilation-dir . -g -D__BPF_TARGET_MISSING="GCC error \"The eBPF is using target specific macros, please provide -target\"" \
		-MD -MP || exit 1

.PHONY: autogen
autogen: .checkver_$(CMD_BPFTOOL)
	$(AUTOGENCMD)

.PHONY: ebpf
ebpf: autogen $(KERN_OBJECTS)

.PHONY: ebpf_noncore
ebpf_noncore: prepare $(KERN_OBJECTS_NOCORE)

.PHONY: $(KERN_OBJECTS_NOCORE)
$(KERN_OBJECTS_NOCORE): %.nocore: %.c \
	| .checkver_$(CMD_CLANG) \
	.checkver_$(CMD_GO) \
	prepare
	$(CMD_CLANG) \
			$(EXTRA_CFLAGS_NOCORE) \
			$(BPFHEADER) \
			-I $(KERN_SRC_PATH)/arch/$(LINUX_ARCH)/include \
			-I $(KERN_BUILD_PATH)/arch/$(LINUX_ARCH)/include/generated \
			-I $(KERN_SRC_PATH)/include \
			-I $(KERN_SRC_PATH)/arch/$(LINUX_ARCH)/include/uapi \
			-I $(KERN_BUILD_PATH)/arch/$(LINUX_ARCH)/include/generated/uapi \
			-I $(KERN_SRC_PATH)/include/uapi \
			-I $(KERN_BUILD_PATH)/include/generated/uapi \
			-c $< \
			-o - |$(CMD_LLC) \
			-march=bpf \
			-filetype=obj \
			-o $(subst kern/,user/bytecode/,$(subst .c,_noncore.o,$<)) || exit 1
	$(CMD_CLANG) \
			$(EXTRA_CFLAGS_NOCORE) \
			$(BPFHEADER) \
			-I $(KERN_SRC_PATH)/arch/$(LINUX_ARCH)/include \
			-I $(KERN_BUILD_PATH)/arch/$(LINUX_ARCH)/include/generated \
			-I $(KERN_SRC_PATH)/include \
			-I $(KERN_SRC_PATH)/arch/$(LINUX_ARCH)/include/uapi \
			-I $(KERN_BUILD_PATH)/arch/$(LINUX_ARCH)/include/generated/uapi \
			-I $(KERN_SRC_PATH)/include/uapi \
			-I $(KERN_BUILD_PATH)/include/generated/uapi \
			-DKERNEL_LESS_5_2 \
			-c $< \
			-o - |$(CMD_LLC) \
			-march=bpf \
			-filetype=obj \
			-o $(subst kern/,user/bytecode/,$(subst .c,_noncore$(KERNEL_LESS_5_2_PREFIX),$<)) || exit 1

.PHONY: assets
assets: .checkver_$(CMD_GO) ebpf ebpf_noncore
	$(CMD_GO) run github.com/shuLhan/go-bindata/cmd/go-bindata $(IGNORE_LESS52) -pkg assets -o "assets/ebpf_probe.go" $(wildcard ./user/bytecode/*.o)

.PHONY: assets_noncore
assets_noncore: \
	.checkver_$(CMD_GO) \
	ebpf_noncore
	$(CMD_GO) run github.com/shuLhan/go-bindata/cmd/go-bindata $(IGNORE_LESS52) -pkg assets -o "assets/ebpf_probe.go" $(wildcard ./user/bytecode/*.o)


.PHONY: $(TARGET_LIBPCAP)
$(TARGET_LIBPCAP):
	test -f ./lib/libpcap/configure || git submodule update --init
	cd lib/libpcap && \
		CC=$(CMD_CC_PREFIX)$(CMD_CC) AR=$(CMD_AR_PREFIX)$(CMD_AR) CFLAGS="-O2 -g -gdwarf-4 -static -Wno-unused-result" ./configure --disable-rdma --disable-shared --disable-usb \
			--disable-netmap --disable-bluetooth --disable-dbus --without-libnl \
			--without-dpdk --without-dag --without-septel --without-snf \
			--without-gcc --with-pcap=linux \
			--without-turbocap --host=$(LIBPCAP_ARCH) && \
	CC=$(CMD_CC_PREFIX)$(CMD_CC) AR=$(CMD_AR_PREFIX)$(CMD_AR) make || exit 1

@cfc4n cfc4n merged commit 91b8be1 into gojue:master Jan 1, 2025
7 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant