misc build system cleanup #5511

garlick · 2023-10-19T01:58:37Z

Misc. build system cleanups split off from other work.

Problem: several convenience libraries are built with shared library options that are no doubt ignored. Drop those options.

grondo

Nice cleanup! I noticed that the fedora38 builder had some segfaults though 😬

grondo · 2023-10-19T02:42:22Z

src/common/libflux/conf.c

-    { "upmi_pluginpath",INSTALLED_UPMI_PLUGINPATH,  INTREE_UPMI_PLUGINPATH },
-    { "no_docs_path",   INSTALLED_NO_DOCS_PATH,     INTREE_NO_DOCS_PATH },
-    { "rundir",         INSTALLED_RUNDIR,           NULL },
+    {


Would this conf.c cleanup belong in a separate commit? (Nice cleanup though!)

Do you mean the reformatting and use of designated initailizers? I did have to touch every entry in that array to accomplish the goal of the commit, so I figured if I use a different format (one more appropriate for the new content) then that was OK to do in one go. If you feel strongly about it, let me know and I'll split it up.

I do not feel strongly about it. Since the designated initializers ended up as most of the change I was just confused and thought this was a reformatting of the structure. (Whatever the main change was it seems to be lost in the other changes)

Oh well that's a fair point! I was also struggling to get meaning out of the diff. I'll fix.

grondo · 2023-10-19T02:45:22Z

src/cmd/Makefile.am

@@ -24,7 +24,6 @@ fluxcmd_ldadd = \
 	$(top_builddir)/src/common/libflux-internal.la \
 	$(top_builddir)/src/common/libflux-core.la \
 	$(top_builddir)/src/common/libflux-optparse.la \
-	$(top_builddir)/src/common/libccan/libccan.la \


Commit message typo: libflux-internl.la

garlick · 2023-10-19T14:28:05Z

The fedora segfaults are a bit mystifying. I was able to replicate in docker and found it was

Thread 10 "flux-broker-0" received signal SIGSEGV, Segmentation fault.
[Switching to Thread 0x7f410d31d6c0 (LWP 272500)]
0x00007f411e457e5e in getenv () from /lib64/libc.so.6
Missing separate debuginfos, use: dnf debuginfo-install glibc-2.37-4.fc38.x86_64 libgcc-13.1.1-1.fc38.x86_64 libstdc++-13.1.1-1.fc38.x86_64
(gdb) bt
#0  0x00007f411e457e5e in getenv () from /lib64/libc.so.6
#1  0x00007f411e7e1220 in kvs_get_namespace ()
    at ../../../../src/common/libkvs/kvs_util.c:65
#2  0x00007f411e7de6f5 in flux_kvs_commit (h=0x7f40f0000bf0, ns=ns@entry=0x0, 
    flags=flags@entry=0, txn=txn@entry=0x7f40f000ad90)
    at ../../../../src/common/libkvs/kvs_commit.c:112
#3  0x00007f410d33c873 in restart_save_state (ctx=ctx@entry=0x7f410d31c890)
    at ../../../../src/modules/job-manager/restart.c:271
#4  0x00007f410d337b8f in mod_main (h=0x7f40f0000bf0, argc=<optimized out>, 
    argv=<optimized out>)
    at ../../../../src/modules/job-manager/job-manager.c:240
#5  0x0000000000410926 in module_thread (arg=0x228ba60)
    at ../../../src/broker/module.c:204
#6  0x00007f411e4a3907 in start_thread () from /lib64/libc.so.6
#7  0x00007f411e529774 in clone () from /lib64/libc.so.6

during broker shutdown. If I manually unloaded the job manager before shutdown, the segfault would move to the next place getenv() was called. Finally in an effort to install a missing glibc debuginfo package to get more clues, and finding that the debuginfo package was somehow not available for the installed glibc, I updated everything with yum -y update and the segfault disappeared. Gah.

garlick · 2023-10-19T17:22:13Z

Apparently this commit is what's causing the broker segfault on fedora. Since it doesn't affect the broker linkage, it must be affecting the environment that the broker runs in? I dunno. I think I'll just drop it for now though!

 diff --git a/src/cmd/Makefile.am b/src/cmd/Makefile.am
index 079185eac..f5351916e 100644
--- a/src/cmd/Makefile.am
+++ b/src/cmd/Makefile.am
@@ -19,13 +19,11 @@ AM_CPPFLAGS = \
 
 fluxcmd_ldadd = \
        $(top_builddir)/src/common/libkvs/libkvs.la \
-       $(top_builddir)/src/common/librlist/librlist.la \
        $(top_builddir)/src/common/libflux-internal.la \
        $(top_builddir)/src/common/libflux-core.la \
        $(top_builddir)/src/common/libflux-optparse.la \
        $(FLUX_SECURITY_LIBS) \
        $(LIBPTHREAD) \
-       $(HWLOC_LIBS) \
        $(JANSSON_LIBS)
 
 LDADD = $(fluxcmd_ldadd)
