build: Disable make builtin rules.

dgoncharov commented at 12:15 pm on June 2, 2021: none

Disable make builtin rules.

When there is no rule to build a target in the makefile, make looks for a builtin rule. When -r is specified make no longer performs this lookup.

E.g. the following in an excerpt from make -d output. Here, make looks for a rule to build ‘all’.

Considering target file ‘all’. File ‘all’ does not exist. Looking for an implicit rule for ‘all’. Trying pattern rule with stem ‘all’. Trying implicit prerequisite ‘all.o’. Trying pattern rule with stem ‘all’. Trying implicit prerequisite ‘all.c’. Trying pattern rule with stem ‘all’. Trying implicit prerequisite ‘all.cc’. Trying pattern rule with stem ‘all’. Trying implicit prerequisite ‘all.C’. Trying pattern rule with stem ‘all’. Trying implicit prerequisite ‘all.cpp’. Trying pattern rule with stem ‘all’. Trying implicit prerequisite ‘all.p’. Trying pattern rule with stem ‘all’. Trying implicit prerequisite ‘all.f’. … Many more lines like this are omitted.

Because this build system does not use make builtin rules or suffixes, there is no benefit in having builtin rules enabled.

There are 2 benefits in having builtin rules disabled.

Improves performance by eliminating redundant lookups.
Simplifies troubleshooting by reducing the output of make -d or make -p.

fanquake added the label Build system on Jun 2, 2021

dgoncharov commented at 12:20 pm on June 2, 2021: none

Tested on linux.

dgoncharov commented at 12:31 pm on June 2, 2021: none

With this change applied the output of make -d in regards to target ‘all’ is the following.

Considering target file ‘all’. File ‘all’ does not exist. Looking for an implicit rule for ‘all’. No implicit rule found for ‘all’.

amadeuszpawlik commented at 1:17 pm on June 2, 2021: contributor

Because this build system does not use make builtin rules or suffixes

What about the dependencies?

fanquake commented at 1:23 pm on June 2, 2021: member

What about the dependencies?

Indeed. This wont work for at least miniupnpc:

0Building miniupnpc...
1make[1]: Entering directory '/tmp/cirrus-ci-build/depends/work/build/x86_64-apple-darwin18/miniupnpc/2.2.2-19f39f7d911'
2/bin/sh updateminiupnpcstrings.sh
3make[1]: *** No rule to make target 'addr_is_reserved.o', needed by 'libminiupnpc.a'.  Stop.
4make[1]: *** Waiting for unfinished jobs....

Note that there is also some related discussion in #18821.

dgoncharov commented at 1:53 pm on June 2, 2021: none

Indeed. Will add unexport.

dgoncharov commented at 2:16 pm on June 2, 2021: none

One thing to consider about unexport of MAKEFLAGS is that it will prevent any flag to be propagated. We’ll need to have something like the following

0saved_makeflags:=$(MAKEFLAGS)
1MAKEFLAGS+=-r
2unexport MAKEFLAGS
3
4$($(1)_built): | $($(1)_configured)
5    $(AT)echo Building $(1)...
6    $(AT)mkdir -p $$(@D)
7    $(AT)+cd $$(@D); MAKEFLAGS=$(saved_makeflags) $($(1)_build_env) $(call $(1)_build_cmds, $(1))
8    $(AT)touch $$@

This should let the options specified by the user on the command line to propagate. Let me test this.

maflcko commented at 7:25 am on June 3, 2021: member

Is this related to #22134?

laanwj commented at 2:47 pm on June 3, 2021: member

Sorry, but I’m not convinced this is a worthwhile change. It adds extra settings for a very small gain. Can you at least quantify the performance win? I would not expect much of the build time to be spent in make itself.

dongcarl commented at 6:25 pm on June 8, 2021: contributor

Sorry, but I’m not convinced this is a worthwhile change. It adds extra settings for a very small gain. Can you at least quantify the performance win? I would not expect much of the build time to be spent in make itself.

I think the potential win here is not w/re performance, but w/re explicitness. I remember struggling with builtin variables and target definitions while working on https://github.com/bitcoin/bitcoin/commit/3d6603e340d6d461832f0aa204b04343d34af3d4, and I think being explicit about where we expect to use the builtin vars and targets is a good thing (also for debugging purposes) if it can be done right.

dgoncharov commented at 1:46 am on June 9, 2021: none

Performance wise -r helps when there is nothing to be built and your program needs to link a lot of libraries. -r helps each library a little bit. For depends subsystem, -r won’t affect performance much, especially given #22134.

-r helps troubleshooting a lot.

E.g. this is the difference in debugging output.

