Autotools RFC: Round 2 #2805
Autotools RFC: Round 2 #2805
Conversation
This is Part of an autotools buildsystem port. Previously MSG_NOSIGNAL was defined in a few places, and -DMSG_NOSIGNAL=0 was passed by the osx build to signify that it was unavailable there. For Win32, it was assumed to be unavailable. This commit flips the logic so that the unix build declares the feature. The HAVE_MSG_NOSIGNAL define will be (automatically detected and) set by autotools as well, so that both build methods will give the same results. There should be no observable changes in functionality here.
This allows us to search for .git, so that we can make a guess as to whether we're in a repository or just an extracted tarball. TODO: might want to check for GIT_DIR env var as well.
For example, CC="ccache gcc" or CXX="ccache g++"
Speeds builds up a substantial amount, as expected. Local tests show full clean builds are 25x-40x faster.
|
Changes look good to me, except that I do not want to maintain two build systems in parallel. If we switch to autotools, we switch, and the existing makefiles go away. Gitian determinism can be fixed later (though just builds should be verified to work). Unfortunately; I can't test myself now (I'm not at home). About -Qt, you'll want an ACK from @laanwj I have no opinion about recursive makefiles or not. |
configure.ac
Outdated
There was a problem hiding this comment.
AC_ARG_WITH is for dependencies.
|
So then you still have to add it for each one you want to build, but you've disabled the default for what (arguably) most builders will want. I still don't see how it's a step up in any way from --with-qt (or --with-bitcoin-qt) or --without-qt. If we're talking >5 or so front-ends in the future, sure. But presumably at that point they'd have to be split into separate projects by then anyway. This whole discussion smells of an over-engineered bikeshed to me ;) |
|
--with-qt implies it's the same client being built with or without Qt, which isn't exactly the case here. |
|
Compromise at --with-qt-frontend ? |
|
or even --enable-qt-frontend, since qt-frontend would be a feature in this case rather than a library for inclusion. |
|
Would it be hard to leave it out entirely so the builder just does: |
|
Sure, those already work. They're awkward with the recursive makefiles, which is one of the reasons I was against. You have to be in the correct dir for them to work: make (makes all) But some phony targets would be no-brainers to add, then they'd work anywhere. However you still need to be able to check for things. If you didn't, you could try to make bitcoind, only to find qrcode missing, and we wouldn't know if that was on purpose or an error. Then you've just negated the purpose of the buildsystem :) So the options and checks in configure need to stay. As implemented, they're what any package maintainer would expect to see. |
|
If qrencode is missing, it should be a warning (and disable itself), not an error, unless --with-qrencode is specified ;) |
|
It is, but qrencode is never checked unless qt is enabled. Nor should it be. This discussion has gone way off track, and this is the kind of thing I was afraid of. This thing will never be merged if the points of contention are cosmetic. I'm happy to change those to whatever makes you guys happy and argue them post-merge. |
|
I don't think anyone considers this an obstacle to merging. I believe that is just waiting on @laanwj at this point. |
|
I preferred a non-recursive makefile as well, but I don't want to bikeshed about this. ACK after squash |
|
Looks really, really good. The only issue I found during review: bitcoin-config.h should normally be the -very- first include, because it may need to change the behavior of certain includes that follow. This may or may not be needed with bitcoin, but it is general practice when coding autotools code. Also, make sure bitcoin-config.h includes the standard header #ifndef FOO_H ... guard at the top and bottom. Google claims AH_TOP and AH_BOTTOM may be used for this. ACK once these issues are resolved. Pull tester want any changes for this? |
There was a problem hiding this comment.
How are these updated in the future (adding new ones to Bitcoin-Qt happens sometimes, when I create a Transifex sync pull)?
There was a problem hiding this comment.
Just add them to QT_TS and QT_QM here. Is that what you meant?
There was a problem hiding this comment.
I mean currently the files need to be listed in src/qt/bitcoin.qrc to be usable by Bitcoin-Qt. Does this whole pull move completely away from our bitcoin-qt.pro file and qmake? If that is a dumb question, sorry I'm that Windows guy and makefiles are magic for me ^^.
There was a problem hiding this comment.
No problem.
Yes, this does away with the need for qmake, instead we run the same tools that qmake uses, but we run them ourselves instead of describing the process in a .pro file.
|
@jgarzik good suggestion on bitcoin-configh.h, thanks. Done. As for the header order, I've changed it as requested, though I would disagree and say that if (in-project) include order matters, something else is broken somewhere. But that's way out of scope here :) |
|
@jgarzik as for BitcoinPullTester, I'm not sure. It seems to have a hard-coded build process. If that's the case, it should change to: |
|
RE: the pull tester: The pull tester build/test script lives at: A patch that checks for configure.ac (or whatever) and Does the Right Thing in the right places would be nifty. |
This uses makensis to create the win32 installer exe. Usage: make windows-deploy
|
Automatic sanity-testing: FAILED BUILD/TEST, see http://jenkins.bluematt.me/pull-tester/bad255871be523773c07fc3ac64f7d5f03032041 for binaries and test log. This could happen for one of several reasons:
If you believe this to be in error, please ping BlueMatt on freenode or TheBlueMatt here. This test script verifies pulls every time they are updated. It, however, dies sometimes and fails to test properly. If you are waiting on a test, please check timestamps to verify that the test.log is moving at http://jenkins.bluematt.me/pull-tester/current/ |
|
What needs to be done to move this forward? |
|
I believe all that is left is rebasing, squashing, removing the old makefiles, and updating some documentation? Fixing determinism can be done after merge, IMHO, but the build should work on all supported environments. Fixing pulltester can only be done after merging as well. |
|
I have a bunch of work in a local branch that needs to be cleaned up and pushed here. I'll have this merge-ready without fail by this time next week (my schedule is back to normal then). |
|
Closing in favor of a new (final) PR. |
|
Edit: Whoops, fixed link. |
9 participants
Changes since Round 1:
At this point I consider it to be merge-ready, with the exception of verification of gitian descriptors and documentation. Presumably removal of the old makefile.* would coincide with updated documentation. Several things can be improved and cleaned up after merge (dealing with versioning, for example), but I've tried to ignore these things as much as possible for the sake of a quick merge.
Edit: Removed crazy markup that looked like yelling -- sorry.
Here is the second go at an Autotools build system replacement. It is meant to be a drop-in replacement for the current system(s), providing the same features with no net changes. It can also live side-by-side with the old system while sharing the same build-related variables in order to facilitate a smooth transition.
I hope the benefits are obvious enough: A single/shared build procedure, portability, ease of packaging, ease for downstreams, ease for repository maintainers, cross-compilation, etc. I don't vouch for Autotools in any way, in fact, this configure.am is downright ugly (mainly just because of mingw though), but it's portable and well-established.
I've opted not to write the documentation yet, because I would like to see what comments/concerns come out of the first round of review before committing to anything.
This does away with the need for qmake, as the Makefile is capable of generating everything it needs in a portable way.
Qt-creator can be used with the Autotools plugin, and is working nicely. For those who wish to use it this way, install the Autotools plugin from the 'about' menu, then open Makefile.am as a project. It handles the build procedure, so there is no need to mess with the command-line procedures listed below.
Building from CLI:
For Linux, assuming the dependencies have been met, the build procedure looks like this:
Same for OS X, but the pkg-config path needs to be hooked up from macports first:
For mingw it's the same, but you will need to provide lots of paths in the form of:
In addition, there are helpers for qt and boost to help with finding some locations.
Use ./configure --help to see the available options.
Native windows built is untested, as I don't have a windows environment at my disposal.
Gitian should be working for win32 builds. I've guessed at Linux but have not yet verified.
'make check' will run the unit tests and print the results.
I've done my best to avoid adding any new behavior or features, and I would much prefer to aim for feature-parity before making any improvements.
TODO: