Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

CMake's INCLUDES adds CMAKE_PREFIX_PATH before staging area leading to problems between .H and .cxx files on API changes #982

Open
ggarra13 opened this issue May 26, 2024 · 6 comments
Assignees
Labels
CMake CMake bug or needs CMake support

Comments

@ggarra13
Copy link
Contributor

ggarra13 commented May 26, 2024

Describe the bug
When using system libs, FLTK's CMake scripts are adding CMAKE_PREFIX_PATH/include first before its staging area.
This is a problem as it leads to subsequent compilations to fail when there's a discrepancy in the API.

To Reproduce
fltk_cmake_build_bug.zip

Just run:
./runme.sh

The first compilation will work, the second run (where there was an API changing) without cleaning the directory will fail.
For more info, please refer to the included README.txt file.

Expected behavior
Compilation should not fail. Cmake is smart enough to re-compile only what's needed. The CMAKE_PREFIX_PATH should be added later or the FLTK's FL includes first. There should not be any need to clean the directory.

Screenshots
N/A.

FLTK Version
1.4.0 tags in runme.sh script

FLTK Configure / Build Options
See runme.sh script

Operating System / Platform:
All OSes.

@ggarra13
Copy link
Contributor Author

ggarra13 commented May 26, 2024

I looked into the FLTK file structure a bit, and I see the problem. All of FLTK's built-in dependencies (png, zlib, jpeg, libdecor and nanosvg) are in the same directory as FL directory includes.
The solution might be to move the FL/ directory into its own include/FL directory and change configure, CMakeLists.txt and fltk-config to point to the new location.
Or move FLTK's dependencies to a deps/ directory, so that includes are:
-I FLTK_DIR/FL -I CMAKE_PREFIX_PATH/include -I FLTK_DIR/deps

@ggarra13
Copy link
Contributor Author

An alternative solution that might be easier to implement might be to remove the FL directory from CMAKE_INSTALL_PREFIX/include on an install target.

@Albrecht-S Albrecht-S added the CMake CMake bug or needs CMake support label May 27, 2024
@Albrecht-S
Copy link
Member

@ggarra13 Hi Gonzalo, thanks for the report. I can see the problem but I can't imagine a proper solution in FLTK's build system. The basic problem is that you set CMAKE_PREFIX_PATH to the build's install directory for all builds including all dependencies. IMHO this can't work.

This is not an FLTK problem but an issue of your build procedure. The same problem you are describing would happen if any other dependency of your project would be changed in an incompatible way - and would "see" its own header files of the previous build. This depends on how you set up all dependencies in your build.

If you want to avoid cleaning the build directory before new builds of your project's dependencies, then the solution I'm thinking of is:

  • install all dependencies to different directories in your project's build folder
  • set CMAKE_PREFIX_PATH to the concatenation (a CMake 'list') of the install folders of all dependencies that are built prior to each other (i.e. set it to the install path of ZLIB to build FLTK)
  • set CMAKE_PREFIX_PATH for the final build (your program) to the concatenation of all install paths of all dependencies (in the example case to ZLIB + FLTK).

This way each dependency wouldn't "know" of its own install path and thus wouldn't include outdated headers of a previous build. You could also easily delete the build folders of any dependency it you change the GIT tag of that dependency.

A tiny change in your build setup made it "work" for me. This is only a proof of concept, not the final solution. The only change in my proof of concept is to set CMAKE_PREFIX_PATH for the FLTK build to an empty string. This is not "correct" (it doesn't find the pre-built ZLIB) but it is an evidence that this approach could work for your project. I'm leaving the final solution to you.

diff --git a/SuperBuild/cmake/Build/BuildFLTK.cmake b/SuperBuild/cmake/Build/BuildFLTK.cmake
index f4ec429..46449c6 100755
--- a/SuperBuild/cmake/Build/BuildFLTK.cmake
+++ b/SuperBuild/cmake/Build/BuildFLTK.cmake
@@ -50,7 +50,7 @@ ExternalProject_Add(
     -DCMAKE_MODULE_PATH=${CMAKE_MODULE_PATH}
     -DCMAKE_BUILD_TYPE=${FLTK_BUILD_TYPE}
     -DCMAKE_INSTALL_PREFIX=${CMAKE_INSTALL_PREFIX}
-    -DCMAKE_PREFIX_PATH=${CMAKE_PREFIX_PATH}
+    -DCMAKE_PREFIX_PATH=""
     -DCMAKE_C_FLAGS=${FLTK_C_FLAGS}
     -DCMAKE_CXX_FLAGS=${FLTK_CXX_FLAGS}
     -DCMAKE_INSTALL_MESSAGE=${CMAKE_INSTALL_MESSAGE}

The only other solution I can imagine is to clean the build if you change any of the dependencies, i.e. if you update one of the GIT tags. This should be rare while you develop your own application.

Please let me know if I missed anything or if anything is unclear -- and please close the issue if you agree with my suggestion.

@Albrecht-S Albrecht-S self-assigned this Jun 23, 2024
@ggarra13
Copy link
Contributor Author

@Albrecht-S Hi, Albrecht. Thanks for taking the time with this problem, which is difficult to solve.

This is not an FLTK problem but an issue of your build procedure.

I disagree.

AFAICT, it happens only with FLTK, as it reads dependencies and include files all from the root directory, which is the issue..

Basically, what I imagine FLTK's CMAKE_INCLUDE_PATH or whatever is called (I can't remember how it is named in cmake now) should be:

${FLTK_SOURCE_DIRECTORY_where_FL_and_src_are} {chosen_built_in_dependencies} ${CMAKE_PREFIX_PATH}

So, for example, if FLTK is installed in libs/FLTK (I omit the full path to it), with built-in png, zlib and libdecor the include path should be:

libs/FLTK libs/FLTK/deps/png libs/FLTK/deps/zlib libs/FLTK/deps/libdecor ${CMAKE_PREFIX_PATH}

and lets say CMAKE_PREFIX_PATH contains the system dependency for libjpeg, which is not listed as one of the deps/, but would get picked last.

The dependencies would be moved to two additional directories (so they can be chosen individually based on system/built-in setting), like:
deps/png/png/png.h
deps/zlib/zlib/zconf.h
deps/libdecor/libdecor/libdecor.h

With that approach, you would not have any issues with header files getting out of sync. It would be, indeed, a major restructuring of the current distribution of directories in FLTK, so it would need some heavy testing to make sure nothing breaks.

@Albrecht-S
Copy link
Member

This comment is off-topic, please read but don't reply to these statements.

@ggarra13 Gonzalo, I agree that there are issues with the bundled libraries and you made some good points in your latest comment. It is however even more complicated than you described, and I won't change this directory structure in FLTK 1.4.x, i.e. as long as we have to maintain both configure/Makefiles and CMake, together with fltk-config which makes handling of these libs even more problematic. I can't go into details now, and this would really be off-topic here.

Please understand that I can't spend time on this "bundled library problem" now and risk the long overdue release of FLTK 1.4.0.

I'm really aware of this "bundled library problem" but if this concerns you in any way, please start a discussion thread in fltk.coredev where I may spend some time to explain the issue in more detail. But don't expect big changes regarding this in FLTK 1.4.0.

Please read further in my follow-up comment...

@Albrecht-S
Copy link
Member

Back on-topic. Gonzalo, please let's focus on the topic of your report:

The first compilation will work, the second run (where there was an API changing) without cleaning the directory will fail. For more info, please refer to the included README.txt file.

This happens in your build with or w/o bundled libraries, I believe you could even remove the external ZLIB project and it would still happen. The FLTK build in the second run - after changing the Git commit hash of the FLTK library - fails because the compiler "sees" the old FLTK header files and misses the new method added in the updated Git commit hash. I hope we can both agree with this analysis so far.

Where we don't agree yet is the reason why this happens. You wrote, among others:

... The CMAKE_PREFIX_PATH should be added later or the FLTK's FL includes first. There should not be any need to clean the directory. ...
...
Or move FLTK's dependencies to a deps/ directory, so that includes are:
-I FLTK_DIR/FL -I CMAKE_PREFIX_PATH/include -I FLTK_DIR/deps

At this point I don't know exactly what happens in your build process, but what I definitely know is that our FLTK (CMake) build files don't use CMAKE_PREFIX_PATH explicitly to build FLTK with one exception where we add /mingw to CMAKE_PREFIX_PATH, obviously only on MinGW builds. Thus neither my Linux test build nor a macOS build should be affected by any manipulations or the usage of CMAKE_PREFIX_PATH inside FLTK's CMake files. See git grep CMAKE_PREFIX_PATH.

OTOH your build process sets CMAKE_PREFIX_PATH to the local install directory of your build where all the build artifacts of all builds are stored (particularly FLTK). After the first build this install directory and subdirs are populated with FLTK files of that previous build. I could not find out yet how exactly this affects the second build, but I proposed you a way how you can set up your build so this does not happen - w/o changing anything in the FLTK build procedure.

Let's take a look at the CMake docs on CMAKE_PREFIX_PATH:

... specifying installation prefixes to be searched by the find_package(), find_program(), find_library(), find_file(), and find_path() commands.
By default this is empty. It is intended to be set by the project.
...

"It is to be set by the project": what does this mean? Basically I interpret this that "the project" (i.e. in this case the FLTK project) is intended to set this path that affects searches. I do also believe that "the user building the project" can legitimately set it to affect the build of "the project (FLTK)" like you do, for instance to find a particular ZLIB installation or anything else. However, I really don't know what happens if you set CMAKE_PREFIX_PATH to point to an install directory of the same project - as you do in your build process. Whatever happens is done by CMake and its generated command lines for the compiler and linker...

At this point I'm stuck. I can't test and investigate further because I need more time for other FLTK issues to be able to release 1.4.0. I'll come back to this later - unless you decide to close this issue yourself.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
CMake CMake bug or needs CMake support
Projects
None yet
Development

No branches or pull requests

2 participants