Skip to content

Improve build system #262

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.

Sign up for GitHub

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

Open
lahwaacz wants to merge 5 commits into
base: main
Choose a base branch
from
Open

Improve build system #262

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
d657876
Respect CFLAGS from the environment in nvidia-modprobe.mk
lahwaacz Apr 14, 2024
7cfacf0
Respect CGO_CFLAGS and CGO_LDFLAGS from the environment in nvcgo/Make…
lahwaacz Apr 14, 2024
26818f5
Add GO_LDFLAGS variable to nvcgo/Makefile to allow packagers specifyi…
lahwaacz Apr 14, 2024
b7bf5ec
Add STRIP_DEBUG_INFO variable to allow omitting strip during build phase
lahwaacz Apr 14, 2024
d59a46c
Modify WITH_TIRPC variable for consistency with WITH_LIBELF and WITH_…
lahwaacz Apr 14, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
23 changes: 19 additions & 4 deletions Makefile
View file
Open in desktop
Comment thread
lahwaacz marked this conversation as resolved.
Original file line number Diff line number Diff line change
Expand Up @@ -21,6 +21,7 @@ WITH_NVCGO ?= yes
WITH_LIBELF ?= no
WITH_TIRPC ?= no
WITH_SECCOMP ?= yes
STRIP_DEBUG_INFO ?= yes
Comment thread
henry118 marked this conversation as resolved.

##### Global definitions #####

Expand Down Expand Up @@ -162,6 +163,9 @@ else
LIB_LDLIBS_STATIC += -l:libelf.a
endif
ifeq ($(WITH_TIRPC), yes)
LIB_CPPFLAGS += -DWITH_TIRPC $(shell pkg-config --cflags libtirpc)
LIB_LDLIBS_SHARED += -lpthread $(shell pkg-config --libs libtirpc)
else
LIB_CPPFLAGS += -isystem $(DEPS_DIR)$(includedir)/tirpc -DWITH_TIRPC
LIB_LDLIBS_STATIC += -l:libtirpc.a
LIB_LDLIBS_SHARED += -lpthread
Expand Down Expand Up @@ -219,22 +223,28 @@ $(BIN_OBJS): %.o: %.c | shared
-include $(DEPENDENCIES)

$(LIB_SHARED): $(LIB_OBJS)
$(MKDIR) -p $(DEBUG_DIR)
$(CC) $(LIB_CFLAGS) $(LIB_CPPFLAGS) $(LIB_LDFLAGS) $(OUTPUT_OPTION) $^ $(LIB_SCRIPT) $(LIB_LDLIBS)
ifeq ($(STRIP_DEBUG_INFO), yes)
$(MKDIR) -p $(DEBUG_DIR)
$(OBJCPY) --only-keep-debug $@ $(LIB_SONAME)
$(OBJCPY) --add-gnu-debuglink=$(LIB_SONAME) $@
$(MV) $(LIB_SONAME) $(DEBUG_DIR)
$(STRIP) --strip-unneeded -R .comment $@
endif

$(LIB_STATIC_OBJ): $(LIB_OBJS)
# FIXME Handle user-defined LDFLAGS and LDLIBS
$(LD) -d -r --exclude-libs ALL -L$(DEPS_DIR)$(libdir) $(OUTPUT_OPTION) $^ $(LIB_LDLIBS_STATIC)
ifeq ($(STRIP_DEBUG_INFO), yes)
$(OBJCPY) --localize-hidden $@
$(STRIP) --strip-unneeded -R .comment $@
endif

$(BIN_NAME): $(BIN_OBJS)
$(CC) $(BIN_CFLAGS) $(BIN_CPPFLAGS) $(BIN_LDFLAGS) $(OUTPUT_OPTION) $^ $(BIN_SCRIPT) $(BIN_LDLIBS)
ifeq ($(STRIP_DEBUG_INFO), yes)
$(STRIP) --strip-unneeded -R .comment $@
endif

##### Public rules #####

Expand Down Expand Up @@ -262,12 +272,12 @@ endif
ifeq ($(WITH_LIBELF), no)
$(MAKE) -f $(MAKE_DIR)/elftoolchain.mk DESTDIR=$(DEPS_DIR) install
endif
ifeq ($(WITH_TIRPC), yes)
ifeq ($(WITH_TIRPC), no)

