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

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

Closed
wants to merge 7 commits into from
Closed

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

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
d586cf6
Build googletest only if testing is enabled
majetideepak May 20, 2022
faa06ba
add comment for FRIEND_TEST macro
majetideepak May 25, 2022
a395fc0
Guard FRIEND_TEST
majetideepak May 25, 2022
230b9b0
rename FRIEND_TEST to VELOX_FRIEND_TEST
majetideepak May 26, 2022
ab0100f
fix comment
majetideepak May 26, 2022
c3c078d
Enable googletest by default
majetideepak May 26, 2022
895d074
switch option to enable other build systems
majetideepak May 26, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
8 changes: 8 additions & 0 deletions CMakeLists.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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.
Copy link
Contributor

Choose a reason for hiding this comment

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

Perhaps, file an issue?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

#1704

set(VELOX_ENABLE_GOOGLETEST OFF)
if(VELOX_BUILD_TESTING OR VELOX_ENABLE_BENCHMARKS_BASIC)
set(VELOX_ENABLE_GOOGLETEST ON)
add_definitions(-DVELOX_ENABLE_GOOGLETEST)
endif()

include_directories(SYSTEM velox)
include_directories(SYSTEM velox/external)
include_directories(SYSTEM velox/external/duckdb)
Expand Down
6 changes: 5 additions & 1 deletion third_party/CMakeLists.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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)
Copy link
Contributor

Choose a reason for hiding this comment

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

Why not use VELOX_BUILD_TESTING here?

Copy link
Collaborator Author

@majetideepak majetideepak May 25, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

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.

add_subdirectory(googletest)
endif()

add_subdirectory(xsimd)
2 changes: 0 additions & 2 deletions velox/benchmarks/basic/CMakeLists.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -23,8 +23,6 @@ set(velox_benchmark_deps
${FOLLY}
${FOLLY_BENCHMARK}
${DOUBLE_CONVERSION}
gtest
gtest_main
${gflags_LIBRARIES}
glog::glog)

Expand Down
3 changes: 0 additions & 3 deletions velox/benchmarks/tpch/CMakeLists.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -33,9 +33,6 @@ target_link_libraries(
velox_type
velox_caching
velox_vector
velox_vector_test_lib
gtest
gtest_main
${FOLLY_WITH_DEPENDENCIES}
${FOLLY_BENCHMARK}
${FMT})
25 changes: 25 additions & 0 deletions velox/common/base/GTestMacros.h
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,25 @@
/*
* Copyright (c) Facebook, Inc. and its affiliates.
*
* Licensed under the Apache License, Version 2.0 (the "License");
* you may not use this file except in compliance with the License.
* You may obtain a copy of the License at
*
* http://www.apache.org/licenses/LICENSE-2.0
*
* Unless required by applicable law or agreed to in writing, software
* distributed under the License is distributed on an "AS IS" BASIS,
* 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.
*/

#pragma once

#ifdef VELOX_ENABLE_GOOGLETEST
Copy link
Contributor

Choose a reason for hiding this comment

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

Why do we need this? Shouldn't we just not compile the code that includes gtest unless a specific variable is 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.

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

Copy link
Contributor

Choose a reason for hiding this comment

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

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.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

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

Copy link
Contributor

Choose a reason for hiding this comment

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

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?

Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

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

#include <gtest/gtest_prod.h>
#else
// FRIEND_TEST macro is only used when testing is enabled.
// Replacing it with "nothing" is okay when testing is disabled.
#define FRIEND_TEST(X, Y)
Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

done. Thanks.