0$ ls -l depends/packages/*.mk |wc
1     29     261    2398
2$ cat d.mk
3include depends/packages/*.mk
4all:
5$ make -d -f d.mk |wc
6  12752   64905  758491
7$ make -rd -f d.mk |wc
8    197    1200   12678

It has been my experience that people get buried in this output and do not realize they can drastically simplify it.

dgoncharov commented at 1:48 am on June 9, 2021: none

Is this related to #22134?

Not related.

DrahtBot commented at 7:27 am on June 9, 2021: contributor

The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.

Conflicts

Reviewers, this pull request conflicts with the following ones:

#24285 (build, refactor: Drop useless call Make function by hebasto)
#24279 (build: Make $(package)_*_env available to all $(package)_*_cmds by hebasto)
#22811 (build: Fix depends build system when working with subtargets by hebasto)
#19952 (build, ci: Add file-based logging for individual packages by hebasto)

If you consider this pull request important, please also help to review the conflicting pull requests. Ideally, start with the one that should be merged first.

luke-jr commented at 6:19 pm on June 12, 2021: member

GNU Make says make will provide information necessary for accessing the jobserver through the environment to its children, in the MAKEFLAGS environment variable.

Won’t this break that?

dgoncharov commented at 9:06 pm on June 13, 2021: none

Options specified by the user on the command line are all exported to make’s children. Including -j and –jobserver-auth. This change only prevents the -r flag, set in this very makefile, from being exported. If the user specifies -r on the command line, then -r is exported.

dongcarl commented at 4:09 pm on June 16, 2021: contributor

Concept ACK

Our depends/ system in itself (not including the sub-makes it calls) does not (and likely should not) use any of the built-in rules/definitions, and the current changeset limits the effect of the -r to only depends itself and not its sub-makes (this is why miniupnpc still builds)
All UNIX make implementations (GNU and BSD) support the -r flag (apparently there is no busybox/alpine make) @dgoncharov Would be nice to include some illustrative examples in your comments in depends/Makefile

dgoncharov commented at 1:15 pm on June 18, 2021: none

i added a test package here https://github.com/dgoncharov/bitcoin/tree/test_package. If you checkout this branch, you can see the value of MAKEFLAGS, that make passes to its children. E.g.

0$ TEST=1 make test_configured -k -j2 --no-print-directory 
1Extracting test...
2Preprocessing test...
3Configuring test...
4in print_makeflags.mk MAKEFLAGS=k --no-print-directory
5in print_makeflags.mk in the recipe MAKEFLAGS=k -j2 --jobserver-auth=4,5 --no-print-directory
6make[1]: 'all' is up to date.
7$

You can then pass various flags and see the output. Or whatever other test you want to run. There is simply too many combinations to list them in the makefile itself.

DrahtBot added the label Needs rebase on Dec 17, 2021

Disable builtin rules and suffixes.

When there is no rule to build a target in the makefile, make looks
for a builtin rule.
When -r is specified make no longer performs this lookup.

E.g. the following in an excerpt from make -d output.
Here, make looks for a rule to build 'all'.

Considering target file 'all'.
 File 'all' does not exist.
 Looking for an implicit rule for 'all'.
 Trying pattern rule with stem 'all'.
 Trying implicit prerequisite 'all.o'.
 Trying pattern rule with stem 'all'.
 Trying implicit prerequisite 'all.c'.
 Trying pattern rule with stem 'all'.
 Trying implicit prerequisite 'all.cc'.
 Trying pattern rule with stem 'all'.
 Trying implicit prerequisite 'all.C'.
 Trying pattern rule with stem 'all'.
 Trying implicit prerequisite 'all.cpp'.
 Trying pattern rule with stem 'all'.
 Trying implicit prerequisite 'all.p'.
 Trying pattern rule with stem 'all'.
 Trying implicit prerequisite 'all.f'.
...
Many more lines like this are omitted.

Because this build system does not use make builtin rules or suffixes,
there is no benefit in having builtin rules enabled.

There are 2 benefits in having builtin rules disabled.

1. Improves performance by eliminating redundant lookups.
2. Simplifies troubleshooting by reducing the output of make -d or
make -p.

Implementation notes:

Construct mflags to ensure -r is not passed down to children.
Some packages use builtin rules.

There is no need to handle
$ make MAKEFLAGS=<value>
use case, because this makefile filters out everything from MAKEOVERRIDES,
aside from V=%.
If the filtering of MAKEOVERRIDES is removed, then the code which modifies
MAKEFLAGS and sets mflags should be conditional e.g.

ifeq (,$(filter MAKEFLAGS=%,$(MAKEOVERRIDES)))
endif

16f46df03b

dgoncharov force-pushed on Feb 5, 2022

DrahtBot removed the label Needs rebase on Feb 5, 2022

DrahtBot added the label Needs rebase on Apr 13, 2022

DrahtBot commented at 10:13 pm on April 13, 2022: contributor

🐙 This pull request conflicts with the target branch and needs rebase.

Want to unsubscribe from rebase notifications on this pull request? Just convert this pull request to a “draft”.

achow101 commented at 6:31 pm on October 12, 2022: member

Closing this as it has not had any activity in a while. If you are interested in continuing work on this, please leave a comment so that it can be reopened.

achow101 closed this on Oct 12, 2022

achow101 added the label Up for grabs on Oct 12, 2022

achow101 commented at 6:31 pm on October 12, 2022: member

cc @dongcarl

bitcoin locked this on Oct 12, 2023

build: Disable make builtin rules. #22126

Conflicts