Build googletest only if testing or basic benchmarking are enabled #1666

majetideepak · 2022-05-20T17:07:45Z

Also, adds a dummy definition forFRIEND_TEST when googletest is disabled.
Resolves #1654

amitkdutta

Loos good to me.

majetideepak · 2022-05-20T17:10:26Z

@amitkdutta thanks for the review. Looks like there are a couple of more FRIEND_TEST uses. I will fix them shortly.

majetideepak · 2022-05-21T02:02:48Z

@amitkdutta can you take another look? The S3 test failure is unrelated.

majetideepak · 2022-05-24T12:22:59Z

@kgpai @amitkdutta can you take another look at this?

mbasmanova

@majetideepak Nice.

mbasmanova · 2022-05-25T13:40:11Z

CMakeLists.txt

@@ -292,6 +292,14 @@ if(VELOX_BUILD_TESTING AND NOT VELOX_ENABLE_DUCKDB)
  )
 endif()

+# Benchmarks and tests at some places are coupled which is not great. See
+# velox/vector/CMakeLists.txt. TODO: Decouple.


Perhaps, file an issue?

mbasmanova · 2022-05-25T13:41:10Z

velox/common/base/GTestMacros.h

+#ifdef VELOX_ENABLE_GOOGLETEST
+#include <gtest/gtest_prod.h>
+#else
+#define FRIEND_TEST(X, Y)


nit: Perhaps, add a comment to explain why is it Ok to replace this macro with "nothing".

done. Thanks.

facebook-github-bot · 2022-05-25T14:13:54Z

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

kgpai

LGTM. Thanks Deepak.

mbasmanova · 2022-05-25T18:14:38Z

I'm getting errors:

buck-out/v2/gen/fbcode/c6b399001575978c/velox/common/base/__velox_common_base__/headers/velox/common/base/GTestMacros.h:24:9: error: 'FRIEND_TEST' macro redefined [-Werror,-Wmacro-redefined]
#define FRIEND_TEST(X, Y)

majetideepak · 2022-05-25T21:02:31Z

@mbasmanova Can you share some more context from the error log. Does it see the previous definition from the gtest header? Thanks.

mbasmanova · 2022-05-25T21:09:20Z

@majetideepak Here is what I get:

In file included from velox/common/memory/Memory.h:34:
velox/common/base/GTestMacros.h:24:9: error: 'FRIEND_TEST' macro redefined [-Werror,-Wmacro-redefined]
#define FRIEND_TEST(X, Y)
        ^
third-party-buck/platform009/build/googletest/include/gtest/gtest_prod.h:58:9: note: previous definition is here
#define FRIEND_TEST(test_case_name, test_name)\
        ^
1 error generated.

mbasmanova · 2022-05-25T21:10:05Z

I assume something else is including gtest_prod.h

mbasmanova · 2022-05-25T21:10:46Z

Maybe add an #ifndef guard?

mbasmanova · 2022-05-25T21:56:48Z

Getting more errors

Xxx.h:536:3: error: ambiguous expansion of macro 'FRIEND_TEST' [-Werror,-Wambiguous-macro]
  FRIEND_TEST(ConfigTest, ParseScopeResolutionCondition);
  ^

mbasmanova · 2022-05-25T21:59:09Z

I wonder if we end up having one definition is some cases and another in other cases.

mbasmanova · 2022-05-25T22:02:22Z

I'm not sure how to make this work given that we cannot change all the code to replace gtest.h with GTestMacros.h

CC: @kgpai @Yuhta

mbasmanova · 2022-05-25T22:03:40Z

velox/common/base/GTestMacros.h

+
+#pragma once
+
+#ifdef VELOX_ENABLE_GOOGLETEST


Why do we need this? Shouldn't we just not compile the code that includes gtest unless a specific variable is set?

As you observed, FRIEND_TEST is coupled with non-test code.

mbasmanova · 2022-05-25T22:04:13Z

third_party/CMakeLists.txt

@@ -11,5 +11,9 @@
 # WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
 # See the License for the specific language governing permissions and
 # limitations under the License.
-add_subdirectory(googletest)
+
+if(VELOX_ENABLE_GOOGLETEST)


Why not use VELOX_BUILD_TESTING here?

Since googletest is needed by both VELOX_BUILD_TESTING and VELOX_ENABLE_BENCHMARKS_BASIC as some of the benchmarks are coupled with the test classes.
So VELOX_ENABLE_GOOGLETEST is a proxy for either.

mbasmanova · 2022-05-25T22:06:00Z

I wonder if we could get rid of usages of FRIEND_TEST macro altogether.

mbasmanova · 2022-05-25T22:07:38Z

Tight coupling that is created by using FRIEND_TEST macro doesn't feel quite right.

mbasmanova · 2022-05-25T22:22:55Z

One option might be to introduce a new macro VELOX_FRIEND_TEST and define it as nothing if VELOX_BUILD_TESTING is off and FRIEND_TEST if on.

majetideepak · 2022-05-25T23:01:13Z

One option might be to introduce a new macro VELOX_FRIEND_TEST and define it as nothing if VELOX_BUILD_TESTING is off and FRIEND_TEST if on.

FRIEND_TEST macro is really simple (see below) and I don't think VELOX_FRIEND_TEST is going to help much.
The real problem is the gtest_prod.h header included directly in the source code without a guard. If we can replace them with GTestMacros.h, it should resolve these issues.

https://github.com/google/googletest/blob/main/googletest/include/gtest/gtest_prod.h#L57

#define FRIEND_TEST(test_case_name, test_name)\
friend class test_case_name##_##test_name##_Test

mbasmanova · 2022-05-25T23:05:02Z

If we can replace them with GTestMacros.h, it should resolve these issues.

Unfortunately, there are way too many places to update for this to be practical. @kgpai @Yuhta Krishna, Jimmy, what do you think?

majetideepak · 2022-05-25T23:18:03Z

Unfortunately, there are way too many places to update for this to be practical.

I see your point. Then changing FRIEND_TEST to VELOX_FRIEND_TEST should help. I will make this change.

facebook-github-bot · 2022-05-26T01:24:56Z

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

mbasmanova · 2022-05-26T01:28:27Z

velox/common/base/GTestMacros.h

+
+#pragma once
+
+#ifdef VELOX_ENABLE_GOOGLETEST


I wonder if it is possible to have this ON by default. Otherwise, I'll need to figure out how too enable this for all the libraries that use it internally.

I did this. Let me know if it does not work.

mbasmanova · 2022-05-26T02:19:34Z

velox/common/base/GTestMacros.h

+
+#pragma once
+
+#ifdef VELOX_ENABLE_GOOGLETEST


Sorry, I should have been clearer. I think I need #include <gtest/gtest_prod.h> to happen by default, e.g. without setting any variable as I don't have these variables in the internal build files. Is this possible?

e.g. maybe we could have VELOX_DISABLE_GOOGLETEST variable instead of VELOX_ENABLE_GOOGLETEST

I made this change. I realized it should work for non-CMake systems as well by default.

facebook-github-bot · 2022-05-26T03:18:20Z

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

wenleix · 2022-05-31T23:55:01Z

TorchArrow build starts to fail after submodule update to this commit: https://github.com/facebookresearch/torcharrow/runs/6605394261?check_suite_focus=true (Velox submodule commit: 118feb5), since velox/common/memory/Memory.h includes gtest/gtest_prod.h:

In file included from /home/runner/work/torcharrow/torcharrow/csrc/velox/velox/velox/common/memory/Memory.h:34,
                 from /home/runner/work/torcharrow/torcharrow/csrc/velox/lib.cpp:12:
/home/runner/work/torcharrow/torcharrow/csrc/velox/velox/velox/common/base/GTestMacros.h:24:10: fatal error: gtest/gtest_prod.h: No such file or directory

Build succeed with the previous commit (3591bbe) : https://github.com/facebookresearch/torcharrow/runs/6604765945?check_suite_focus=true

Looks like VELOX_MINIMAL should automatically enable VELOX_DISABLE_GOOGLETEST . Looking into.

cc @kgpai

amitkdutta · 2022-06-01T00:15:27Z

@majetideepak @kgpai Presto CPP builds are failing as well.

velox/velox/dwio/dwrf/test/utils/E2EWriterTestUtil.cpp:19:10: fatal error: gtest/gtest.h: No such file or directory

https://app.circleci.com/pipelines/github/facebookexternal/presto_cpp/5216/workflows/0eddd571-7781-43f5-b5eb-392d97197a3e/jobs/19922

wenleix · 2022-06-01T02:15:41Z

@amitkdutta I found manually set compiler flag VELOX_DISABLE_GOOGLETEST at TorchArrow CMakeFile level would make it build: pytorch/torcharrow#352 .

Basically there is

target_compile_definitions(
  _torcharrow
  PRIVATE
  VELOX_DISABLE_GOOGLETEST
)

