autotools: Gui split #2700
autotools: Gui split #2700
Conversation
|
Automatic sanity-testing: FAILED BUILD/TEST, see http://jenkins.bluematt.me/pull-tester/37d4ab5e11da48e7627b446a2fae716555c58f18 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/ |
|
Automatic sanity-testing: PASSED, see http://jenkins.bluematt.me/pull-tester/feb4b7ef8d3ffb49b9bca41928051f7a8f6b4f29 for binaries and test log. |
|
What's the benefit of this refactor? |
|
@jonasschnelli: Compilation speed and sanity. This allows compiling the There are only very few differences left under QT_GUI defines (some help |
|
Any QT_GUI ifdef in the core code means that the core <-> GUI split wasn't done correctly in the first place. The core semantics shouldn't depend on whether there is a GUI on top of it or not... |
src/init_noui.cpp
Outdated
There was a problem hiding this comment.
Suggestion: move AppInit to this file as well. It is a bit empty, and AppInit is only used by the bitcoind (not by either bitcoin-qt or the tests), so it fits here.
There was a problem hiding this comment.
I left AppInit where it was because there's no logical reason to split it out (meaning no risk of symbol collisions if left in the static archive). It could feasibly be used to start a non-graphical session via the qt binary using something like 'bitcoin-qt --no-gui' if such behavior was ever desired in the future. But if there's opposition, sure, I can do that.
There was a problem hiding this comment.
Such a feature (start a daemon session from bitcoin-qt) will never be
supported. There is already the debug console window and the --server
option.
Functions that are only used by either the UI or daemon should be moved to
their respective frontends (unless there is a very good reason not to, like
keeping methods acting on an object together, but I disagree there is one
in this case).
|
ACK on the concept; I haven't compiled/tested. |
|
ACK |
|
@jonasschnelli you said: "I like ifdefs". If that's the resounding attitude here, I'll need to reevaluate my autotools work. Ifdefs are a portability nightmare if used to control runtime behavior. In this case, depending on the compiler/linker/settings used, the unreachable paths might be stripped away just as if ifdefs had been used. Would you mind explaining your position, and if it's common for bitcoin development? I have no problem adapting (even if i disagree), but it may negate the reasoning for my work. |
|
@theuni I don't think @jonasschnelli 's ifdef love is representative of most developers here. It makes sense for some optional build-time features (UPnP, IPv6), but not so much in this case. |
|
@theuni: no. just go on (even when i – generally – like #ifdef's). I just like binaries that only containing code which will be runned through. But for this case ifdefs are not the right thing. |
|
Roger, thanks. I have more to say on the subject, but I'll do it in code/PR form as I go rather than discussing vague concepts here. |
|
#ifdefs are evaluated on a case-by-case basis, if they make sense. If the GUI supports a runtime switch that forces daemon mode, then "#ifdef GUI" construct is not applicable for that build. It depends on the build and platform. Clearly, #ifdef GUI is applicable to a bitcoind-only build. Thus, just giving that one example, you can see where #ifdef is, and is not, applicable. Personal preference never enters into the equation. If #ifdef is applicable, it is used. Otherwise it is not. |
|
ACK on the general concept |
|
Ok, I've pushed changes to address comments here. I'll squash it down if everyone is ok with the above. |
|
Automatic sanity-testing: PASSED, see http://jenkins.bluematt.me/pull-tester/c5e9ebcfb206deb19db166db4063120f220a3f3b for binaries and test log. |
|
Any reason why the init_noui code isn't just in noui? |
|
Agree with @sipa. We could call the resulting file |
|
Good idea. |
|
yep, makes perfect sense. will do. |
This will allow each to have its own main(), meaning that we can build a common base client and simply link in the correct startup object to create the appropriate binary.
|
renamed init_noui.cpp to bitcoind.cpp, rebased to current HEAD, and squashed logically. Anything else needed? |
|
Grr, I just reread the comment above and realized I didn't make the change you guys were after. doing now. |
|
Actually, the noui stuff can't move into bitcoind.cpp, as that would defeat the purpose of this rework. That would mean that test_bitcoin would have to link in bitcoind.o for noui_connect(), while also pulling in a conflicting main(). The object containing main() needs to stay as thin as possible. |
|
Automatic sanity-testing: PASSED, see http://jenkins.bluematt.me/pull-tester/6887bb9ad7f6722a64e9fbdf0a780f362dc796a1 for binaries and test log. |
|
ACK |
1 similar comment
|
ACK |
|
ACK Coordination note: Recommend waiting until #2154 is merged, to merge this. |
|
#2154 won't be a while. See that pull req already discussing a hold on other pull reqs. |
|
Great, best of all worlds :) |
|
great, thanks |
8 participants
This is the beginning of an autotools build-system refactor. My first step is to be able to create a common static lib for the binaries (console/qt/test) to link against, meaning that each of those will need its own entry-point object file.
This series gets rid of the QT_GUI define in favor of letting the build-system link in the proper startup file, and a global var to determine how the binary was started.
I've tried to match this project's formatting and general practices.