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

Avoiding wrong includes (only include from FLTK's repository not from CMAKE_PREFIX_PATH) #962

Closed
ggarra13 opened this issue Apr 28, 2024 · 5 comments

Comments

@ggarra13
Copy link
Contributor

ggarra13 commented Apr 28, 2024

Is your feature request related to a problem? Please describe.

This is a feature request albeit I consider it a bug, as FLTK is the only library that exhibits this.

When building FLTK with ExternalProject_Add and setting CMAKE_PREFIX_PATH to the same install location of CMAKE_INSTALL_PREFIX, FLTK will read the .H files from CMAKE_INSTALL_PREFIX first instead of those from its own checkout FL directory, leading often to having the .H and .cxx files become out of sync.

I am using FLTK

  • Version 1.4.0 master

Describe the solution you'd like
FLTK should build with its contents only putting its internal Fl/ direcory first in the list of include directories, so FLTK builds in a sandbox.
I think something is broken in the CMake build files that makes FLTK not do this.

Describe alternatives you've considered
None. Currently I am being forced to remember to do:

rm -rf $CMAKE_INSTALL_PREFIX/include/FL

but that is error prone.

Additional context
Thank you Albrecht and sorry for drawing you insane with CMake requests 👍

@Albrecht-S
Copy link
Member

@ggarra13 Do I understand this correctly? You say that the FLTK build reads FLTK header files "first" from its future install location (CMAKE_INSTALL_PREFIX), i.e. from a previous build (if it exists) and only then (i.e. if not found) from its own source ("checkout") directory, but this is caused by setting CMAKE_PREFIX_PATH and CMAKE_INSTALL_PREFIX to the same directory?

Since you also say that you need to rm -rf $BUILD_DIR/include/FL this implies that the first build succeeds as expected, and a following build (with updated FLTK sources) would potentially fail because the build uses the header files of the previous build (if not deleted). Is this correct?

AFAICT the FLTK build sets the compiler include path(s) such that its own build directory comes first, i.e. before its own source directory. I'm not sure how CMAKE_PREFIX_PATH and CMAKE_INSTALL_PREFIX interact with this while building FLTK. Are you really sure that this is really the case?

Please do the following or something similar to verify this and post the commandline of the compile command requested below together with your full real paths:

  1. Build FLTK as usual (we don't need any output from this successful build)
  2. touch exactly one FLTK source file: touch src/Fl_Tabs.cxx
  3. Build FLTK again, using cmake --build <build_dir> --verbose or something else that outputs the commandline

This should display the compiler commandline of Fl_Tabs.cxx. We want only that one file to be changed to see only one compile command. Please post this command here (skip following commands if any).

Thank you Albrecht and sorry for drawing you insane with CMake requests 👍

Welcome, I always appreciate all input. Users often do things differently than devolopers expect, and it's good to see this before the release is done.

@ggarra13
Copy link
Contributor Author

ggarra13 commented Apr 29, 2024

@ggarra13 Do I understand this correctly? You say that the FLTK build reads FLTK header files "first" from its future install location (CMAKE_INSTALL_PREFIX), i.e. from a previous build (if it exists) and only then (i.e. if not found) from its own source ("checkout") directory, but this is caused by setting CMAKE_PREFIX_PATH and CMAKE_INSTALL_PREFIX to the same directory?

Correct. That seems to be the behavior I am seeing with ExternalProject_Add.

Since you also say that you need to rm -rf $BUILD_DIR/include/FL this implies that the first build succeeds as expected, and a following build (with updated FLTK sources) would potentially fail because the build uses the header files of the previous build (if not deleted). Is this correct?

Correct.

Please do the following or something similar to verify this and post the commandline of the compile command requested below together with your full real paths:

  1. Build FLTK as usual (we don't need any output from this successful build)
  2. touch exactly one FLTK source file: touch src/Fl_Tabs.cxx
  3. Build FLTK again, using cmake --build <build_dir> --verbose or something else that outputs the commandline

That will work as no .H file has changed.

To properly test it is more difficult.

The test has to be done with ExternalProject_Add, a repository you control in a branch you also control for testing, so that you update the .H/.cxx files in an incompatible way, do a commit to the same branch and have cmake refetch the directory, but not update/remove $CMAKE_INSTALL_PREFIX/include/FL.

@Albrecht-S
Copy link
Member

Albrecht-S commented Apr 29, 2024

@ggarra13 You wrote regarding my request to test and to post the commandline:

That will work as no .H file has changed.

Sure, it works, I know that. The only reason I asked you to do that was to see the commandline that is generated by your build system, together with the full path(s) of your build.

To properly test it is more difficult.

That's not what I wanted, as I wrote above. I can imagine what happens if you change headers and build with incompatible headers. Please post the commandline from the test I suggested above - or whatever you may do to get the compile commandline. TIA.

@ggarra13
Copy link
Contributor Author

I deleted the message as it was not showing the right command-line.

@ggarra13
Copy link
Contributor Author

I have two logs from two compiles with differing tags. FLTK built fine in the sandbox and the includes are right. Something else is amiss either in cmake or in a clock skew that triggers the issue.

compile_first_run.log

compile.log

I am closing the issue.

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

No branches or pull requests

2 participants