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

Improve build system #262

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

Improve build system #262

wants to merge 5 commits into from

Conversation

lahwaacz
Copy link

These changes make building with system flags and libs more straightforward or actually possible. I had to patch the build system a lot while making a package for Arch Linux.

…ng their own flags

For example, Arch Linux wants debug information in the binaries.

Signed-off-by: Jakub Klinkovský <[email protected]>
For example, Arch Linux strips debug information after build during the
package creation and producing separate debug info files during the
build interferes with the Arch tooling.

Signed-off-by: Jakub Klinkovský <[email protected]>
…SECCOMP

"yes" means linking to system libtirpc, "no" means building libtirpc from source.

Signed-off-by: Jakub Klinkovský <[email protected]>
@lahwaacz
Copy link
Author

@elezar Can you look at this please? 🙏

@elezar elezar self-assigned this May 22, 2024
@elezar
Copy link
Member

elezar commented May 22, 2024

@lahwaacz sorry for the delay. I will have a look.

@elezar elezar requested review from elezar and klueska and removed request for elezar May 23, 2024 08:33
Copy link
Member

@elezar elezar left a comment

Choose a reason for hiding this comment

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

Thanks for the changes.

I think they look good. The one thing missing is ensuring that the defaults for all major platforms are the same given the WITH_TIRPC changes.

@@ -31,8 +31,8 @@ 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)
Copy link
Member

Choose a reason for hiding this comment

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

Does using += over := make sense in this context?

Copy link
Author

Choose a reason for hiding this comment

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

Using += instead of := would change the order of the flags. The user's CGO_CFLAGS should be at the end so they can override the project defaults (particularly the optimization level).

Copy link
Member

Choose a reason for hiding this comment

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

Since we've flipped the meaning of WITH_TIRPC, do we not need to also update the default?

At the very least we chould also update the ARG WITH_TIRPC set in the various docker files.

Copy link
Author

@lahwaacz lahwaacz May 24, 2024

Choose a reason for hiding this comment

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

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)
  • after: "yes" means linking to system libtirpc, "no" means building libtirpc from source

I've checked the Dockerfiles and they seem correct: Dockerfile.centos installs libtirpc-devel and has WITH_TIRPC=yes, all others have WITH_TIRPC=no since they don't install the system package.

Copy link
Contributor

Choose a reason for hiding this comment

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

Previously no meant -- use the native sunrpc library that is bundled in the OS (instead of tirpc).

Copy link
Author

Choose a reason for hiding this comment

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

@elezar Can this be marked as resolved or do you think some changes are needed?

@lahwaacz
Copy link
Author

@elezar Any progress with merging this? It's been almost half a year...

@lahwaacz lahwaacz requested a review from elezar October 1, 2024 07:00
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