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++] arrow-testing.pc.in's Cflags are incorrectly set if gtest isn't built as part of the arrow build #33796

Closed
pleicht opened this issue Jan 19, 2023 · 1 comment · Fixed by #33812
Assignees
Milestone

Comments

@pleicht
Copy link

pleicht commented Jan 19, 2023

If we don't build gtest as part of the arrow build process (we could get it pre-built somewhere else), then the following variable is unset:

gtest_includedir=@GTEST_INCLUDE_DIR@

Which is currently only set in the macro(build_gtest) cmake function found here:

set(GTEST_INCLUDE_DIR "${GTEST_PREFIX}/include")

As a result the Cflags generated in:

Cflags: -I${gtest_includedir}

End up being just -I, which then causes an -I to appear in the compile command for users building against the arrow project, which in our case (and I assume all cases?) is invalid. Specifically, the behavior we saw was nothing inside of the std:: namespace could be referenced anymore
As an example, taking out a sub portion of our compile command which was generated with this issue:
-pthread -I -std=gnu++17

A solution here would be to not generate any Cflags in the case that GTEST_INCLUDE_DIR isn't set. The -I in Cflags: -I${gtest_includedir} needs to be created conditionally. I'll try to add a PR in the next few days to address the issue.

Component(s)

C++

@kou kou changed the title arrow-testing.pc.in's Cflags are incorrectly set if gtest isn't built as part of the arrow build [C++] arrow-testing.pc.in's Cflags are incorrectly set if gtest isn't built as part of the arrow build Jan 20, 2023
@kou
Copy link
Member

kou commented Jan 20, 2023

Ah, we should add gtest to Requires when we use system GTest.

kou added a commit to kou/arrow that referenced this issue Jan 21, 2023
…oogleTest

Bundled GoogleTest isn't supported yet.
kou added a commit that referenced this issue Jan 25, 2023
…est (#33812)

### Rationale for this change

Empty `-I` in `Cflags:` generates an invalid build command line.

### What changes are included in this PR?

Add `Requires: gtest` if `gtest.pc` exists.

Add `Cflags: -I...` and `Libs: /.../libgtest.a` if `gtest.pc` doesn't exist.

Bundled GoogleTest isn't supported yet.

### Are these changes tested?

Yes.

### Are there any user-facing changes?

Yes.
* Closes: #33796

Authored-by: Sutou Kouhei <kou@clear-code.com>
Signed-off-by: Sutou Kouhei <kou@clear-code.com>
@kou kou added this to the 12.0.0 milestone Jan 25, 2023
sjperkins pushed a commit to sjperkins/arrow that referenced this issue Feb 10, 2023
…oogleTest (apache#33812)

### Rationale for this change

Empty `-I` in `Cflags:` generates an invalid build command line.

### What changes are included in this PR?

Add `Requires: gtest` if `gtest.pc` exists.

Add `Cflags: -I...` and `Libs: /.../libgtest.a` if `gtest.pc` doesn't exist.

Bundled GoogleTest isn't supported yet.

### Are these changes tested?

Yes.

### Are there any user-facing changes?

Yes.
* Closes: apache#33796

Authored-by: Sutou Kouhei <kou@clear-code.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.

2 participants