#612 change paths to relative

[gaida-exe]

change path in openvdb from #include <openvdb/..> to #include ".." add target_include_directories for executables

[linux-foundation-easycla[bot]]

The committers listed above are authorized under a signed CLA.

• :white_check_mark: login: gaida-exe / name: Sebastian Gaida (8f211a2ebc448dd27c1cc7f6ebe5d8841f650cb3)

[gaida-exe]

Hey, @danrbailey since you were involved in #612 I wanted to ask if this PR is correct or what should be changed. I shortened the paths like you suggested, from #includes <openvdb/math/Math.h> to #include "Math.h". I added target_include_directories to some cmake files, to be able to include without a ../. Also, all includes could be done without the openvdb folder #includes "openvdb/math/Math.h" -> #includes "math/Math.h", could this cause any problems? It was used like that in points.cc.

[danrbailey]

This looks pretty good to me. I've approved running our CI on this PR so we can see if there are any issues uncovered through doing this on different platforms.

The main thing I would suggest changing is to revert the changes to cases where we are using angle brackets (<>) for headers that are located outside of the file's directory as stated in our coding style. For example, openvdb_print.cc should continue to use angle brackets to include headers from the core openvdb library. In essence, removing all the CMake target_include_directories(XXX PRIVATE ../.) changes and fixing all the resulting errors should resolve this.

[gaida-exe]

I reverted the changes and the build failed. The problem was, that includes from other cmake files/executables did not find shortend paths like #include "openvdb.h" instead they need a #include "openvdb/openvdb.h". After I changed every file to that signature, everything build fine again.

[gaida-exe]

I looked it all through and resolved all the issues you had. Also changed the #include <openvdb/version.h> to #include "openvdb/version.h".

[danrbailey]

Thanks @gaida-exe, this looks pretty close to being ready to merge. Should we move this out of being a draft PR now?

[gaida-exe]

Sure, if you have no other suggestions.

[e4lam]

Sorry, I'm late to the game but I'm going to voice my dissent here anyways. First change I see is in openvdb/Exceptions.h which got <openvdb/version.h> modified to "openvdb/version.h" instead. But since there's no openvdb/openvdb/version.h, this would just go back to -I compiler options anyways. If the intention here was actually to change to relative paths, then it should become "version.h"

[Idclip]

Sorry, I'm late to the game but I'm going to voice my dissent here anyways. First change I see is in openvdb/Exceptions.h which got <openvdb/version.h> modified to "openvdb/version.h" instead. But since there's no openvdb/openvdb/version.h, this would just go back to -I compiler options anyways. If the intention here was actually to change to relative paths, then it should become "version.h"

Is this the only issue you see or are you disagreeing with this PR more generally? This is easily fixable by tweaking the CMake include paths (which are needed as version.h is auto-generated)

[e4lam]

Is this the only issue you see or are you disagreeing with this PR more generally? This is easily fixable by tweaking the CMake include paths (which are needed as version.h is auto-generated)

I think I disagree with this change generally until pending a review of that these changes are doing what we actually intend on. I'm not entirely sure what "tweaking CMake include paths" has to do with how openvdb is consumed on the library client side, which is what these include changes affect.

[gaida-exe]

I've made the suggested change that @e4lam pointed out (in openvdb/Exceptions.h). But I'll set the PR back to draft, until the discussion is resolved.

[Idclip]

I think I disagree with this change generally until pending a review of that these changes are doing what we actually intend on. I'm not entirely sure what "tweaking CMake include paths" has to do with how openvdb is consumed on the library client side, which is what these include changes affect.

We've reviewed what these changes intend... right? We just missed this one. version.h is special because, at build time, it has to be pulled from the binary directory. Obviously once it's installed it should be included as "version.h" from any files that live next to it (Exceptions/Types/etc.). But the current CMake include paths (the paths configured when building vdb) pull it from a parent folder, not from the folder it lives in - edit: so we should change this.

[Idclip]

I've made the suggested change that @e4lam pointed out (in openvdb/Exceptions.h). But I'll set the PR back to draft, until the discussion is resolved.

You may find that simply changing this causes VDB not to build. You may need to add an additional include path to ${CMAKE_CURRENT_BINARY_DIR}/openvdb/openvdb

[jmlait]

The goal, I think, is to handle a situation where openvdb headers exist on /usr/local/include, so are in either -Isystem or -I path. However, in my code, I want to include one particular bespoke openvdb library.

For concreteness, say I do this by a direct absolute path include without using path search: #include "/opt/mystuff/openvdb_special/openvdb/openvdb/tools/Merge.h"

This will then in turn hit: #include "openvdb/tree/NodeManager.h"

Because the current directory of Merge.h is openvdb/tools, there is no openvdb/tools/openvdb/tree, so the relative path lookup fails. #include then devolves into a system path search and I'll pick up the system library. I now have a mixture of system and special libraries being used.

Alternatively, if it has #include "../tree/NodeManager.h" I will get the correct /opt version of all the dependencies.

I think this means, as a rule, that if one uses "" one wants the relative path to ALWAYS succeed.

The down side is you can't patch VDB by adding a Merge.h to your -I path that doesn't have the rest of the openvdb library present, as all the relative searching will fail and ../tree isn't on the global search path.

I recall there being some discussion that -Isystem and -I behave differently for "" and <> but I can't find reference to that, both seem to be searched for both. The difference is that -I always pre-empts -Isystem in searches.

[e4lam]

We've reviewed what these changes intend... right? We just missed this one. version.h is special because, at build time, it has to be pulled from the binary directory.

But that doesn't seem to be what happened in plenty of other cases that I see in the change. To pick another random example, we have openvdb/math/ConjGradient.h including "openvdb/Exceptions.h" which has the exact same issue. If the intention is to change to use relative paths, then I'd expect the change to be "../Exceptions.h". As it stands, there's lot of changes here that have no new effect with the switch to double-quotes, in which case, we might as well have kept them as angle brackets. What am I missing?

[gaida-exe]

@danrbailey pointed out that these changes with "../" should not be made, because of the style-guide mentioned here. I could revert the changes, where the correct relative path would include a "../", since it apparently has no effect.

[gaida-exe]

I've made the suggested change that @e4lam pointed out (in openvdb/Exceptions.h). But I'll set the PR back to draft, until the discussion is resolved.

You may find that simply changing this causes VDB not to build. You may need to add an additional include path to ${CMAKE_CURRENT_BINARY_DIR}/openvdb/openvdb

If you mean the change that I did in 92f2779 . It compiled in my build before I pushed it and also some files in master use this type of include like here.

[e4lam]

If we can't have upwards relative include paths, then I don't see why we should make any of these changes at all since it defeats the purpose of changing to use include double-quotes in the first place. Perhaps, I don't understand the motivation for this PR?

[Idclip]

If we can't have upwards relative include paths, then I don't see why we should make any of these changes at all since it defeats the purpose of changing to use include double-quotes in the first place. Perhaps, I don't understand the motivation for this PR?

Yeah sorry I think using the term "relative include paths" has become missleading with the objective of this PR. There are three main scenarios we're dealing with:

• 1 Building OpenVDB and its components all from source
• 2 Building some OpenVDB components from source against an existing OpenVDB installation
• 3 Building a downstream codebase against an existing installation of OpenVDB

These changes aim to address issues with 1 and 2 and should not really impact 3 (unless you have multiple installations of OpenVDB which are included both through -I and -isystem).

Lets say you have an existing installation of OpenVDB and TBB in /usr/local/include and /usr/local/lib and you end up with a compile command that looks like this:

c++ -I/path/to/source/openvdb -isystem /usr/local/include -o some_vdb_object.o -c some_vdb_source.cc

GCC (and afaik clang) treat quotes and bracketed includes differently when the path is given with -I or -isystem. Headers quoted "" will first be searched for in the same directory the current source resides. This is improved in a few cases, but as you point out doesn't apply to cases where a file in /tool includes something like `"openvdb/version.h". However, if a path is given with -I then this causes the path to be searched after the current directory and AHEAD of the standard system directories. But this is only true for the quoted form of #include. The bracketed version always searches system paths first i.e. /usr/local/include.

We're really missusing the term "relative" here as I think the main object of this PR is to simply force the compiler to read from -I directories before -isystem for openvdb includes when building openvdb. Downstream software can use -I or -isystem when including vdb headers and should notice any difference (the only percievable difference I'm aware of would be if you have two installations of OpenVDB which both end up on the include path - this PR will change indirect headers to be picked up from the -I one over -isystem).

Either way if there is concern this doesn't have to go in to 9.1. And it's late here now so I may be entirely wrong, I'll re-read this all tomorrow 👍

[e4lam]

However, if a path is given with -I then this causes the path to be searched after the current directory and AHEAD of the standard system directories. But this is only true for the quoted form of #include. The bracketed version always searches system paths first i.e. /usr/local/include.

As far as I can tell in my test (attached here inctest.tar.gz), regardless of whether the include uses quotes or angle brackets, the -I options on gcc are always searched BEFORE -isystem paths:

On my cygwin gcc 11, doing source run.sh gives:

g++ (GCC) 11.2.0
Copyright (C) 2021 Free Software Foundation, Inc.
This is free software; see the source for copying conditions.  There is NO
warranty; not even for MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.

=== gcc -isystem /tmp/test/inc2 -I /tmp/test/inc1 -c -o foo.o src/foo.cpp
In file included from src/foo.cpp:1:
/tmp/test/inc1/openvdb/subdir/file.h:1:17: note: ‘#pragma message: inside inc1/openvdb/subdir/file.h’
    1 | #pragma message "inside inc1/openvdb/subdir/file.h"
      |                 ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
In file included from /tmp/test/inc1/openvdb/subdir/file.h:2,
                 from src/foo.cpp:1:
/tmp/test/inc1/openvdb/version.h:1:17: note: ‘#pragma message: inside inc1/openvdb/version.h’
    1 | #pragma message "inside inc1/openvdb/version.h"
      |                 ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
=== g++ -I /tmp/test/inc1 -isystem /tmp/test/inc2 -c -o foo.o src/foo.cpp
In file included from src/foo.cpp:1:
/tmp/test/inc1/openvdb/subdir/file.h:1:17: note: ‘#pragma message: inside inc1/openvdb/subdir/file.h’
    1 | #pragma message "inside inc1/openvdb/subdir/file.h"
      |                 ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
In file included from /tmp/test/inc1/openvdb/subdir/file.h:2,
                 from src/foo.cpp:1:
/tmp/test/inc1/openvdb/version.h:1:17: note: ‘#pragma message: inside inc1/openvdb/version.h’
    1 | #pragma message "inside inc1/openvdb/version.h"
      |                 ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
=== gcc -isystem /tmp/test/angle2 -I /tmp/test/angle1 -c -o foo.o src/foo.cpp
In file included from src/foo.cpp:1:
/tmp/test/angle1/openvdb/subdir/file.h:1:17: note: ‘#pragma message: inside angle1/openvdb/subdir/file.h’
    1 | #pragma message "inside angle1/openvdb/subdir/file.h"
      |                 ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
In file included from /tmp/test/angle1/openvdb/subdir/file.h:2,
                 from src/foo.cpp:1:
/tmp/test/angle1/openvdb/version.h:1:17: note: ‘#pragma message: inside angle1/openvdb/version.h’
    1 | #pragma message "inside angle1/openvdb/version.h"
      |                 ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
=== g++ -I /tmp/test/angle1 -isystem /tmp/test/angle2 -c -o foo.o src/foo.cpp
In file included from src/foo.cpp:1:
/tmp/test/angle1/openvdb/subdir/file.h:1:17: note: ‘#pragma message: inside angle1/openvdb/subdir/file.h’
    1 | #pragma message "inside angle1/openvdb/subdir/file.h"
      |                 ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
In file included from /tmp/test/angle1/openvdb/subdir/file.h:2,
                 from src/foo.cpp:1:
/tmp/test/angle1/openvdb/version.h:1:17: note: ‘#pragma message: inside angle1/openvdb/version.h’
    1 | #pragma message "inside angle1/openvdb/version.h"
      |                 ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~

And the docs https://gcc.gnu.org/onlinedocs/gcc/Directory-Options.html bear out what I see:

You can specify any number or combination of these options on the command line to search for header files in several directories. The lookup order is as follows:

1. For the quote form of the include directive, the directory of the current file is searched first.
2. For the quote form of the include directive, the directories specified by -iquote options are searched in left-to-right order, as they appear on the command line.
3. Directories specified with -I options are scanned in left-to-right order.
4. Directories specified with -isystem options are scanned in left-to-right order.
5. Standard system directories are scanned.
6. Directories specified with -idirafter options are scanned in left-to-right order.

So as far as I can tell, we can merrily go along right now without this PR and build with the OpenVDB lib/components from source as long as the we use -I for the include directory that we want instead of -isystem.

[e4lam]

PS. I can't seem to edit my comment above but you'll need to rename the decompressed inctest.tar.gz to /tmp/test to use run.sh, or else you'll need to modify the paths accordingly.

[Idclip]

@e4lam thanks very much for the tests. I thought this behaviour was explicit with default system dirs, not necessarily dirs given with -isystem i.e:

> gcc -E -Wp,-v -

#include "..." search starts here:
#include <...> search starts here:
 /usr/lib/gcc/aarch64-linux-gnu/9/include
 /usr/local/include
 /usr/include/aarch64-linux-gnu
 /usr/include

But I'm also unable to produce the behaviour I was describing! I've been using these docs: https://gcc.gnu.org/onlinedocs/cpp/Search-Path.html

The most commonly-used option is -Idir, which causes dir to be searched after the current directory (for the quote form of the directive) and ahead of the standard system directories.

I think I misinterpreted the parenthesised clause, but was confident in that interpretation as I was sure that I'd experienced this behaviour before. I was perhaps muddling it with other platforms/compiler options (-I-, -iquote etc).

It does seem that MSVC at least treats quotes in a potentially more desriable way:

https://docs.microsoft.com/en-us/cpp/preprocessor/hash-include-directive-c-cpp?view=msvc-170

If the #include directive is specified using double-quote form, it first searches local directories. The search begins in the same directory as the file that contains the #include directive. If it fails to find the file, it searches next in the directories of the currently opened include files, in the reverse order in which they were opened. The search begins in the directory of the parent include file and continues upward through the directories of any grandparent include files.

In any case we should revisit this. I'm not convinced that just because relative includes are disallowed by the "style guide" that we shouldn't use them and so doing would make changing to the quoted form more relevant.

Also:

I'm not entirely sure what "tweaking CMake include paths" has to do with how openvdb is consumed on the library client side, which is what these include changes affect.

It seems that we've ultimately come to the conlusion that this change won't change anything for clients.

[e4lam]

It seems that we've ultimately come to the conclusion that this change won't change anything for clients.

As Jeff noted above, the only potential advantage of using quote includes with relative paths if some client needs to directly include and use different OpenVDB versions within the same project. And I thought that was the motivation for this PR. If it's for building OpenVDB, then I don't see any advantage for this change.

FWIW, I'd like to point out that Boost remains in the camp of using angle bracket includes when referencing other boost libraries. First random result I found:

• https://github.com/boostorg/thread/blob/1de55fceda1829fa3b29fcfd4efe8f94de0a01d1/include/boost/thread/thread_time.hpp#L9

[Idclip]

Yes I just wanted to clarify that, as it stands, this change should have absolutely no impact - contrary to what was originally implied.

FWIW, I'd like to point out that Boost remains in the camp of using angle bracket includes when referencing other boost libraries. First random result I found:

Clang/LLVM uses quotes. But I don't think we should arbitrarily pick repos to use as a reference as none of this is standardized. It's whatever makese the most sense for the project.

[gaida-exe]

#1418