Create a wrapper header file which includes config/bitcoin-config.h only if it exists (according to a macro). Files can include this wrapper header directly instead of having each including file repeat the check-if-config-exists-and-only-include-if-it-does macros.
p.s. Open to suggestions if anyone has a better filename for the wrapper header.
How about just
#include "bitcoin-config.h"
which includes config/bitcoin-config.h.
brittle to include paths changes
I believe there is a plan to make all includes relative to src/ cc @laanwj.
Another option is to change the generated to, for instance
AC_CONFIG_HEADERS([src/config/bitcoin-config-gen.h])
And always have config/bitcoin-config.h which checks for that.
Another option is to change the generated to, for instance
AC_CONFIG_HEADERS([src/config/bitcoin-config-gen.h]) And always have config/bitcoin-config.h which checks for that.
That's fine with me if more people agree that's preferable.
Personally I think
"#include "config/bitcoin-config-if-exists.h"
or
"#include "bitcoin-config-if-exists.h"
reads nice and explicit, saying what we are actually doing - including the config file, but just in case it exists.
Other than refactoring, does this have any benefit? I'm not too keen on this idea tbh
@MeshCollider In general, refactoring in order to DRY code has benefits.
And more. But I think that's enough.
Also, any particular reason why you are not keen on the idea?
@danra, not exactly sure, its just something about creating an entire new file just for 3 lines to import another file. IMO its clear enough as it is, because its just an if-statement, and anyone who would currently forget the ifdef would probably also forget to use this file, and just include bitcoin-config.h directly. Yes DRY is better, but I think this is taking it a bit far, that's all :)
@MeshCollider In addition to the considerations I mentioned, it's a macro if, easy to either misspell or get wrong (checking for value instead of just being defined, etc.) Better contain it to a single file and forget about it.
@achow101 It's fine. I didn't miss anything :P
Seriously, I'm not sure a script would help very much, since there're subtle variations of the original ifdefs as you can see in the diff. So at best the script would show handling of some variations, and the rest of the code would have to be checked anyway for any other variations not covered by the script.
At best, the script would prove no misspellings of the new include, but that would be caught as a compile error anyway.
A scripted diff is easier to review and verify that the changes are correct, especially for such a change like this which is basically a scripted diff.
DAK why Travis is failing to find "config/bitcoin-config-if-present.h" in the build? Is this some cache issue? It builds fine on my local machine.
@danra I haven't looked by it could be that out of tree builds are failing.
@MarcoFalke Tested out of tree build on my local machine - works fine. Would appreciate any other pointers as to what else might be causing the travis build to fail.
Run make distcheck.
-BEGIN VERIFY SCRIPT-
GLOBIGNORE="*bitcoin-config-if-present*" ;
sed -i '/HAVE_CONFIG_H/ { N;N; s/.*HAVE_CONFIG_H.*\n#include "config\/bitcoin-config.h"\(.*\)\n#endif.*/#include "config\/bitcoin-config-if-present.h"\1/; }' ./src/*.h ./src/*.cpp ./src/*/*.h ./src/*/*.cpp ./src/*/*/*.h ./src/*/*/*.cpp ;
-END VERIFY SCRIPT-Does anyone know if GLOBIGNORE is supported in the shell on the Travis host which fails the scripted-diff check? It works on my local bash but I can't get it to work here, tried multiple variations.
Closing this for now, I don't think there is agreement to do this, and rebasing it will be a ton of work.
