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.

Sign up for GitHub

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

Jump to bottom

Enable googletest for test utils #1738

Closed
wants to merge 1 commit into from
Closed

Enable googletest for test utils #1738

wants to merge 1 commit into from

Conversation

majetideepak
Copy link
Collaborator

velox/dwio/dwrf/test/utils/E2EWriterTestUtil.cpp depends on googletest. Enabling VELOX_BUILD_TEST_UTILS now enables googletest.

@majetideepak majetideepak requested a review from mbasmanova May 31, 2022 13:44
@facebook-github-bot facebook-github-bot added the CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. label May 31, 2022
@majetideepak
Copy link
Collaborator Author

This should fix presto_cpp build failure

@majetideepak majetideepak mentioned this pull request Jun 1, 2022
Closed
@majetideepak majetideepak requested a review from kgpai June 1, 2022 02:41
kgpai
kgpai reviewed Jun 1, 2022
View reviewed changes
CMakeLists.txt
@@ -312,7 +312,9 @@ endif()
# Benchmarks and tests at some places are coupled which is not great. See
# velox/vector/CMakeLists.txt. TODO: Decouple.
set(VELOX_DISABLE_GOOGLETEST OFF)
if(NOT VELOX_BUILD_TESTING AND NOT VELOX_ENABLE_BENCHMARKS_BASIC)
if(NOT VELOX_BUILD_TEST_UTILS
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There might be other projects of which Velox is a submodule and it seems unnecessary to build test utils etc when you just need gtest as a dependency. How about we have another flag say VELOX_LINK_GTEST which if set wont trigger the section below. It also makes it evident why its set.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Each project should ideally have its own GTest dependency and should not depend on Velox for GTest. But unfortunately, GTest is coupled with Velox in strange ways, which is causing all these issues.
This PR is a temporary fix for presto_cpp. We should fix Velox and have presto_cpp vendor its own GTest.
If there are other projects in a similar situation, we should do as you suggested.
Add a flag VELOX_VENDOR_GTEST and enable GTest. What do you think?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok , sounds good.

@facebook-github-bot
Copy link
Contributor

@kgpai has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator.

@majetideepak
Copy link
Collaborator Author

I filed #1739 to free Velox test utils from gtest.

@majetideepak majetideepak deleted the fix-gtest branch June 16, 2022 18:37
zhejiangxiaomai pushed a commit to zhejiangxiaomai/velox that referenced this pull request Jun 21, 2022
@majetideepak
Enable googletest for test utils (facebookincubator#1738)
37dc478 
Summary:
`velox/dwio/dwrf/test/utils/E2EWriterTestUtil.cpp` depends on googletest. Enabling `VELOX_BUILD_TEST_UTILS` now enables googletest.

Pull Request resolved: facebookincubator#1738

Reviewed By: amitkdutta

Differential Revision: D36819015

Pulled By: kgpai

fbshipit-source-id: fbe45a9c893ad78871055de68c8a0a515e3020a4
shiyu-bytedance pushed a commit to shiyu-bytedance/velox-1 that referenced this pull request Aug 18, 2022
@majetideepak @liushengxuan
Enable googletest for test utils (facebookincubator#1738)
9663ead 
Summary:
`velox/dwio/dwrf/test/utils/E2EWriterTestUtil.cpp` depends on googletest. Enabling `VELOX_BUILD_TEST_UTILS` now enables googletest.

Pull Request resolved: facebookincubator#1738

Reviewed By: amitkdutta

Differential Revision: D36819015

Pulled By: kgpai

fbshipit-source-id: fbe45a9c893ad78871055de68c8a0a515e3020a4
marin-ma pushed a commit to marin-ma/velox-oap that referenced this pull request Dec 15, 2023
@lwz9103
[GLUTEN-1738][CH]Update libchbuilder dockerfile to make build latest …
a6adf7b 
…Clickhouse successfully (facebookincubator#1740)

What changes were proposed in this pull request?
(Fixes: facebookincubator#1738)

How was this patch tested?
manual tests, run ep/build-clickhouse/src/build_clickhouse.sh
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@kgpai kgpai kgpai approved these changes

@mbasmanova mbasmanova Awaiting requested review from mbasmanova

Assignees
No one assigned
Labels
CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@majetideepak @facebook-github-bot @kgpai