BobBuildTool/bob

wrong path joining in common generator

Closed this issue · 14 comments

mahaase commented

bob/pym/bob/generators/common.py

Line 329 in bd03a97

self.appendIncludeDirectories.append(os.path.join(e,root))

hej, i tested this line with absolute and relative paths for linux and MSYS2.
in both platforms there are scenarios, where this line fails the result:

-I ./dev/build/fdt
e.g. result:

e=./dev/build/fdt
root=./dev/build/fdt/ros/datalogging-tgt/x86_64-cross-linux-gnu/1/workspace/build/fdt.ros.datalogging/CMakeFiles/fdt.ros.datalogging.dir/src
join=./dev/build/fdt/./dev/build/fdt/ros/datalogging-tgt/x86_64-cross-linux-gnu/1/workspace/build/fdt.ros.datalogging/CMakeFiles/fdt.ros.datalogging.dir/src

same, if i use -I dev/build/fdt. so relative paths always fail.
absolute paths for linux are okay, e.g.: -I $PWD/dev/build/fdt

result:

e=/home/mhaase/ws/fdt.bobrecipes/dev/build/fdt
root=/home/mhaase/ws/fdt.bobrecipes/dev/build/fdt/ros/datalogging-tgt/x86_64-cross-linux-gnu/1/workspace/build/fdt.ros.datalogging/CMakeFiles/fdt.ros.datalogging.dir/src
join=/home/mhaase/ws/fdt.bobrecipes/dev/build/fdt/ros/datalogging-tgt/x86_64-cross-linux-gnu/1/workspace/build/fdt.ros.datalogging/CMakeFiles/fdt.ros.datalogging.dir/src

but the join here is also useles, because the result is always the same as root.

for windows absolute paths all fail for all path-variants (unix, windows, mixed):
e.g.:

e=D:/msys64/home/mhaase/ws/fdt.bobrecipes/dev/build/fdt
root=D:/msys64/home/mhaase/ws/fdt.bobrecipes/dev/build/fdt/vcar-dev/x86_64-pc-win32/1/workspace/install/usr/lib/cmake/fdt.vcar
join=D:/msys64/home/mhaase/ws/fdt.bobrecipes/dev/build/fdt/D:/msys64/home/mhaase/ws/fdt.bobrecipes/dev/build/fdt/vcar-dev/x86_64-pc-win32/1/workspace/install/usr/lib/cmake/fdt.vcar

As i tested, it is important, that the path is always a absolute one!

mahaase commented

bob/pym/bob/generators/QtCreatorGenerator.py

Line 305 in bd03a97

hList.append(os.path.join(e,root))

the same "issues" here, if we use qt-creator for MSYS!

e=D:/msys64/home/mhaase/ws/fdt.bobrecipes/dev/build/fdt
root=D:/msys64/home/mhaase/ws/fdt.bobrecipes/dev/build/fdt/vcar-dev/x86_64-pc-win32/1/workspace/install/usr/lib/cmake/fdt.vcar
join=D:/msys64/home/mhaase/ws/fdt.bobrecipes/dev/build/fdt/D:/msys64/home/mhaase/ws/fdt.bobrecipes/dev/build/fdt/vcar-dev/x86_64-pc-win32/1/workspace/install/usr/lib/cmake/fdt.vcar
mahaase commented

a solution for me, would be to replace: os.path.join(e,root) by os.path.abspath(root)

for linux, absolute and relative paths are working, for MSYS2 the the user have to use this: -I $(pwd -W)/dev/build/... (works for qtcreator and vs2019)

jkloetzke commented

The line is just wrong for what it tries to accomplish. Changing it to os.path.abspath(root) looks like the correct solution. Interestingly the code in question was introduced silently in e9ec42a. Before this commit the include directory was added as-is.

I'm wondering why we would ever need to recursively add these additional include directories. @rhubert do you remember the purpose of this recursive approach? It somewhat feels wrong...

rhubert commented

Not exactly... I think the QT-creator needs to know all sub directories for the indexer to be happy. Not sure if this is still the case for a up-to-date version. The commit is >5 years old...

Before e9ec42a the include-dir-list was build of all directories containing a header-file within the checkout-dirs. With -I you can specify a directory outside of the bob-known world which you want to have in the index to and this is also added recursively. Not sure what the usecase of this was. Maybe some host-builds where you want to have /usr/include.

@mahaase why do you need to add the build directory? All packaged dep headers should be found?

I think it's simply not supported to add directories relative to the bob-workspace. Adding them absolute -I $(pwd)/dev/build/fdt should work - at least for unix. I don't know what the windows issue is as a don't know what

