Pedantic fixes to build system (`configure`, `Makefile`) by whitslack · Pull Request #8933 · ElementsProject/lightning

whitslack · 2026-03-07T19:01:48Z

Checklist

Before submitting the PR, ensure the following tasks are completed. If an item is not applicable to your PR, please mark it as checked:

The changelog has been updated in the relevant commit(s) according to the guidelines.
Tests have been added or modified to reflect the changes. (N/A)
Documentation has been reviewed and updated as needed. (N/A)
Related issues have been listed and linked, including any that this PR closes. (N/A)
Important All PRs must consider how to reverse any persistent changes for tools/lightning-downgrade (N/A)

Andezion · 2026-08-04T12:19:11Z

-CFLAGS = $(CPPFLAGS) $(CWARNFLAGS) $(CDEBUGFLAGS) $(COPTFLAGS) -I $(CCANDIR) $(EXTERNAL_INCLUDE_FLAGS) -I . -I$(CPATH) $(SQLITE3_CFLAGS) $(SODIUM_CFLAGS) $(POSTGRES_INCLUDE) $(FEATURES) $(COVFLAGS) $(DEV_CFLAGS) -DSHACHAIN_BITS=48 -DJSMN_PARENT_LINKS $(PIE_CFLAGS) $(COMPAT_CFLAGS) $(CSANFLAGS)
+# Put the environment-inherited flags *last* so the user has the final say.
+CPPFLAGS := -DCLN_NEXT_VERSION="\"$(CLN_NEXT_VERSION)\"" -DPKGLIBEXECDIR="\"$(pkglibexecdir)\"" -DBINDIR="\"$(bindir)\"" -DPLUGINDIR="\"$(plugindir)\"" -DCCAN_TAL_NEVER_RETURN_NULL=1 -DSHACHAIN_BITS=48 -DJSMN_PARENT_LINKS $(CPPFLAGS)
+CFLAGS := $(CWARNFLAGS) $(CDEBUGFLAGS) $(COPTFLAGS) -I $(CCANDIR) $(EXTERNAL_INCLUDE_FLAGS) -I . -I$(CPATH) $(SQLITE3_CFLAGS) $(SODIUM_CFLAGS) $(POSTGRES_INCLUDE) $(FEATURES) $(COVFLAGS) $(DEV_CFLAGS) $(PIE_CFLAGS) $(COMPAT_CFLAGS) $(CSANFLAGS) $(CFLAGS)


Isnt the old CFLAGS = $(CPPFLAGS) $(CWARNFLAGS) . . . included CPPFLAGS directly, and new CFLAGS := $(CWARNFLAGS) $(CDEBUGFLAGS) . . . $(CFLAGS) does not - CPPFLAGS now only reaches the compiler through $(COMPILE.c), which the pr only wires into the generic %.o: %.c pattern rule. But Makefile has some explicit rules like:

ccan-tal.o: $(CCANDIR)/ccan/tal/tal.c @$(call VERBOSE, "cc $<", $(CC) $(CFLAGS) -c -o $@ $<)

And these still call $(CC) $(CFLAGS) directly?, so they now lose everything that moved into CPPFLAGS, plus any macOS Homebrew -I paths. The worst case is ccan-tal.o, and ccan/tal/tal.c has:

static void *null_alloc_failed(void) { #ifdef CCAN_TAL_NEVER_RETURN_NULL abort(); #else return NULL; #endif }

Without -DCCAN_TAL_NEVER_RETURN_NULL=1, an out of memory tal_alloc now returns NULL instead of aborting? - while the rest of the codebase (compiled through the fixed pattern rule) is written on the assumption tal never returns NULL?

Maybe we need to keep CPPFLAGS folded into CFLAGS (drop the split), or update every explicit compile rule to use $(COMPILE.c) or at minimum $(CC) $(CPPFLAGS) $(CFLAGS) instead of $(CC) $(CFLAGS)?

Thanks. I missed that. I'm on vacation now, but I'll do a thorough review for other direct invocations of $(CC) after I return (in a few days). The correct usage is $(COMPILE.c) when $(CC) is being invoked to compile a C source file to produce an object file. There really isn't a common scenario in which $(CC) should be used in a Make recipe except perhaps when querying/testing for compiler features. When asking the compiler to do something, the more specific variables should be used.

I think I have now fixed all the explicit compile rules to use $(COMPILE.c) rather than $(CC). There are still some instances in the Makefiles in the ccan and jsmn submodules, but I think that those Makefiles are not actually used by the CLN build and so can be ignored.

Andezion · 2026-08-04T12:20:19Z

            echo "Warning: dsymutil not found. Install Xcode Command Line Tools for better debug support."
        fi
    else
-        CDEBUGFLAGS=${CDEBUGFLAGS--std=gnu11 -g -fstack-protector-strong}


Should we add $CFLAGS to all of these invocations, or keep -std=gnu11 centralized in one flags variable that all of them already reference??

Some build systems put the -std= option directly in CC (and CXX if applicable). I don't really like that, as some code assumes that CC and friends contain simple path names with no arguments. However, there isn't a great place to specify the -std= option. It needs to be passed to the preprocessor, as feature test macros are affected by the C/C++ standard in use, but naïvely adding the -std= option to CPPFLAGS is suboptimal because C++ code will need a different standard than C code. Some build systems work around this by introducing a CXXCPPFLAGS variable that is specific to the C++ preprocessor, but that's very non-standard. I usually make the compromise of locally overriding the value of CPPFLAGS just for C++ source files. This won't be an issue for CLN for the time being since it currently has no C++ source files.

Okay, so that means we need to add $CPPFLAGS to

configure at line 382 - that have_function_sections probe

configure at line 393 - that builds the $CONFIGURATOR binary

configure at line 443 - to $CONFIGURATOR --extra-tests ..., which generates ccan/config.h ??

Indeed. I have done all that in f0d3497, although there's a bigger problem here, which I'm working on now: this doesn't support cross-compiling. Configurator needs to be built with $CC_FOR_BUILD (and its associated $CFLAGS_FOR_BUILD, $CPPFLAGS_FOR_BUILD, and $LDFLAGS_FOR_BUILD).

I think I'll open a new PR for cross-compilation support since I think it's going to involve quite a lot of changes, and it's logically independent of this PR.

It's not POSIX-compatible. Use printf instead. Changelog-None

Changelog-None

daywalker90 · 2026-08-17T13:08:09Z

Build is consistently failing in CI.

whitslack · 2026-08-17T16:27:28Z

Build is consistently failing in CI.

Um, yes. Realize that every build of CLN fails in CI. The CI never suceeds on this project. It's been that way for as long as I've been contributing to CLN.

daywalker90 · 2026-08-17T16:41:08Z

I am talking about the build step (not the tests), that one usually only fails if the azure servers are down for apt.

Your PR is also failing me locally with:

ccan/ccan/cdump/tools/cdump-enumstr lightningd/channel_state.h > lightningd/channel_state_names_gen.h
cc plugins/sql.c
cc plugins/libplugin.c
cc common/addr.c
cc common/amount.c
In file included from ./bitcoin/script.h:5,
                 from common/addr.c:4:
./bitcoin/tx.h:6:10: fatal error: wally_transaction.h: No such file or directory
    6 | #include <wally_transaction.h>
      |          ^~~~~~~~~~~~~~~~~~~~~
compilation terminated.
make: *** [Makefile:343: common/addr.o] Error 1
make: *** Waiting for unfinished jobs....
In file included from ./common/coin_mvt.h:5,
                 from ./common/json_parse.h:5,
                 from ./common/json_param.h:6,
                 from plugins/libplugin.c:12:
./bitcoin/tx.h:6:10: fatal error: wally_transaction.h: No such file or directory
    6 | #include <wally_transaction.h>
      |          ^~~~~~~~~~~~~~~~~~~~~
compilation terminated.
In file included from ./common/coin_mvt.h:5,
                 from ./common/json_parse.h:5,
                 from ./common/json_param.h:6,
                 from plugins/sql.c:10:
./bitcoin/tx.h:6:10: fatal error: wally_transaction.h: No such file or directory
    6 | #include <wally_transaction.h>
      |          ^~~~~~~~~~~~~~~~~~~~~
compilation terminated.
make: *** [Makefile:343: plugins/libplugin.o] Error 1
make: *** [Makefile:343: plugins/sql.o] Error 1

whitslack · 2026-08-17T18:57:14Z

Your PR is also failing me locally

@daywalker90: Ah. I always build against system-installed libwallycore, so I didn't hit that failure. I'll investigate.

EDIT: The problem was that EXTERNAL_INCLUDE_FLAGS is set after CFLAGS, so CFLAGS must remain a recursively expanded variable. I switched to a strategy of saving the CFLAGS inherited from the environment in CFLAGS_FROM_ENV, keeping CFLAGS as a recursively expanded variable, and tacking on $(CFLAGS_FROM_ENV) at the end of CFLAGS.

Andezion

gcc 15 default dialect is C23 (__STDC_VERSION__ 202311L), but -std=gnu11 gives C11 (201112L). So after this pr, ccan/config.h is generated under C23 while the actual project sources compile under C11. configurator.c even does __typeof__/statement-expression feature probes that feed that file, right?

The code intends to pass "$DEFAULT_COPTFLAGS" and "$DEBUGBUILD" as arguments $1 and $4 to default_cwarnflags(), but it had mistakenly doubled the double- quotes, which would have caused the values of those variables to be subjected to word splitting after substitution. Remove the extra double-quote marks. Changelog-None

Pass these flags variables when building configurator and tests. Makefile now *prepends* its default CFLAGS, CPPFLAGS, and LDFLAGS to the environment-supplied flags. This allows the user to override individual flags by setting these variables through configure, without disturbing all the rest of the flags that Makefile wants by default. Changelog-None

Make predefines variables COMPILE.c and LINK.c, providing the default commands for compiling and linking C programs: COMPILE.c = $(CC) $(CFLAGS) $(CPPFLAGS) $(TARGET_ARCH) -c LINK.c = $(CC) $(CFLAGS) $(CPPFLAGS) $(LDFLAGS) $(TARGET_ARCH) Use these variables where appropriate. A few points of interest: * Using $(LINK.o) to link a C program is not correct, as it does not pass $(CFLAGS) to the linker driver. Passing $(CFLAGS) may be necessary for correct operation. For instance, -m32 can be specified in CFLAGS to build for a 32-bit ABI on a 64-bit-native system, and -flto can be specified in CFLAGS to enable link-time optimization. The linker driver needs to be told both of these in order to produce correct output. * CFLAGS is not supposed to subsume CPPFLAGS. The latter are logically the flags for the C preprocessor, while the former are the flags for the C compiler. The standard COMPILE.c variable incorporates both sets of flags since it invokes both the preprocessor and the compiler with one command. The standard LINK.c variable also incorporates both since it can be used to preprocess, compile, and link a C program all in one shot. In its more typical usage (linking precompiled object files), the linker driver accepts but makes no use of any preprocessor flags supplied to it. * CFLAGS logically shouldn't include any -D, -U, or -I options, as those are meant for the preprocessor and not the compiler. Move such flags to CPPFLAGS. Changelog-None

It doesn't logically belong in CDEBUGFLAGS. Changelog-None

Changelog-None

Since we're linking a standalone program, we have to define main(). Changelog-None

whitslack · 2026-08-20T17:18:58Z

gcc 15 default dialect is C23 (__STDC_VERSION__ 202311L), but -std=gnu11 gives C11 (201112L). So after this pr, ccan/config.h is generated under C23 while the actual project sources compile under C11. configurator.c even does __typeof__/statement-expression feature probes that feed that file, right?

@Andezion: I think this should be fixed now since configurator is invoked with $CPPFLAGS.

whitslack force-pushed the build-system-pedantry branch from 39e6288 to d8f5abf Compare March 7, 2026 20:09

Andezion reviewed Aug 4, 2026

View reviewed changes

madelinevibes added this to the v26.09 milestone Aug 7, 2026

whitslack force-pushed the build-system-pedantry branch from 7238aa2 to b1aea32 Compare August 11, 2026 14:17

whitslack added 2 commits August 17, 2026 14:55

configure: don't use 'echo -n' under /bin/sh

1e30a02

It's not POSIX-compatible. Use printf instead. Changelog-None

configure: be consistent in writing status output to stderr

a3c48e1

Changelog-None

daywalker90 force-pushed the build-system-pedantry branch from 488cdbc to aece46d Compare August 17, 2026 12:55

daywalker90 requested a review from Andezion August 17, 2026 12:58

Andezion reviewed Aug 17, 2026

View reviewed changes

Comment thread Makefile

whitslack force-pushed the build-system-pedantry branch from 1f9cc13 to c6aa5c6 Compare August 17, 2026 19:28

daywalker90 requested a review from Andezion August 19, 2026 06:52

Andezion approved these changes Aug 19, 2026

View reviewed changes

Andezion self-requested a review August 19, 2026 11:17

Andezion requested changes Aug 19, 2026

View reviewed changes

whitslack added 6 commits August 20, 2026 13:02

build: move -std=gnu11 to CPPFLAGS

49e6e5d

It doesn't logically belong in CDEBUGFLAGS. Changelog-None

Makefile: -Wl,--gc-sections goes in LDFLAGS, not LDLIBS

ecc69ea

Changelog-None

configure: fix have_function_sections test

34cf285

Since we're linking a standalone program, we have to define main(). Changelog-None

whitslack force-pushed the build-system-pedantry branch from c6aa5c6 to 34cf285 Compare August 20, 2026 17:12

Uh oh!

Conversation

whitslack commented Mar 7, 2026

Checklist

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

daywalker90 commented Aug 17, 2026

Uh oh!

whitslack commented Aug 17, 2026

Uh oh!

daywalker90 commented Aug 17, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

whitslack commented Aug 17, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Andezion left a comment

Choose a reason for hiding this comment

Uh oh!

whitslack commented Aug 20, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

daywalker90 commented Aug 17, 2026 •

edited

Loading

whitslack commented Aug 17, 2026 •

edited

Loading