Non-recursive make #4241
Non-recursive make #4241
Conversation
|
@jgarzik You originally nack'd non-recursive, but I think your opinion has shifted since. You've got right of first refusal :) |
|
Untested ACK. Build-system changes like this shouldn't require extensive review; if the pull-tester is happy, I say pull it and do the final changes/tweaks as future pull requests. |
|
Build works fine for me. I tried to reproduce #3955: $ touch src/checkpoints.cpp
$ make src/bitcoind
make: `src/bitcoind' is up to date.Seems it doesn't detect the change in another directory even with the non-recursive make. |
|
The top-level is disconnected from the others, that hasn't changed. You're looking for: I could move it all up a dir so that would work, but I'm not sure there's much to gain from that? |
|
Don't care much, but changes look fine to me. |
|
Whoops, didn't mean to close. |
|
@theuni I don't mind, but I'd like to be able to close that issue and others that claim 'the build system is broken!&!!'. Thanks for explaining. ACK in any case as this speeds up the build. |
|
No objection if others want it. |
Build logic moves from individual Makefile.am's to include files, which the main src/Makefile.am includes. This avoids having to manage a gigantic single Makefile. TODO: Move the rules from the old Makefile.include to where they actually belong and nuke the old file.
Rules and targets no longer need to be shared between subdirectories, so this is no longer needed.
- Some file generation was still noisy, silence it. - AM_V_GEN is used rather than @ so that 'make V=1' works as intended - Cut down on file copies and moves when using sed, use pipes instead - Avoid the use of top_ and abs_ dirs where possible
Using them has the side effect of confusing the dependency-tracking logic.
Now that the build is non-recursive, adding to AM_CPPFLAGS means adding to _all_ cppflags. Logical groups of includes have been added instead, and are used individually by various targets.
|
Pushed a cleaned up and rebased version with some additional changes. On @gavinandresen's advise, I'm not going to bother breaking the move from .am's to .include's into smaller chunks for the sake of easier review. It's a pretty straightforward change. Now that we're non-recursive, things like AM_CPPFLAGS become global to all builds. I've removed our usage of those globals in favor of setting the flags per-target. Also (mainly to help in my testing of the changes) I've silenced the qt build spew. Now, the usual As discussed with @laanwj yesterday: This does not address the issue in #3955, but it doesn't regress it eaither. That can be addressed as a follow-up. I've removed the RFC description as (if pull-tester is happy) I believe this is now merge-ready. |
|
Automatic sanity-testing: PASSED, see http://jenkins.bluematt.me/pull-tester/efe6888407186b8a468f357fa7084c0a8c3de503 for binaries and test log. |
|
Pulltester is happy. I've done some testing as well
Note that before pulling you need to remove the old generated Makefles, as otherwise you get a file collision error. rm src/test/Makefile src/qt/Makefile src/qt/test/Makefile(or alternatively, |
efe6888 build: fix version dependency (Cory Fields) f4d8112 build: quit abusing AM_CPPFLAGS (Cory Fields) 56c157d build: avoid the use of top_ and abs_ dir paths (Cory Fields) 70c71c5 build: Tidy up file generation output (Cory Fields) 6b9f0d5 build: nuke Makefile.include from orbit (Cory Fields) 8b09ef7 build: add stub makefiles for easier subdir builds (Cory Fields) be4e9ae build: delete old Makefile.am's (Cory Fields) 65e8ba4 build: Switch to non-recursive make (Cory Fields)
|
Doing make -j2 check in src/: It seems that when descending into make check-TESTS, all libbitcoin_server and libbitcoin_common objects are rebuilt (very fast, probably ccache'd), but still... I'd expect the makefiles to only build them once? |
d003110 Remove unused guard (Hennadii Stepanov) Pull request description: `BITCOIN_QT_TEST` is no longer used since switching to autotools build system. Some historical refs: - #807 - bitcoin#4241 ACKs for top commit: practicalswift: utACK d003110 promag: ACK d003110. jonasschnelli: Verified ACK d003110 Tree-SHA512: 1242ef7927d2dbd2e47cdb50de6ebb20e4ac427a66a37b4d4de8ca1b50581d34f818cb576fc9fdfb3e7dd7259d11812e3807da33b3357850d67548b837d5549b
Summary: PR description: > BITCOIN_QT_TEST is no longer used since switching to autotools build system. > > Some historical refs: > > bitcoin/bitcoin#807 > bitcoin/bitcoin#4241 Backport of Core [[bitcoin/bitcoin#16350 | PR16350]] Test Plan: ``` grep -r BITCOIN_QT_TEST[^_] . cd build ninja && ninja check ``` Reviewers: O1 Bitcoin ABC, #bitcoin_abc, Fabien Reviewed By: O1 Bitcoin ABC, #bitcoin_abc, Fabien Differential Revision: https://reviews.bitcoinabc.org/D7846
f52fafc build: Drop pointless sed commands (Hennadii Stepanov) Pull request description: Since moving to Autotools build system (35b8af9, #2943, 2013-09), tag strings created by Qt specialized compilers ([uic](https://doc.qt.io/qt-5/uic.html), [moc](https://doc.qt.io/qt-5/moc.html), [rcc](https://doc.qt.io/qt-5/rcc.html)) were being removed. A bit later (70c71c5, #4241, 2014-06) this rule was dropped for the uic, and since then all of the generated `ui_*.h` files contain the following string: ``` ** Created by: Qt User Interface Compiler version 5.12.8 ``` Such strings do not contain any timestamps, and cannot cause any non-determinism. The removing of them seems pointless. Diffs for some files: ```diff --- master/intro.moc +++ pr/intro.moc @@ -1,6 +1,7 @@ /**************************************************************************** ** Meta object code from reading C++ file 'intro.cpp' ** +** Created by: The Qt Meta Object Compiler version 67 (Qt 5.12.8) ** ** WARNING! All changes made in this file will be lost! *****************************************************************************/ ``` ```diff --- master/moc_addressbookpage.cpp +++ pr/moc_addressbookpage.cpp @@ -1,6 +1,7 @@ /**************************************************************************** ** Meta object code from reading C++ file 'addressbookpage.h' ** +** Created by: The Qt Meta Object Compiler version 67 (Qt 5.12.8) ** ** WARNING! All changes made in this file will be lost! *****************************************************************************/ ``` ```diff --- master/qrc_bitcoin.cpp +++ pr/qrc_bitcoin.cpp @@ -1,6 +1,7 @@ /**************************************************************************** ** Resource object code ** +** Created by: The Resource Compiler for Qt version 5.12.8 ** ** WARNING! All changes made in this file will be lost! *****************************************************************************/ ``` ACKs for top commit: laanwj: ACK f52fafc Tree-SHA512: 31f5c19b37645b4914f17d8c234b7ae8781a0499c4b250ffef07d70b7552954fb682f58a75d76162f98ab5e1667288b3a041df2705573fb00523e87b9c1fd47f
I decided to take care of this before handling the libsecp256k1 changes, because this should end up making future work much less tangled.
The point of this work is to avoid having separate logical builds for each directory. That causes complications because one target cannot know about changes in another directory. For example, if bitcoin/net.h changes, the qt target does not know that, and will continue to happily build/link against the stale intermediate libraries.
With a non-recursive system, however, each target is aware of all of its own components. In the example above, changing src/net.h would force a rebuild of all files that include that header, regardless of where they're located. Non-recursive also means that lots of spaghetti can go away, because we can include any dependency anywhere we want.
It also helps with parallelism because multiple targets can be compiling at once. A quick test shows that a fully ccached make -j8 of src/ (eliminating compiler time) is reduced from 20.4 sec to 13.7 sec. A make -j8 without ccache is reduced from 73.6 sec to 62.1.
Good/Simple description of the difference here: https://www.flameeyes.eu/autotools-mythbuster/automake/nonrecursive.html
Build logic moves from individual Makefile.am's to include files, which the main src/Makefile.am includes. This avoids having to manage a gigantic single Makefile.
Stub Makefiles have been added to imitate previous behavior, for those who may have gotten used to doing a make or clean in specific subdirs. These can be expanded as desired, they're very simple for now.
Most deviations from the original Makefile.am's involve eliminating $(BUILT_SOURCES), so that generated files are created as late as possible, and without requiring a vanilla 'make' (without a target) as they do currently.