e=D:/msys64/home/mhaase/ws/fdt.bobrecipes/dev/build/fdt
root=D:/msys64/home/mhaase/ws/fdt.bobrecipes/dev/build/fdt/vcar-dev/x86_64-pc-win32/1/workspace/install/usr/lib/cmake/fdt.vcar
join=D:/msys64/home/mhaase/ws/fdt.bobrecipes/dev/build/fdt/D:/msys64/home/mhaase/ws/fdt.bobrecipes/dev/build/fdt/vcar-dev/x86_64-pc-win32/1/workspace/install/usr/lib/cmake/fdt.vcar

is at all...

If qt-creator still needs all directories, the solution might be:

os.path.abspath(os.path.join(e,root))

?

mahaase commented

@mahaase why do you need to add the build directory? All packaged dep headers should be found?

use-case 1: using https://cmake.org/cmake/help/latest/module/GenerateExportHeader.html inside the build-directory a *_export.h file will be generated for export header stuff for shared libraries.

use-case 2: inside the source-step, just checking out IDL files. in build-step a generator will create the headerfiles, that also should be resolved in IDE.

especially for VS2019 projects use-case 1 is important, becuase of intellisense will declare the header file defective, if class UNKNOWN_DEFINE Name { exists.

@rhubert the os.path.join(e,root) is a problem for MSYS2. like i debugged and wrote, if e value is D:/msys64/home/mhaase/ws/fdt.bobrecipes/dev/build/fdt and root value is D:/msys64/home/mhaase/ws/fdt.bobrecipes/dev/build/fdt/vcar-dev/x86_64-pc-win32/1/workspace/install/usr/lib/cmake/fdt.vcar, than python will join them to this result:

D:/msys64/home/mhaase/ws/fdt.bobrecipes/dev/build/fdt/D:/msys64/home/mhaase/ws/fdt.bobrecipes/dev/build/fdt/vcar-dev/x86_64-pc-win32/1/workspace/install/usr/lib/cmake/fdt.vcar

that path is of course not working.

mahaase commented

sooo, this issue was just found, because of missing functionality of IDE generators, that the build-directories shall checked for headerfiles, too. would that be another ticket?

rhubert commented

I don't think it's a good idea to add build dirs to the IDE. ATM we add only the src dir of the current package and the dist dirs of the dependencies.
I'd assume you have many duplicated headers if you also all the build-dirs. (rsync $1/ .). For downloaded-deps you don't have a build dir? The only valid build-dir is IMO the build dir of the component the project is generated for.

If you really need this in your setup -I ./dev/build is the way to go..

                    for root, directories, filenames in os.walk(e):
                        filterDirs(directories)
                        self.appendIncludeDirectories.append(os.path.join(e,root))

is just broken. It doesn't make sense to filter the directory list and ignore the result.

 self.appendIncludeDirectories.append(os.abspath(directory)
jkloetzke commented

I think there are multiple issues here. First of all the "add directories recursively code" is broken. The os.path.join(e,root) is boguous. The path in root will already have the prefix path of e. But I question the whole approach because the user should be able to set the directories explicitly. Blindly adding anything below a path is wrong. The user can always give another -I if required.

sooo, this issue was just found, because of missing functionality of IDE generators, that the build-directories shall checked for headerfiles, too. would that be another ticket?

That is not so simple. Just adding random directories with header files in the build workspace will only create confusion. There are again two things to consider. IDL generators should run in the src workspace and together with #453 it should be possible to properly implement that. Regarding the generated CMake export headers I had the idea years ago to add tool specific scanners in the common code:

bob/pym/bob/generators/common.py

Line 142 in bd03a97

GenericScanner # must be last as "catch all" last resort

Some CMake specific code could search for special include paths but I never took the idea further.

mahaase commented

I don't think it's a good idea to add build dirs to the IDE. ATM we add only the src dir of the current package and the dist dirs of the dependencies.

but this is not the current state. we just add src-directories into the project. for the CMake export header stuff, it would be okay to get them from the package directory. the export header stuff is just used for exported public headers, so they must be moved into the package directory.

rhubert commented

sorry - I can't follow you...

mahaase commented

the current generators do not use anything of the dist-dirs (of the dependencies).

jkloetzke commented

The problem with the "dist" directories is that you cannot really edit these files. They will be overwritten on the next build of the respective package. I'm not really sure how to properly handle generated header files. They must be added selectively in a read-only way to the IDE. Finding those in a reliable looks even more challenging...

rhubert commented
daxcore commented

Just collecting the additional include dirs in dist dir would not be super dangerous. the files are not shown in the project-tree/explorer, but intellisense (vs2019) would be happy. of course it is possible to open the include file by IDE technics. also we could filter the files explicitly, like *_export.h... adding more if needed. normally the developers know there projects and the generated files. also file header comments with warnings are possible.