Bugzilla – Bug 2002
Hardcoded include paths cause breakage
Last modified: 2015-01-29 22:20:55 EST
Created attachment 1898 [details] Patch Several internet tests include header which are not normally exported by waf, i.e aren't public. For example, src/internet/test/tcp-header.cc: #include "../src/internet/model/tcp-option-rfc793.h" This assumes that waf is building in $(hg root)/build. This is not guaranteed, since waf supports building into other directories, using the --out option. The simplest patch I can think of is just to export those headers. In src/internet/wscript:253: 'model/tcp-option-rfc793.h', 'model/tcp-option-winscale.h', 'model/tcp-option-ts.h', Is there any reason these *shouldn't* be exported?
(In reply to Peter Barnes from comment #0) > Created attachment 1898 [details] > Patch > > Several internet tests include header which are not normally exported by > waf, i.e aren't public. For example, src/internet/test/tcp-header.cc: > > #include "../src/internet/model/tcp-option-rfc793.h" > > This assumes that waf is building in $(hg root)/build. This is not > guaranteed, since waf supports building into other directories, using the > --out option. > > The simplest patch I can think of is just to export those headers. In > src/internet/wscript:253: > > 'model/tcp-option-rfc793.h', > 'model/tcp-option-winscale.h', > 'model/tcp-option-ts.h', > > Is there any reason these *shouldn't* be exported? These are not being exported because they are not intended as part of the public API. I agree the include path is hackish and something better is desirable. While I don't have any major issue with exporting these in particular, my broader concern is that we will stop distinguishing between public and private headers if we make everything public to work around these issues, and later, if/when we start publishing libraries and development headers, this distinction will have been lost. So I wonder whether there should be a second directory besides 'build/ns3' for private headers to be copied to, and they could be included in tests as e.g. 'build/ns3-private/tcp-option-ts.h'. I looked in the Waf book and in search engines for some guidance on this topic and couldn't find any; it may be a bit Waf-specific.
I agree on not exporting "semi-private" headers. On the other hand, the current situation leaves me dead in the water. To manage architecture-dependent builds we use the --out to create build directories for each architecture. Right now the build fails unless we disable tests, *all* tests. One way to leverage --out *and* track semi-private api would be to install them in out_dir/ns3/private or out_dir/ns3/test-only
Created attachment 1900 [details] patch for private includes This seems to work; places private headers in 'build/ns3-private' (other directory names are possible). It works with the waf --out argument. caveats: - I had to add a ns3-private/_placeholder_ to get waf to create this directory. This seems crufty but I couldn't find a better way today, so I copied Gustavo's original approach for ns3/_placeholder_. I wonder if ns3/_placeholder_ and friends could be auto-generated early in the waf configure process, and removed by waf clean/distclean. - Any private headers can't include other public headers without prefix ns3 since they are now in a different directory. Therefore, for example, tcp-option-ts.h has to #include "ns3/tcp-option.h" instead of #include "tcp-option.h"
> caveats: ... also, I think the patch could be slightly tweaked to make the headers land in ns3/private if that is desirable from a doxygen perspective
Would ns3/private obviate the need for ns3-private/_placeholder_? (I assume it would become ns3/private/_placeholder_) I would prefer this b/c it wouldn't add anything to the root directory. Agreed that we shouldn't need these placeholders at all; waf and/or wscript should be able to create the necessary directories automatically.
Patch works for me.
Created attachment 1928 [details] Obviate the need for ns3/_placeholder_ in the repo
The previous patch obviates the need for ns3/_placeholder_ at the top level of the repo. Instead, build/ns3 is created as needed.
(In reply to Peter Barnes from comment #8) > The previous patch obviates the need for ns3/_placeholder_ at the top level > of the repo. Instead, build/ns3 is created as needed. This behavior seems to have been introduced in waf-1.6; patch looks good to me. For the other patch, it seems like the main thing left to decide is whether the private headers go into build/ns3-private or build/private. I don't care strongly; any preference?
Created attachment 1929 [details] Complete solution to private headers This patch removes the two placeholder directories, ns3/_placeholder_ ns3-private/_placeholder_ from the top level of the repo. Instead, assuming the build directory is "build", waf creates build/ns3 build/ns3/private and places the appropriate public and private includes there. The internet/test/tcp-... tests have had the affected includes updated to #include "ns3/private/..."
(In reply to Peter Barnes from comment #10) +1, I checked to see what boost libraries typically do and they organize private headers in a subdirectory like this, such as 'detail/', 'cpp_int/' or 'aux_/'. I suggest to plan to apply this next week if no further comments.
+1, works for me. T. (In reply to Tom Henderson from comment #11) > (In reply to Peter Barnes from comment #10) > > > +1, I checked to see what boost libraries typically do and they organize > private headers in a subdirectory like this, such as 'detail/', 'cpp_int/' > or 'aux_/'. > > I suggest to plan to apply this next week if no further comments.
pushed this patch in changeset 11183:52ff59697ec9