Decouple the building of benchmarks and tests

[leoluan2009]

Tests and benchmarks targets are now de-coupled.
That means they can be built independently.
Shared functionality is moved to a common utility library.

Resolves #1704

[netlify]

✅ Deploy Preview for meta-velox canceled.

Name	Link
🔨 Latest commit	86783c4
🔍 Latest deploy log	https://app.netlify.com/sites/meta-velox/deploys/63213a3989306b0008bc84e8

[leoluan2009]

cc @mbasmanova @kgpai @majetideepak can you help to review this pr? the build error is due to #2442

[kgpai]

Hi @leoluan2009 , Can you give a brief description of how you have decoupled them ? Thanks !

[leoluan2009]

Hi @leoluan2009 , Can you give a brief description of how you have decoupled them ? Thanks !

use VELOX_ENABLE_BENCHMARKS to control whether to build benchmarks directory and use VELOX_BUILD_TESTING to control whether to build tests directory, if some benchmarks use import the files from tests directory, Add the files to utils subdirectory in tests. When VELOX_BUILD_TESTING is on or VELOX_BUILD_TEST_UTILS is on, will build tests/utils.

[kgpai]

Thanks again @leoluan2009 , Based on my perusal, I see the following changes:

• Move common test utilities to a separate library.
• Create separate flag for Benchmarks and decouple benchmark building from tests.

Some questions:

• Are benchmarks still built along with tests ? If not how should they be invoked ?
• We run benchmark comparisons to catch performance regressions on merge to main ; We need to ensure that these dont break , due to these changes.

[leoluan2009]

Are benchmarks still built along with tests ? If not how should they be invoked ?

Not along with tests, use VELOX_ENABLE_BENCHMARKS or VELOX_ENABLE_BENCHMARKS_BASIC to control whether to build benchmarks, and if one of this two flags is on, set VELOX_BUILD_TEST_UTILS to on for import some common test utilities

[leoluan2009]

We run benchmark comparisons to catch performance regressions on merge to main ; We need to ensure that these dont break , due to these changes.

This is still controlled by VELOX_ENABLE_BENCHMARKS_BASIC

[kgpai]

Thanks @leoluan2009 , Can you make changes here : https://github.com/facebookincubator/velox/blob/main/.circleci/config.yml#L247 so that the benchmarks builds are not broken, as a part of this PR.

[majetideepak]

@leoluan2009 thank you for this work. I actually have an issue open here #1704

[leoluan2009]

@leoluan2009 thank you for this work. I actually have an issue open here #1704

Yes, I think I already follow the design for share functionality in #1704

[majetideepak]

@kgpai the benchmarks are still running with this change.
The benchmark target uses the right option.

velox/Makefile

Lines 95 to 96 in d9f1373

	benchmarks-basic-build:
	$(MAKE) release EXTRA_CMAKE_FLAGS="-DVELOX_BUILD_MINIMAL=ON -DVELOX_ENABLE_BENCHMARKS_BASIC=ON"

https://app.circleci.com/pipelines/github/facebookincubator/velox/12821/workflows/44ace784-d480-4dfe-9d36-fbc6d928d8da/jobs/76628

[majetideepak]

@leoluan2009 nit: I guess we can remove the TODO here https://github.com/facebookincubator/velox/blob/main/CMakeLists.txt#L326

[leoluan2009]

@leoluan2009 nit: I guess we can remove the TODO here https://github.com/facebookincubator/velox/blob/main/CMakeLists.txt#L326

@majetideepak Done, thanks

[majetideepak]

@kgpai @Yuhta can you please merge this? Thanks.

[leoluan2009]

cc @majetideepak @kgpai @Yuhta

[facebook-github-bot]

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

[kgpai]

@majetideepak I will change bunch of places where relative path is used instead of absolute path.

[majetideepak]

@majetideepak I will change bunch of places where relative path is used instead of absolute path.

@kgpai thank you.

[leoluan2009]

Hi, @majetideepak @kgpai, I fix all comments, would you please review again? Thanks!