(it needs #1745 to avoid linker error)

My guess (I am a CMake noob so I can be totally wrong) the compiler flag set at

velox/CMakeLists.txt

Line 317 in 118feb5

add_definitions(-DVELOX_DISABLE_GOOGLETEST)

doesn't affects the outer-level project compiler flag (e.g. presto-cpp and torcharrow use Velox as submodule, thus their compiler flag doesn't get VELOX_DISABLE_GOOGLETEST set on )

Update: never mind, presto-cpp indeed needs GTest; this is different from TorchArrow

majetideepak · 2022-06-01T02:16:03Z

@amitkdutta #1738 should fix the presto_cpp failure

…acebookincubator#1666) Summary: Also, adds a dummy definition for`FRIEND_TEST` when googletest is disabled. Resolves facebookincubator#1654 Pull Request resolved: facebookincubator#1666 Reviewed By: kgpai Differential Revision: D36664732 Pulled By: mbasmanova fbshipit-source-id: a08859f96f68d5bc9d2c5092be6f7f397508706f

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 20, 2022

amitkdutta approved these changes May 20, 2022

View reviewed changes

majetideepak force-pushed the fix-googletest branch 2 times, most recently from cb8f53d to 6d6beb9 Compare May 20, 2022 23:30

Build googletest only if testing is enabled

d586cf6

majetideepak force-pushed the fix-googletest branch from 6d6beb9 to d586cf6 Compare May 21, 2022 01:02

majetideepak changed the title ~~Build googletest only if testing is enabled~~ Build googletest only if testing or basic benchmarking are enabled May 21, 2022

majetideepak requested review from amitkdutta and kgpai May 23, 2022 17:26

majetideepak requested a review from mbasmanova May 25, 2022 13:09

mbasmanova approved these changes May 25, 2022

View reviewed changes

add comment for FRIEND_TEST macro

faa06ba

kgpai approved these changes May 25, 2022

View reviewed changes

Guard FRIEND_TEST

a395fc0

mbasmanova reviewed May 25, 2022

View reviewed changes

majetideepak added 2 commits May 25, 2022 20:45

rename FRIEND_TEST to VELOX_FRIEND_TEST

230b9b0

fix comment

ab0100f

mbasmanova reviewed May 26, 2022

View reviewed changes

Enable googletest by default

c3c078d

mbasmanova reviewed May 26, 2022

View reviewed changes

switch option to enable other build systems

895d074

facebook-github-bot closed this in 118feb5 May 26, 2022

wenleix mentioned this pull request Jun 1, 2022

Set VELOX_DISABLE_GOOGLETEST ON for TorchArrow pytorch/torcharrow#352

Closed

majetideepak deleted the fix-googletest branch June 16, 2022 18:37


		#pragma once

		#ifdef VELOX_ENABLE_GOOGLETEST


		#pragma once

		#ifdef VELOX_ENABLE_GOOGLETEST


		#pragma once

		#ifdef VELOX_ENABLE_GOOGLETEST

Build googletest only if testing or basic benchmarking are enabled #1666

Build googletest only if testing or basic benchmarking are enabled #1666

Conversation

majetideepak commented May 20, 2022 • edited Loading

amitkdutta left a comment

Choose a reason for hiding this comment

majetideepak commented May 20, 2022 • edited Loading

majetideepak commented May 21, 2022

majetideepak commented May 24, 2022

mbasmanova left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

facebook-github-bot commented May 25, 2022

kgpai left a comment

Choose a reason for hiding this comment

mbasmanova commented May 25, 2022

majetideepak commented May 25, 2022

mbasmanova commented May 25, 2022

mbasmanova commented May 25, 2022

mbasmanova commented May 25, 2022

mbasmanova commented May 25, 2022

mbasmanova commented May 25, 2022

mbasmanova commented May 25, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

majetideepak May 25, 2022 • edited Loading

Choose a reason for hiding this comment

mbasmanova commented May 25, 2022

mbasmanova commented May 25, 2022

mbasmanova commented May 25, 2022

majetideepak commented May 25, 2022 • edited Loading

mbasmanova commented May 25, 2022

majetideepak commented May 25, 2022

facebook-github-bot commented May 26, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

facebook-github-bot commented May 26, 2022

wenleix commented May 31, 2022 • edited Loading

amitkdutta commented Jun 1, 2022

wenleix commented Jun 1, 2022 • edited Loading

majetideepak commented Jun 1, 2022

majetideepak commented May 20, 2022 •

edited

Loading

majetideepak commented May 20, 2022 •

edited

Loading

majetideepak May 25, 2022 •

edited

Loading

majetideepak commented May 25, 2022 •

edited

Loading

wenleix commented May 31, 2022 •

edited

Loading

wenleix commented Jun 1, 2022 •

edited

Loading