@henry118 henry118 Jul 13, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

This flips the existing behavior.

Currently we will build the library from source when this flag is yes, but after this change it behaves the opposite.

This might break some platforms that package this repo.

@lahwaacz lahwaacz Jul 13, 2026

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

As I wrote before:

Well, the meaning of WITH_TIRPC was not flipped:

  • before: "yes" means building libtirpc from source, "no" means don't use libtirpc at all (I think)
    (@klueska explained that "Previously no meant -- use the native sunrpc library that is bundled in the OS (instead of tirpc).")
  • after: "yes" means linking to system libtirpc, "no" means building libtirpc from source

Which other approach enabling linking to system libtirpc would you propose?

@henry118 henry118 Jul 13, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

imho we should keep the existing semantics.

  • yes - build from source
  • no - use system library

My concern is that many downstream users might already depend on the current behavior and silently flipping the meaning would cause unexpected breaks.

@lahwaacz lahwaacz Jul 13, 2026 •
edited
Loading

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

WITH_TIRPC=no does not use a system libtirpc - it uses something else for RPC.

AFAICT, there is currently no way to build with system libtirpc without patching or passing custom compiler flags. IMHO this is a good enough reason for a breaking change.

@henry118 henry118 Jul 13, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

That's the SunRPC implementation that used to be shipped as part of glibc. Like Ubuntu 18.04 and older, they often still had the glibc SunRPC headers. But modern distros would require libtirpc-dev to compile code using RPC.

To avoid changing the meaning of WITH_TIRPC, perhaps we can introduce a 3 state selector instead:

TIRPC_MODE ?= sunrpc
# sunrpc  - use the platform-provided SunRPC implementation
# bundled - build and statically link the bundled libtirpc
# system  - use the system libtirpc discovered through pkg-config

Then preserve the existing flag as a compatibility shim:

ifdef WITH_TIRPC
  ifeq ($(WITH_TIRPC),yes)
    TIRPC_MODE := bundled
  else
    TIRPC_MODE := sunrpc
  endif
endif

This keeps existing build scripts working as they do today:

make WITH_TIRPC=yes   # still means bundled libtirpc
make WITH_TIRPC=no    # still means SunRPC

while allowing the new behavior explicitly:

make TIRPC_MODE=system

$(MAKE) -f $(MAKE_DIR)/libtirpc.mk DESTDIR=$(DEPS_DIR) install
endif

install: all
$(INSTALL) -d -m 755 $(addprefix $(DESTDIR),$(includedir) $(bindir) $(libdir) $(docdir) $(libdbgdir) $(pkgconfdir))
$(INSTALL) -d -m 755 $(addprefix $(DESTDIR),$(includedir) $(bindir) $(libdir) $(docdir) $(pkgconfdir))
# Install header files
$(INSTALL) -m 644 $(LIB_INCS) $(DESTDIR)$(includedir)
# Install library files
Expand All @@ -280,7 +290,10 @@ ifeq ($(WITH_NVCGO), yes)
endif
$(LDCONFIG) -n $(DESTDIR)$(libdir)
# Install debugging symbols
ifeq ($(STRIP_DEBUG_INFO), yes)
$(INSTALL) -d -m 755 $(addprefix $(DESTDIR),$(libdbgdir))
$(INSTALL) -m 644 $(DEBUG_DIR)/$(LIB_SONAME) $(DESTDIR)$(libdbgdir)
endif
# Install configuration files
$(MAKE_DIR)/$(LIB_PKGCFG).in "$(strip $(VERSION))" "$(strip $(LIB_LDLIBS_SHARED))" > $(DESTDIR)$(pkgconfdir)/$(LIB_PKGCFG)
# Install binary files
Expand All @@ -298,7 +311,9 @@ ifeq ($(WITH_NVCGO), yes)
$(RM) $(addprefix $(DESTDIR)$(libdir)/,$(LIBGO_SHARED) $(LIBGO_SONAME) $(LIBGO_SYMLINK))
endif
# Uninstall debugging symbols
ifeq ($(STRIP_DEBUG_INFO), yes)
$(RM) $(DESTDIR)$(libdbgdir)/$(LIB_SONAME)
endif
# Uninstall configuration files
$(RM) $(DESTDIR)$(pkgconfdir)/$(LIB_PKGCFG)
# Uninstall binary files
Expand All @@ -320,7 +335,7 @@ endif
ifeq ($(WITH_LIBELF), no)
-$(MAKE) -f $(MAKE_DIR)/elftoolchain.mk clean
endif
ifeq ($(WITH_TIRPC), yes)
ifeq ($(WITH_TIRPC), no)
-$(MAKE) -f $(MAKE_DIR)/libtirpc.mk clean
endif