@@ -140,13 +138,15 @@ flux_start_LDADD = \
        $(top_builddir)/src/common/libczmqcontainers/libczmqcontainers.la
 
 flux_job_LDADD = \
-       $(fluxcmd_ldadd) \
        $(top_builddir)/src/shell/libmpir.la \
        $(top_builddir)/src/common/libjob/libjob.la \
        $(top_builddir)/src/common/libutil/libutil.la \
+       $(top_builddir)/src/common/librlist/librlist.la \
        $(top_builddir)/src/common/libczmqcontainers/libczmqcontainers.la \
        $(top_builddir)/src/common/libdebugged/libdebugged.la \
-       $(top_builddir)/src/common/libterminus/libterminus.la
+       $(top_builddir)/src/common/libterminus/libterminus.la \
+       $(fluxcmd_ldadd) \
+       $(HWLOC_LIBS)
 
 flux_terminus_LDADD = \
        $(top_builddir)/src/common/libterminus/libterminus.la \
@@ -185,7 +185,13 @@ flux_top_CPPFLAGS = \
        $(AM_CPPFLAGS) \
        $(CURSES_CFLAGS)
 flux_top_LDADD = \
+       $(top_builddir)/src/common/librlist/librlist.la \
+       $(top_builddir)/src/common/libczmqcontainers/libczmqcontainers.la \
+       $(top_builddir)/src/common/libhostlist/libhostlist.la \
+       $(top_builddir)/src/common/libidset/libidset.la \
+       $(top_builddir)/src/common/libutil/libutil.la \
        $(fluxcmd_ldadd) \
+       $(HWLOC_LIBS) \
        $(CURSES_LIBS)
 
 #

Naturally, hwloc is somehow involved :-)

garlick · 2023-10-19T17:24:45Z

Just force pushed with

