Skip to content

Conversation

@agicy
Copy link
Contributor

@agicy agicy commented Dec 10, 2025

This pull request updates the CFLAGS and LDFLAGS in the Makefile to enable static linking. This ensures that the generated binary does not depend on shared libraries at runtime.

Closes: #60

Copy link

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull request overview

This pull request modifies the Makefile to enable static linking of binaries, ensuring they don't depend on shared libraries at runtime. However, the implementation introduces several issues that need to be addressed.

Key changes:

  • Adds -static flag to both CFLAGS and LDFLAGS
  • Moves -lpthread from LDFLAGS to CFLAGS (in addition to keeping it in LDFLAGS)
  • Removes --sysroot=$(ROOT) from CFLAGS

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

@li041
Copy link

li041 commented Dec 10, 2025

I think directly adding -static to the general CFLAGS is not ideal, because it forces static linking for all architectures and all builds, which may not be desired. Instead, it makes more sense to conditionally append -static based on the target architecture.

In Makefile, lines 60-71:

ifeq ($(ARCH), arm64)
	ifeq ($(VIRTIO_GPU), y)
		include_dirs += -I/usr/$(toolchain)/include -I/usr/$(toolchain)/include/libdrm -L/usr/$(toolchain)/lib -ldrm 
	endif
else ifeq ($(ARCH), riscv)
	CFLAGS += -static
else ifeq ($(ARCH), loongarch)
	CFLAGS += -DLOONGARCH64 -static
	ifeq ($(VIRTIO_GPU), y)
		include_dirs += -I/opt/libdrm-install/include -L/opt/libdrm-install/lib -I/opt/libdrm-install/include/libdrm -ldrm
	endif
endif

Copy link

@li041 li041 left a comment

Choose a reason for hiding this comment

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

See Copilot advice and #61 (comment)

@agicy
Copy link
Contributor Author

agicy commented Dec 10, 2025

I think directly adding -static to the general CFLAGS is not ideal, because it forces static linking for all architectures and all builds, which may not be desired. Instead, it makes more sense to conditionally append -static based on the target architecture.

In Makefile, lines 60-71:

ifeq ($(ARCH), arm64)
	ifeq ($(VIRTIO_GPU), y)
		include_dirs += -I/usr/$(toolchain)/include -I/usr/$(toolchain)/include/libdrm -L/usr/$(toolchain)/lib -ldrm 
	endif
else ifeq ($(ARCH), riscv)
	CFLAGS += -static
else ifeq ($(ARCH), loongarch)
	CFLAGS += -DLOONGARCH64 -static
	ifeq ($(VIRTIO_GPU), y)
		include_dirs += -I/opt/libdrm-install/include -L/opt/libdrm-install/lib -I/opt/libdrm-install/include/libdrm -ldrm
	endif
endif

You are right that -static belongs in LDFLAGS. I will move it there. We intend to statically link hvisor-tool, so I will update the configuration accordingly.

@agicy agicy force-pushed the fix/static-linking branch from ed3411a to 1986ca6 Compare December 10, 2025 07:17
@agicy agicy requested a review from li041 December 10, 2025 07:20
Copy link

@li041 li041 left a comment

Choose a reason for hiding this comment

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

LGTM

@li041 li041 merged commit 394929f into syswonder:main Dec 10, 2025
1 check passed
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.

Enable Static Linking

2 participants