Expand Down
4 changes: 1 addition & 3 deletions mk/docker.mk
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -127,7 +127,6 @@ docker-amd64-verify: $(patsubst %, %-verify, $(AMD64_TARGETS)) \

# private centos target with overrides
--centos%: OS := centos
--centos%: WITH_TIRPC = yes
--centos%: WITH_LIBELF = yes
--centos8%: BASEIMAGE = quay.io/centos/centos:stream8

Expand All @@ -139,8 +138,7 @@ docker-amd64-verify: $(patsubst %, %-verify, $(AMD64_TARGETS)) \
--rhel%: OS := centos
--rhel%: VERSION = $(patsubst rhel%-$(ARCH),%,$(TARGET_PLATFORM))
--rhel%: ARTIFACTS_DIR = $(DIST_DIR)/rhel$(VERSION)/$(ARCH)
--rhel8%: CFLAGS := -I/usr/include/tirpc
--rhel8%: LDLIBS := -ltirpc
--rhel%: WITH_TIRPC = yes
--rhel8%: BASEIMAGE = quay.io/centos/centos:stream8

--verify-rhel%: OS := centos
Expand Down
2 changes: 1 addition & 1 deletion mk/nvidia-modprobe.mk
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -25,7 +25,7 @@ PATCH_FILE := $(MAKE_DIR)/nvidia-modprobe.patch

ARFLAGS := -rU
CPPFLAGS := -D_FORTIFY_SOURCE=2 -DNV_LINUX
CFLAGS := -O2 -g -fdata-sections -ffunction-sections -fstack-protector -fno-strict-aliasing -fPIC
CFLAGS := -O2 -g -fdata-sections -ffunction-sections -fstack-protector -fno-strict-aliasing -fPIC $(CFLAGS)

##### Private rules #####

Expand Down
9 changes: 6 additions & 3 deletions src/nvcgo/Makefile
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -31,15 +31,18 @@ OBJ_NAME := $(LIB_NAME).so
HDR_NAME := $(LIB_NAME).h
CTYPES_H := ctypes.h

CGO_CFLAGS := -std=gnu11 -O2
CGO_LDFLAGS := -Wl,--gc-sections -Wl,-s -Wl,-soname,$(LIB_SONAME)
CGO_CFLAGS := -std=gnu11 -O2 $(CGO_CFLAGS)
Comment thread
lahwaacz marked this conversation as resolved.
CGO_LDFLAGS := -Wl,--gc-sections -Wl,-s -Wl,-soname,$(LIB_SONAME) $(CGO_LDFLAGS)

# by default, omit the symbol table and debug information (-s) and omit the DWARF symbol table (-w).
GO_LDFLAGS ?= -s -w

build: $(OBJ_NAME)

$(OBJ_NAME): $(wildcard $(CURDIR)/*.go) $(wildcard */*.go)
export CGO_CFLAGS="$(CGO_CFLAGS)"; \
export CGO_LDFLAGS="$(CGO_LDFLAGS)"; \
$(GO) build -o $(@) -ldflags "-s -w" -buildmode=c-shared .
$(GO) build -o $(@) -ldflags "$(GO_LDFLAGS)" -buildmode=c-shared .

install: $(OBJ_NAME)
$(INSTALL) -d -m 755 $(addprefix $(DESTDIR),$(libdir) $(includedir)/$(PKG_NAME))
Expand Down