the offending commit dropped
the config table commit split into just the reformat, then the substantive change (as discussed)
added one commit that drops some Makefile.am variables that are barely used
that change also affects variables set in flux-core.pc but a quick check of other projects didn't turn up any usage
Edit: oh! and I had messed up one of the config table paths (/run/flux didn't have the flux part!) Noticed due to splitting that commit so thanks for that.

grondo

LGTM! Just noticed a few commit message typos.

grondo · 2023-10-19T18:04:35Z

configure.ac

-
-AS_VAR_SET(fluxrc3dir, $sysconfdir/flux/rc3.d)
-AC_SUBST(fluxrc3dir)
+AS_VAR_SET(fluxconfdir, $sysconfdir/flux)


commit msg typo: "ther"

grondo · 2023-10-19T18:05:34Z

src/common/libflux/conf.c

@@ -45,27 +45,96 @@ struct flux_conf {
 static const char *conf_auxkey = "flux::conf_object";

 static struct builtin builtin_tab[] = {
-    { "lua_cpath_add",  INSTALLED_LUA_CPATH_ADD,    INTREE_LUA_CPATH_ADD },


Commit message typos: "Problen", "desigated"

grondo · 2023-10-19T18:07:41Z

src/common/libdebugged/Makefile.am

@@ -11,11 +11,6 @@ AM_CPPFLAGS = \
 noinst_LTLIBRARIES = libdebugged.la
 libdebugged_la_SOURCES = debugged.c debugged.h

-libdebugged_la_CPPFLAGS = \


Commit message: unnecessary apostrophe in Makefile.am's

Typo: "The caused the library" -> "This caused the library" ?

grondo · 2023-10-19T18:08:40Z

src/cmd/Makefile.am

@@ -13,7 +13,6 @@ AM_CPPFLAGS = \
 	$(ZMQ_CFLAGS) \
 	$(FLUX_SECURITY_CFLAGS) \
 	$(HWLOC_CFLAGS) \
-	$(PMIX_CFLAGS) \


Commit message typo: "refrence"

Also, maybe missing a leading "The" in problem statement?

Problem: configure.ac defines fluxrc1dir and fluxrc3dir but these are little used and can be defined where needed. fluxrcdir should have a more general name now that more than rc files are stored there. Rename fluxrcdir to fluxconfdir. Drop the global definitions of fluxrc1dir and fluxrc3dir. Add a local definition of fluxrc1dir to etc/Makefile.am. The changes are propagated to the flux-core.pc file, but a cursory check of framework projects did not show that they are being used so there should be no impact.

Problem: the table of built-in config values is formatted in a compact manner that is not easy to update. Reformat using designated initializers, one per line.

Problem: running a 'make' in libflux is slow. The slowness seems to result from the way we are defining many C preprocessor symbols on the command line. Modify configure.ac to define some of those in config.h and do the rest in the C source file. Serial build times on my system are reduced from 17.3s to 3.3s. Defining CPPFLAGS separately for libflux.la was causing every object to be prefixed with the library name. Now that there are fewer flags, just add them to AM_CPPFLAGS.

Problem: the libmessage.la convenience library in libflux is no longer warranted. Fold the private message functions into the libflux.la convenience library and eliminate the libmessage.la one. Since they are not "flux_" prefixed, they will not be made public in libflux-core.so. Link the libzmqutil unit tests and the broker against the libflux.la convenience library so they can use those functions.

Problem: several convenience library Makefile.ams set libfoo_la_CPPFLAGS = $(AM_CPPFLAGS) which is the default. Drop libfoo_la_CPPFLAGS when it's only defined to $(AM_CPPFLAGS). The caused the library name to be prefixed to objects for no reason. Also drop libfoo_la_LDFLAGS when it's only defined to $(AM_LDFLAGS) since that's a no-op too.

Problem: only one executable in the cmd directory requires libpmi but all the executables are linked against it. Create a special LDADD for the flux command so only flux-pmi (builtin) is linked against the pmi convenience libraries.

Problem: commands are linked against libccan.la and also libflux-internal.la which contains it. Drop libccan.la from fluxcmd_ldadd.

Problem: src/cmd/Makefile.am includes vestigal reference to PMIX_CFLAGS and PMIX_LIBS, but those are no longer set. Drop them.

Problem: only one executable in the cmd directory requires libarchive and the internal libfilemap library, but all the executables are linked with them. Move those libs to flux_LDADD so only the builtins that require those libraries are linked against them.

garlick · 2023-10-19T18:20:58Z

Oof, sorry to give you so many typos! All fixed and re-pushed. I'll set MWP.

Thanks!

codecov · 2023-10-19T18:57:35Z

Codecov Report

Merging #5511 (abb87fb) into master (475229a) will decrease coverage by 0.01%.
Report is 11 commits behind head on master.
The diff coverage is n/a.

@@            Coverage Diff             @@
##           master    #5511      +/-   ##
==========================================
- Coverage   83.69%   83.69%   -0.01%     
==========================================
  Files         485      485              
  Lines       81622    81622              
==========================================
- Hits        68314    68311       -3     
- Misses      13308    13311       +3

Files	Coverage Δ
src/common/libflux/conf.c	`85.56% <ø> (ø)`

... and 6 files with indirect coverage changes

build: drop inappropriate libtool options

20c29e9

Problem: several convenience libraries are built with shared library options that are no doubt ignored. Drop those options.

garlick force-pushed the build_fixes branch 2 times, most recently from d24475f to 7ff1568 Compare October 19, 2023 11:40

grondo approved these changes Oct 19, 2023

View reviewed changes

garlick force-pushed the build_fixes branch from 7ff1568 to d4c4adf Compare October 19, 2023 17:23

grondo approved these changes Oct 19, 2023

View reviewed changes

garlick added 9 commits October 19, 2023 11:17

libflux: reformat builtin config table

4583c52

Problem: the table of built-in config values is formatted in a compact manner that is not easy to update. Reformat using designated initializers, one per line.

build: don't link all commands with pmi libraries

18fd3e1

Problem: only one executable in the cmd directory requires libpmi but all the executables are linked against it. Create a special LDADD for the flux command so only flux-pmi (builtin) is linked against the pmi convenience libraries.

build: don't link commands with extra libccan.la

9b278e7

Problem: commands are linked against libccan.la and also libflux-internal.la which contains it. Drop libccan.la from fluxcmd_ldadd.

build: drop undefined PMIX_CFLAGS, _LIBS

e128bfe

Problem: src/cmd/Makefile.am includes vestigal reference to PMIX_CFLAGS and PMIX_LIBS, but those are no longer set. Drop them.

garlick force-pushed the build_fixes branch from d4c4adf to abb87fb Compare October 19, 2023 18:20

garlick added the merge-when-passing label Oct 19, 2023

mergify bot merged commit 4bf109f into flux-framework:master Oct 19, 2023
30 checks passed

garlick deleted the build_fixes branch March 1, 2024 16:08

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

misc build system cleanup #5511

misc build system cleanup #5511

garlick commented Oct 19, 2023

grondo left a comment

grondo Oct 19, 2023

garlick Oct 19, 2023

grondo Oct 19, 2023

garlick Oct 19, 2023

grondo Oct 19, 2023

garlick commented Oct 19, 2023

garlick commented Oct 19, 2023

garlick commented Oct 19, 2023 •

edited

Loading

grondo left a comment

grondo Oct 19, 2023

grondo Oct 19, 2023

grondo Oct 19, 2023

grondo Oct 19, 2023

grondo Oct 19, 2023

garlick commented Oct 19, 2023

codecov bot commented Oct 19, 2023

misc build system cleanup #5511

misc build system cleanup #5511

Conversation

garlick commented Oct 19, 2023

grondo left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

garlick commented Oct 19, 2023

garlick commented Oct 19, 2023

garlick commented Oct 19, 2023 • edited Loading

grondo left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

garlick commented Oct 19, 2023

codecov bot commented Oct 19, 2023

Codecov Report

garlick commented Oct 19, 2023 •

edited

Loading