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

[C++] Bundled GTest CI jobs seem to #includeing GTest headers from somewhere else #36379

Closed
bkietz opened this issue Jun 29, 2023 · 5 comments · Fixed by #37612
Closed

[C++] Bundled GTest CI jobs seem to #includeing GTest headers from somewhere else #36379

bkietz opened this issue Jun 29, 2023 · 5 comments · Fixed by #37612
Assignees
Milestone

Comments

@bkietz
Copy link
Member

bkietz commented Jun 29, 2023

Describe the bug, including details regarding any error messages, version, and platform.

In #36334 I saw link errors in some CI jobs which build bundled GTest:

The symbol which is undefined is the internal FormatMatcherDescription function. That function is present in the bundled version of GTest (v1.11), but its signature is different in that version.

This suggests that somehow although we're building GTest v1.11 the tests are #includeing headers from a more recent version of GTest and we just haven't noticed before now.

Component(s)

C++

@lidavidm
Copy link
Member

FWIW, that pipeline also installs GTest via Homebrew. We've seen similar issues before when we had both Conda-installed and bundled GTest in the same build (because we stuck the Conda include directory early on in the search path).

@bkietz
Copy link
Member Author

bkietz commented Jun 29, 2023

The same link error occurs on the manylinux JNI job. I'll update the description with a more informative listing of the link errors

@bkietz
Copy link
Member Author

bkietz commented Sep 7, 2023

The problem is actually more subtle: when the dependency's source is AUTO, ThirdpartyToolchain.cmake checks for a compatible system package. If the system package is not compatible, it will build a bundled version.

if(COMPATIBLE)
set(${DEPENDENCY_NAME}_SOURCE "SYSTEM")
else()
build_dependency(${DEPENDENCY_NAME})
set(${DEPENDENCY_NAME}_SOURCE "BUNDLED")
endif()

Building the bundled dependency adds its include directories... but it appends them so include directories which are already added have precedence. In this case the incompatible gtest headers from the conda environment are not being overridden by the bundled headers. It should be sufficient to use target_include_directories() with the BEFORE keyword to ensure that bundled projects' headers can override system headers

@bkietz
Copy link
Member Author

bkietz commented Sep 7, 2023

I'm not sure how this interacts with #37483 yet. Among other things bundled gtest is no longer built as an ExternalProject. I'll open a PR using target_include_directories+BEFORE for the dependencies which still are ExternalProjects

@bkietz
Copy link
Member Author

bkietz commented Sep 7, 2023

Nevermind, FetchContent_MakeAvailable does produce an ExternalProject.

@kou kou closed this as completed in #37612 Sep 8, 2023
kou pushed a commit that referenced this issue Sep 8, 2023
…em include dirs (#37612)

### Rationale for this change

Bundled dependencies' include directories should override system include dirs. Otherwise an incompatible header in the system might be included when we wanted a header from the bundled dependency.

### What changes are included in this PR?

bundled dependencies explicitly insert their own include dirs ahead of others
* Closes: #36379

Authored-by: Benjamin Kietzman <bengilgit@gmail.com>
Signed-off-by: Sutou Kouhei <kou@clear-code.com>
@kou kou added this to the 14.0.0 milestone Sep 8, 2023
loicalleyne pushed a commit to loicalleyne/arrow that referenced this issue Nov 13, 2023
…e system include dirs (apache#37612)

### Rationale for this change

Bundled dependencies' include directories should override system include dirs. Otherwise an incompatible header in the system might be included when we wanted a header from the bundled dependency.

### What changes are included in this PR?

bundled dependencies explicitly insert their own include dirs ahead of others
* Closes: apache#36379

Authored-by: Benjamin Kietzman <bengilgit@gmail.com>
Signed-off-by: Sutou Kouhei <kou@clear-code.com>
dgreiss pushed a commit to dgreiss/arrow that referenced this issue Feb 19, 2024
…e system include dirs (apache#37612)

### Rationale for this change

Bundled dependencies' include directories should override system include dirs. Otherwise an incompatible header in the system might be included when we wanted a header from the bundled dependency.

### What changes are included in this PR?

bundled dependencies explicitly insert their own include dirs ahead of others
* Closes: apache#36379

Authored-by: Benjamin Kietzman <bengilgit@gmail.com>
Signed-off-by: Sutou Kouhei <kou@clear-code.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants