-
Notifications
You must be signed in to change notification settings - Fork 367
build: add check for static builds #1846
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
Conversation
Reviewer's GuideEnsures reliable static builds by importing an Autoconf macro to test linker flags, adding a configure-time check for '-static-libgcc -static', and updating the test suite makefile to use the corrected static flags. Class diagram for new macro and build configurationclassDiagram
class configure_ac {
+AX_CHECK_LINK_FLAG
+Check for '-static-libgcc -static'
}
class ax_check_link_flag_m4 {
+AX_CHECK_LINK_FLAG(FLAG, ACTION-SUCCESS, ACTION-FAILURE, EXTRA-FLAGS, INPUT)
}
class Makefile_am {
+tests_init_LDFLAGS = -static-libgcc -static
}
configure_ac --> ax_check_link_flag_m4 : uses macro
Makefile_am --> configure_ac : uses flags set by
Flow diagram for static build flag verification in configure.acflowchart TD
Start([Start configure]) --> CheckFlags{Check '-static-libgcc -static' linker flags}
CheckFlags -- Success --> Proceed[Continue configuration]
CheckFlags -- Failure --> Error[Show error: 'Unable to create static linked binaries. make sure glibc-static or equivalent are installed']
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hey @fabbione - I've reviewed your changes - here's some feedback:
- The m4 macro file is a direct copy from the autoconf-archive; consider pulling in the archive via a submodule or other mechanism to simplify maintenance and upstream updates.
- If you have any C++ dependencies, you may also need to check for and pass -static-libstdc++ alongside -static-libgcc to ensure full static linking of C++ symbols.
- The AX_CHECK_LINK_FLAG macro ends with a call to AX_CHECK_LINK_FLAGS instead of AX_CHECK_LINK_FLAG—please correct the macro name for consistency and to prevent any lookup issues.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- The m4 macro file is a direct copy from the autoconf-archive; consider pulling in the archive via a submodule or other mechanism to simplify maintenance and upstream updates.
- If you have any C++ dependencies, you may also need to check for and pass -static-libstdc++ alongside -static-libgcc to ensure full static linking of C++ symbols.
- The AX_CHECK_LINK_FLAG macro ends with a call to AX_CHECK_LINK_FLAGS instead of AX_CHECK_LINK_FLAG—please correct the macro name for consistency and to prevent any lookup issues.
Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
Ephemeral COPR build failed. @containers/packit-build please check. |
8e902bb
to
60da7db
Compare
a few more tests to fix |
e7a9aa0
to
da6a3e1
Compare
tests/init needs glibc-static (or equivalent) available to run the test suite. update configure.ac to check for libtool ability to link statically update Makefile.am to build tests only if static linking is available update rpm/crun.spec to BuildRequires glibc-static update tests/*/Dockerfile to include glibc-static Signed-off-by: Fabio M. Di Nitto <[email protected]>
da6a3e1
to
eb9912e
Compare
if BUILD_TESTS | ||
UNIT_TESTS = tests/tests_libcrun_utils tests/tests_libcrun_ring_buffer tests/tests_libcrun_errors tests/tests_libcrun_intelrdt | ||
endif |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
does it affect make dist
and what files are included in the tarball if we build without glibc-static?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It doesn´t affect make dist (tested on fedora42). autotools is smart enough to comment out only the build section for the tools and keep the _SOURCE bits for *_DIST sections.
Just one example:
$ grep tests_libcrun_utils Makefile
am__tests_tests_libcrun_utils_SOURCES_DIST = \
tests/tests_libcrun_utils.c
and just to be 100% safe. Done 2 make dist, one with glib-static and one without:
crun-without/tests$ ls -1 | wc -l
34
crun-with/tests$ ls -1 | wc -l
34
fabbione@mazikeen:~/work/containers$ diff -Naurd crun-without/ crun-with
fabbione@mazikeen:~/work/containers$
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
tests/init needs glibc-static (or equivalent) available to run the test suite.
update configure.ac to check for libtool ability to link statically
update Makefile.am to build tests only if static linking is available
update rpm/crun.spec to BuildRequires glibc-static
update tests/*/Dockerfile to include glibc-static