refactor: Make all #includes relative to project root

laanwj commented at 3:47 pm on August 15, 2017: member

(inspired by discussion here: #10976 (comment) and #10752 (comment))

This makes all includes in the project relative to the project root only.

There are some advantages to this, both for developers and for the compiler:

It’s immediately apparent where an included file is when reading the code
There is no confusion if a certain filename is not unique in the project (so there can be an src/init.h and src/wallet/init.h without potential confusion, as discussed in #10976)
By switching to <...> includes the compiler doesn’t have to look everywhere, resulting in less ‘probing’ before looking in the include path -I which can affect compilation performance. For example right now when primitives/block.h includes “primitives/transaction.h”, it will look in src/primitives/primitives first.

Most of the changes by number are in src/qt as all the source files there assume that they can include from there. I’ve separated this out, so that we could decide to do this only for non-qt files first ,for example.

Ping @theuni @ryanofsky

laanwj added the label Build system on Aug 15, 2017

practicalswift commented at 3:56 pm on August 15, 2017: contributor

Concept ACK 👍

paveljanik commented at 4:17 pm on August 15, 2017: contributor

0qt/notificator.cpp:27:10: error: 'macnotificationhandler.h' file not found with <angled> include; use "quotes" instead

jnewbery commented at 5:27 pm on August 15, 2017: member

Concept ACK.

Are you planing on squashing everything to a single commit? Intermediate commits are broken (eg 15d3a6d3343f27e8c01faa5674a2fe406488eac3 breaks building script_error.cpp)

ryanofsky commented at 5:43 pm on August 15, 2017: member

I think it is possible to do more of this as a scripted diff. I was looking into this this morning, and the following command seemed to work well for switching all include paths that don’t depend on the current directory from "" to <> syntax:

 0for f in \
 1  src/*.cpp \
 2  src/*.h \
 3  src/bench/*.cpp \
 4  src/bench/*.h \
 5  src/compat/*.cpp \
 6  src/compat/*.h \
 7  src/consensus/*.cpp \
 8  src/consensus/*.h \
 9  src/crypto/*.cpp \
10  src/crypto/*.h \
11  src/crypto/ctaes/*.h \
12  src/policy/*.cpp \
13  src/policy/*.h \
14  src/primitives/*.cpp \
15  src/primitives/*.h \
16  src/qt/*.cpp \
17  src/qt/*.h \
18  src/qt/test/*.cpp \
19  src/qt/test/*.h \
20  src/rpc/*.cpp \
21  src/rpc/*.h \
22  src/script/*.cpp \
23  src/script/*.h \
24  src/support/*.cpp \
25  src/support/*.h \
26  src/support/allocators/*.h \
27  src/test/*.cpp \
28  src/test/*.h \
29  src/wallet/*.cpp \
30  src/wallet/*.h \
31  src/wallet/test/*.cpp \
32  src/wallet/test/*.h \
33  src/zmq/*.cpp \
34  src/zmq/*.h
35do
36  base=${f%/*} sed -i "s:#include \"\(.*\)\"\(.*\):if test -e \$base/'\\1'; then echo '#include \"\\1\"\\2'; else echo '#include <\\1>\\2'; fi:e" $f
37done

ryanofsky commented at 5:54 pm on August 15, 2017: member

My branch is here (2 commits): https://github.com/ryanofsky/bitcoin/commits/pr/inc

It also updates developer notes. Feel free to take anything there that might be useful

laanwj commented at 6:21 pm on August 15, 2017: member

Are you planing on squashing everything to a single commit? Intermediate commits are broken (eg 15d3a6d breaks building script_error.cpp)

Yes, before merging we could squash it. Before then, the current order of commits is easer to keep up to date with master.

I think it is possible to do more of this as a scripted diff

I’m not entirely sure how to rebase on top of your changes, or how to combine it.

My changes are more complete. For example I’ve changed qt/forms around too, as well as all the includes in src/qt. I think I completely switched over everything.

I’ve cherry-picked the developer notes change, thanks.

jonasschnelli commented at 6:27 pm on August 15, 2017: contributor

Great. utACK cbe3ac57583a683a82a05ab93dec1fb4877ba5c4

laanwj commented at 7:11 pm on August 15, 2017: member

@ryanofsky I’ve adopted your script a bit and now it gets even closer:

0  base=${f%/*}/ relbase=${base#src/} sed -i "s:#include \"\(.*\)\"\(.*\):if test -e \$base'\\1'; then echo '#include <'\$relbase'\\1>\\2'; else echo '#include <\\1>\\2'; fi:e" $f

laanwj force-pushed on Aug 15, 2017

laanwj commented at 7:28 pm on August 15, 2017: member

Changed the base commit to modified @ryanofsky’s script. The rest of the changes is much smaller now.

Also the individual commits should build because I moved the -I build system change to the end.

scripted-diff: Replace #include "" with #include <>

-BEGIN VERIFY SCRIPT-
for f in \
  src/*.cpp \
  src/*.h \
  src/bench/*.cpp \
  src/bench/*.h \
  src/compat/*.cpp \
  src/compat/*.h \
  src/consensus/*.cpp \
  src/consensus/*.h \
  src/crypto/*.cpp \
  src/crypto/*.h \
  src/crypto/ctaes/*.h \
  src/policy/*.cpp \
  src/policy/*.h \
  src/primitives/*.cpp \
  src/primitives/*.h \
  src/qt/*.cpp \
  src/qt/*.h \
  src/qt/test/*.cpp \
  src/qt/test/*.h \
  src/rpc/*.cpp \
  src/rpc/*.h \
  src/script/*.cpp \
  src/script/*.h \
  src/support/*.cpp \
  src/support/*.h \
  src/support/allocators/*.h \
  src/test/*.cpp \
  src/test/*.h \
  src/wallet/*.cpp \
  src/wallet/*.h \
  src/wallet/test/*.cpp \
  src/wallet/test/*.h \
  src/zmq/*.cpp \
  src/zmq/*.h
do
  base=${f%/*}/ relbase=${base#src/} sed -i "s:#include \"\(.*\)\"\(.*\):if test -e \$base'\\1'; then echo \"#include <\"\$relbase\"\\1>\\2\"; else echo \"#include <\\1>\\2\"; fi:e" $f
done
-END VERIFY SCRIPT-

693d942532

qt: refactor: Use absolute include paths in .ui files da98724516

qt: refactor: Changes to make include paths absolute

This makes all include paths in the GUI absolute.

Many changes are involved as every single source file in
src/qt/ assumes to be able to use relative includes.

7216380eb3

test: refactor: Use absolute include paths for test data files 5c2de94c8e

refactor: Include obj/build.h instead of build.h e34cdbb4f7

build: Remove -I for everything but project root

Remove -I from build system for everything but the project root,
and built-in dependencies.

641d4c570a

Recommend #include<> syntax in developer notes 895330f2e3

in src/qt/test/compattests.cpp:5 in 90fe23a29c outdated

1@@ -2,11 +2,11 @@
2 // Distributed under the MIT software license, see the accompanying
3 // file COPYING or http://www.opensource.org/licenses/mit-license.php.
4 
5-#include "paymentrequestplus.h" // this includes protobuf's port.h which defines its own bswap macos

ryanofsky commented at 8:05 pm on August 15, 2017:

The include is being lost here because of the apostophe in the comment. I think replacing echo '...' with echo \"...\" in the script should fix it.

laanwj commented at 6:48 am on August 16, 2017:

Seems to work, apparently we’re nowhere using " in comment after include :-)

laanwj force-pushed on Aug 16, 2017

ryanofsky commented at 7:14 am on August 16, 2017: member

ACK 895330f2e31d6a5cdd68952ae3b35f9089703959. I confirmed all the individual commits in the branch compile. This looks great! I had no idea all those obj and qt include paths were even there, and I’m glad you were able to get rid of them.

promag commented at 7:17 am on August 16, 2017: member

641d4c5 👏 utACK 895330f.

theuni commented at 0:01 am on August 17, 2017: member

This makes me nervous without a subdir-as-a-namespace. gcc (i assume most others as well) searches -I’s left-to-right, meaning that something with a generic name like “config.h” could end up including something from the system if we manage to add our “-I(builddir)” after some other path.

Testing with include <amount.h> for example, I was able to change MAX_MONEY by copying amount.h to /usr/include, and (to mimic an accidental ordering bug) re-arranging the -I’s on the compile line. Trying that with include "amount.h" it at least got confused by the clashing files due to the primitives subdir.

Granted, that’s pretty paranoid.

Much more likely if there was an ordering bug though, would be a compilation failure. A subdir would at least save us from those. Would it make sense to use -I$(top_builddir) and #include <src/amount.h> as a precaution?

laanwj commented at 7:04 am on August 17, 2017: member

This makes me nervous without a subdir-as-a-namespace. gcc (i assume most others as well) searches -I’s left-to-right, meaning that something with a generic name like “config.h” could end up including something from the system if we manage to add our “-I(builddir)” after some other path.

But isn’t that true for "" as well? The only difference is that "" tries relative to the current directory first. It does not (IIRC) reverse -I order. In the example that I give in the OP, primitives/block.h includes "primitives/transaction.h", it will look in src/primitives/primitives, failing that it goes down the entire -I chain, to finally try in src/. So that would find /usr/include/primitives/transaction.h first.

Duplicated include files can lead to more confusion with relative includes as it can be ambiguous which one is included. At least with -I you can just follow the include path when figuring out the issue.

Much more likely if there was an ordering bug though, would be a compilation failure. A subdir would at least save us from those. Would it make sense to use -I$(top_builddir) and #include <src/amount.h> as a precaution?

I think on the longer run we should make sure all source files are in directories under src/, not src itself. Many bigger open source projects seem to have this convention.

I really dislike using top_builddir as an intermediate step though. It’d work, but feels dirty to add src/ before each include. I’d rather just close this.

Regarding active paranoia, someone could still put a src/amount.h in your include path and it will take precedence.

Only insanely disciplined relative inclusion (using explicit ./ and ../) could avoid that “attack”, I think. That’s… crazy though :( In any case if your include path is compromised, there are lots of other ways to create harm.

laanwj commented at 7:11 am on August 17, 2017: member

Would it make sense to use -I$(top_builddir) and #include <src/amount.h> as a precaution?

Do you know of any projects that do this?

Closing this for now, on further thought, this creates more issues than it solves I guess.

laanwj closed this on Aug 17, 2017

ryanofsky commented at 9:28 am on August 17, 2017: member

Closing this for now, on further thought, this creates more issues than it solves I guess.

Is this only being closed because of the issue theuni raised? Or are there other problems?

My earlier scripted diff (branch) is neutral with respect to the issue theuni is talking about because it leaves includes that are relative to the source directory intact, and only modifies includes that rely on the include path from <> to “”. So for example, if an amount.h were added to an prepended include path, nothing different would happen than already happens now.

More broadly though, it is hard to imagine taking this issue seriously, because the project already uses a mix of <> and "" includes, and all of the existing <> includes (as well as nearly all the "" includes in subdirectories below src/) are already relying on the configured include paths. And as laanwj pointed out this change is only removing include paths, not adding any, so it is strictly decreasing whatever ambiguities there may be in interpreting #include directives compared to what we are doing now.

laanwj commented at 10:47 am on August 17, 2017: member

Is this only being closed because of the issue theuni raised? Or are there other problems?

Because of the issue theuni raised. I don’t see any way out of that, he’s right and we’re already screwed, and this doesn’t make it better (though it does make things more consistent, if they mess up! I have to stand by that. You won’t have file A including the right copy and file B including the wrong one, for instance.).

So for example, if an amount.h were added to an prepended include path, nothing different would happen than already happens now.

I did think about that, and that would be true if we weren’t using subdirectories ourselves. However as soon as we #include "amount.h" in one of the subdirectories of src, the same thing happens! This is what I mean with “insanely disciplined relative inclusion” two posts back. #include "../amount.h" would be unambiguous (if you do this consistently, I guess you could even remove -I completely for the project itself).

promag commented at 11:00 am on August 17, 2017: member

In Qt I think each module uses "" for self headers and <> for other module headers and everything else. But public headers always use <>.

laanwj commented at 11:05 am on August 17, 2017: member

if you do this consistently, I guess you could even remove -I completely for the project itself

I’m somewhat starting to like this idea. Make all includes relative and correct, then lose -I. What are the drawbacks?

Edit: To answer this myself, I guess things can become ambiguous if you have chains of include files including each other. E.g. "primitives/primitives.h" including "../amount.h" which includes "util.h". Which util.h would be included? Darn.

But public headers always use <>.

Yep, and we don’t have any public headers besides bitcoinconsensus.h.

theuni commented at 12:10 pm on August 17, 2017: member

@laanwj I think it’s too early to close this. I was thinking outloud about any potential drawbacks, but that’s not to say that this creates more problems than it solves. As you mentioned, both of those potential issues are very unlikely.

ryanofsky commented at 12:33 pm on August 17, 2017: member

As you mentioned, both of those potential issues are very unlikely.

Could someone state clearly what two potential issues this PR causes? I think I might have missed that there is more than one issue…

theuni commented at 12:54 pm on August 17, 2017: member

@ryanofsky

Potentially causing compilation issues with generic header names (util.h for example) if -I$(builddir) isn’t first on the list. This is already an issue due to source files in subdirs, but the likelihood increases with this change.
Due to 1., potentially making it easier for someone in a shared dev environment to hijack a header with no indication. I don’t think this is realistic to worry about because there are so many worse attack vectors. This was mainly just a thought experiment for testing 1.

Again, I’m not at all convinced that those things are real issues. Just thinking through them.

ryanofsky commented at 1:14 pm on August 17, 2017: member

Thanks! Just to give my opinion, I think any of the following changes would be simple and minimally disruptive improvements on the status quo:

A change using -Isrc and #include<full_path> everywhere that would make the build slightly more vulnerable to theuni’s issues than we already are, but would make it more obvious what file is being included in the normal case where you don’t have a messed up build configuration. This is implemented in the current PR.
A change dropping -Isrc and using #include "../rel_path" everywhere that would prevent the issues raised by theuni in present and future code. This should not be hard to implement with a variation of laanwj’s scripted diff.
A change that would not alter current behavior at all, but just improve readability by replacing current hodgepodge of #include "rel_path" and #include "full_path" with #include "rel_path" and #include <full_path>, to make it clear when the include path is supposed to be interpreted as a relative path, and when it’s supposed to be interpreted as a full path. My pr/inc branch basically does this already but it would need a tweak to handle bitcoin-config.h.

Slight preference for (1) over (2) over (3), but I think any of these could be good improvements and there may be other simple solutions as well (like using -iquote or -I-).

If more disruptive changes are ok, it’d also be nice to move headers in the src/ root to appropriate subdirectories. But I think that type of change would be better to handle separately and should not block simple improvements like the one implemented in this PR.

laanwj reopened this on Aug 24, 2017

ryanofsky commented at 5:55 pm on October 12, 2017: member

This has acks from me, jonas, and promag, as well as concept acks from jnewbery and practicalswift and cory withdrew his objection. @laanwj can this be rebased and merged?

MarcoFalke commented at 6:55 pm on November 8, 2017: member

I think this is open for grabs

ryanofsky commented at 8:35 pm on November 9, 2017: member

I think this is open for grabs

What does this mean? Maybe there should be a tag for PRs where author wants someone to take over or needs help with something.

MarcoFalke commented at 9:28 pm on November 9, 2017: member

Ah. I think I meant “this is up for grabs”.

meshcollider commented at 1:27 am on November 10, 2017: contributor

I’ll rebase and open a new PR, I’m happy to maintain it since it already has so many ACKs

fanquake commented at 2:02 am on November 10, 2017: member

Closing now that #11651 is open.

fanquake closed this on Nov 10, 2017

laanwj referenced this in commit 3c098a8aa0 on Nov 16, 2017

DrahtBot locked this on Sep 8, 2021

refactor: Make all #includes relative to project root #11053