Skip to content

[GLUTEN-6562][VL] Decouple BUILD_BENCHMARKS and BUILD_TESTS build options#6563

Merged
PHILO-HE merged 5 commits intoapache:mainfrom
NEUpanning:enable_build_benchmark
Jul 24, 2024
Merged

[GLUTEN-6562][VL] Decouple BUILD_BENCHMARKS and BUILD_TESTS build options#6563
PHILO-HE merged 5 commits intoapache:mainfrom
NEUpanning:enable_build_benchmark

Conversation

@NEUpanning
Copy link
Contributor

@NEUpanning NEUpanning commented Jul 23, 2024

What changes were proposed in this pull request?

Building Gluten tests depends on gluten benchmark , this pr decouples them.

Fixes: #6562

How was this patch tested?

I successfully built gluten tests with ./compile.sh --build_velox_backend=ON --build_tests=ON --build_examples=ON that BUILD_BENCHMARKS is not enabled

@github-actions
Copy link

Thanks for opening a pull request!

Could you open an issue for this pull request on Github Issues?

https://github.com/apache/incubator-gluten/issues

Then could you also rename commit message and pull request title in the following format?

[GLUTEN-${ISSUES_ID}][COMPONENT]feat/fix: ${detailed message}

See also:

@NEUpanning NEUpanning changed the title [VL]Set BUILD_BENCHMARKS=ON if BUILD_TESTS is enabled [GLUTEN-6562][VL]Set BUILD_BENCHMARKS=ON if BUILD_TESTS is enabled Jul 23, 2024
@github-actions
Copy link

#6562

@PHILO-HE
Copy link
Member

@xumingming, I note you also found this issue, could you take a review?

@xumingming
Copy link
Contributor

Is it possible to not mix BUILD_TESTS and BUILD_BENCHMARKS together? I think it is cleaner, what we need to do is do not let tests code depends on benchmark related code.

@PHILO-HE
Copy link
Member

Is it possible to not mix BUILD_TESTS and BUILD_BENCHMARKS together? I think it is cleaner, what we need to do is do not let tests code depends on benchmark related code.

@xumingming, I agree. @NEUpanning, we may need to extract out the shared code from benchmark module.

@NEUpanning
Copy link
Contributor Author

@PHILO-HE @xumingming I have updated the code so that the gluten tests no longer depends on the gluten benchmarks.

target_link_libraries(${TEST_EXEC} velox_benchmark_common GTest::gtest
GTest::gtest_main)
target_link_libraries(${TEST_EXEC} velox GTest::gtest GTest::gtest_main
google::glog benchmark::benchmark)
Copy link
Contributor

Choose a reason for hiding this comment

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

Why benchmark::benchmark is needed?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks. I've remove it.

@xumingming
Copy link
Contributor

Looks good to me.

@NEUpanning NEUpanning force-pushed the enable_build_benchmark branch from 7b90371 to 803b99f Compare July 24, 2024 06:24
Copy link
Member

@PHILO-HE PHILO-HE left a comment

Choose a reason for hiding this comment

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

Thanks!

@PHILO-HE PHILO-HE changed the title [GLUTEN-6562][VL]Set BUILD_BENCHMARKS=ON if BUILD_TESTS is enabled [GLUTEN-6562][VL] Decouple BUILD_BENCHMARKS and BUILD_TESTS build options Jul 24, 2024
@PHILO-HE PHILO-HE merged commit 8ab0b10 into apache:main Jul 24, 2024
@NEUpanning NEUpanning deleted the enable_build_benchmark branch July 26, 2024 04:13
weiting-chen pushed a commit to weiting-chen/gluten that referenced this pull request Nov 12, 2024
PHILO-HE pushed a commit that referenced this pull request Nov 18, 2024
@weiting-chen weiting-chen mentioned this pull request Nov 20, 2024
37 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Can't build gluten tests when BUILD_BENCHMARKS is not enabled

3 participants