#endif
4 changes: 4 additions & 0 deletions velox/common/memory/CMakeLists.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -29,3 +29,7 @@ add_library(

target_link_libraries(velox_memory velox_flag_definitions velox_exception gtest
${FOLLY_WITH_DEPENDENCIES})

if(VELOX_ENABLE_GOOGLETEST)
target_link_libraries(velox_memory gtest)
endif()
3 changes: 1 addition & 2 deletions velox/common/memory/Memory.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -24,15 +24,14 @@
#include <string>

#include <fmt/format.h>
#include <gflags/gflags.h>
#include <glog/logging.h>
#include <gtest/gtest_prod.h>

#include "folly/CPortability.h"
#include "folly/Likely.h"
#include "folly/Random.h"
#include "folly/SharedMutex.h"
#include "folly/experimental/FunctionScheduler.h"
#include "velox/common/base/GTestMacros.h"
#include "velox/common/memory/MemoryUsage.h"
#include "velox/common/memory/MemoryUsageTracker.h"

Expand Down
2 changes: 1 addition & 1 deletion velox/dwio/common/ChainedBuffer.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,8 +16,8 @@

#pragma once

#include <gtest/gtest_prod.h>
#include <bitset>
#include "velox/common/base/GTestMacros.h"
#include "velox/dwio/common/DataBuffer.h"

namespace facebook {
Expand Down
2 changes: 1 addition & 1 deletion velox/dwio/dwrf/common/IntEncoder.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,8 +17,8 @@
#pragma once

#include <folly/Varint.h>
#include <gtest/gtest_prod.h>
#include "velox/common/base/BitUtil.h"
#include "velox/common/base/GTestMacros.h"
#include "velox/common/base/Nulls.h"
#include "velox/common/encode/Coding.h"
#include "velox/dwio/dwrf/common/IntCodecCommon.h"
Expand Down
2 changes: 1 addition & 1 deletion velox/dwio/dwrf/common/RLEv1.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,7 +16,7 @@

#pragma once

#include <gtest/gtest_prod.h>
#include "velox/common/base/GTestMacros.h"
#include "velox/common/base/Nulls.h"
#include "velox/dwio/dwrf/common/Adaptor.h"
#include "velox/dwio/dwrf/common/DecoderUtil.h"
Expand Down
2 changes: 1 addition & 1 deletion velox/dwio/dwrf/common/Range.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,7 +16,7 @@

#pragma once

#include <gtest/gtest_prod.h>
#include "velox/common/base/GTestMacros.h"
#include "velox/dwio/common/exception/Exception.h"

namespace facebook::velox::dwrf {
Expand Down
3 changes: 1 addition & 2 deletions velox/dwio/dwrf/reader/StripeDictionaryCache.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,10 +16,9 @@

#pragma once

#include <gtest/gtest_prod.h>

#include <folly/Function.h>

#include "velox/common/base/GTestMacros.h"
#include "velox/dwio/dwrf/common/Common.h"
#include "velox/dwio/dwrf/common/IntDecoder.h"
#include "velox/vector/BaseVector.h"
Expand Down
3 changes: 1 addition & 2 deletions velox/dwio/dwrf/writer/ColumnWriter.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,8 +16,7 @@

#pragma once

#include "gtest/gtest_prod.h"

#include "velox/common/base/GTestMacros.h"
#include "velox/dwio/dwrf/common/ByteRLE.h"
#include "velox/dwio/dwrf/common/Common.h"
#include "velox/dwio/dwrf/common/IntEncoder.h"
Expand Down
2 changes: 1 addition & 1 deletion velox/dwio/dwrf/writer/IndexBuilder.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,7 +16,7 @@

#pragma once

#include <gtest/gtest_prod.h>
#include "velox/common/base/GTestMacros.h"
#include "velox/dwio/dwrf/common/OutputStream.h"
#include "velox/dwio/dwrf/common/wrap/dwrf-proto-wrapper.h"
#include "velox/dwio/dwrf/writer/StatisticsBuilder.h"
Expand Down
2 changes: 1 addition & 1 deletion velox/dwio/dwrf/writer/IntegerDictionaryEncoder.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,7 +17,7 @@
#pragma once

#include <folly/container/F14Set.h>
#include <gtest/gtest_prod.h>
#include "velox/common/base/GTestMacros.h"
#include "velox/dwio/common/DataBuffer.h"
#include "velox/dwio/dwrf/common/IntEncoder.h"
#include "velox/dwio/dwrf/writer/DictionaryEncodingUtils.h"
Expand Down
3 changes: 2 additions & 1 deletion velox/dwio/dwrf/writer/StringDictionaryEncoder.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,7 +18,8 @@

#include <folly/container/F14Set.h>
#include <folly/hash/Checksum.h>
#include <gtest/gtest_prod.h>

#include "velox/common/base/GTestMacros.h"
#include "velox/dwio/common/DataBuffer.h"

namespace facebook::velox::dwrf {
Expand Down
2 changes: 1 addition & 1 deletion velox/dwio/dwrf/writer/WriterBase.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,7 +16,7 @@

#pragma once

#include "gtest/gtest_prod.h"
#include "velox/common/base/GTestMacros.h"
#include "velox/dwio/dwrf/writer/WriterContext.h"
#include "velox/dwio/dwrf/writer/WriterSink.h"

Expand Down
3 changes: 1 addition & 2 deletions velox/dwio/dwrf/writer/WriterContext.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,8 +16,7 @@

#pragma once

#include <gtest/gtest_prod.h>

#include "velox/common/base/GTestMacros.h"
#include "velox/common/time/CpuWallTimer.h"
#include "velox/dwio/dwrf/common/Compression.h"
#include "velox/dwio/dwrf/writer/IndexBuilder.h"
Expand Down
3 changes: 1 addition & 2 deletions velox/dwio/dwrf/writer/WriterShared.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,8 +19,7 @@
#include <iterator>
#include <limits>

#include <gtest/gtest_prod.h>

#include "velox/common/base/GTestMacros.h"
#include "velox/dwio/dwrf/common/Encryption.h"
#include "velox/dwio/dwrf/common/wrap/dwrf-proto-wrapper.h"
#include "velox/dwio/dwrf/proto/dwrf_proto.pb.h"
Expand Down
10 changes: 2 additions & 8 deletions velox/expression/benchmarks/CMakeLists.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,14 +13,8 @@
# limitations under the License.

set(BENCHMARK_DEPENDENCIES
velox_functions_test_lib
velox_exec
velox_exec_test_util
gtest
gtest_main
${gflags_LIBRARIES}
${FOLLY_WITH_DEPENDENCIES}
${FOLLY_BENCHMARK})
velox_functions_test_lib velox_exec velox_exec_test_util
${gflags_LIBRARIES} ${FOLLY_WITH_DEPENDENCIES} ${FOLLY_BENCHMARK})

add_executable(velox_benchmark_call_null_free_no_nulls
CallNullFreeBenchmark.cpp)
Expand Down
2 changes: 0 additions & 2 deletions velox/functions/prestosql/aggregates/benchmarks/CMakeLists.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -23,6 +23,4 @@ target_link_libraries(
velox_dwio_common_exception
${FOLLY_WITH_DEPENDENCIES}
${FOLLY_BENCHMARK}
gtest
gtest_main
${GFLAGS_LIBRARIES})
10 changes: 2 additions & 8 deletions velox/functions/prestosql/benchmarks/CMakeLists.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,14 +13,8 @@
# limitations under the License.

set(BENCHMARK_DEPENDENCIES_NO_FUNC
velox_functions_test_lib
velox_exec
velox_exec_test_util
gtest
gtest_main
${gflags_LIBRARIES}
${FOLLY_WITH_DEPENDENCIES}
${FOLLY_BENCHMARK})
velox_functions_test_lib velox_exec velox_exec_test_util
${gflags_LIBRARIES} ${FOLLY_WITH_DEPENDENCIES} ${FOLLY_BENCHMARK})

set(BENCHMARK_DEPENDENCIES
velox_functions_prestosql velox_functions_lib velox_vector_fuzzer
Expand Down