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.

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

”cmake_minimum_required(VERSION 3.1)“ is inaccurate #1399

Closed
GskFID opened this issue May 16, 2022 · 9 comments · Fixed by #2581
Closed

”cmake_minimum_required(VERSION 3.1)“ is inaccurate #1399

GskFID opened this issue May 16, 2022 · 9 comments · Fixed by #2581
Assignees
Labels
bug Something isn't working do-not-stale good first issue Good for newcomers help wanted Good for taking. Extra help will be provided by maintainers

Comments

@GskFID
Copy link

GskFID commented May 16, 2022

There are many places to add tests with ”gtest_add_tests“, But gtest_add_tests is a new feature of Cmake 3.9 version, When I try to compile with 3.8.5, it doesn't compile the test code anyway, this confused me for a while.
image

@GskFID GskFID added the bug Something isn't working label May 16, 2022
@lalitb
Copy link
Member

lalitb commented May 16, 2022

Good point. We should -

  • Either not use gtest_add_tests() to add unit tests. (preferably)
  • Or make the minimum required cmake version as 3.9.

@lalitb lalitb added help wanted Good for taking. Extra help will be provided by maintainers good first issue Good for newcomers labels May 16, 2022
@esigo
Copy link
Member

esigo commented May 16, 2022

old cmake compatible example can be found here:

foreach(
testname
tracer_provider_test
span_data_test
simple_processor_test
tracer_test
always_off_sampler_test
always_on_sampler_test
parent_sampler_test
trace_id_ratio_sampler_test
batch_span_processor_test)
add_executable(${testname} "${testname}.cc")
target_link_libraries(
${testname}
${GTEST_BOTH_LIBRARIES}
${CMAKE_THREAD_LIBS_INIT}
opentelemetry_common
opentelemetry_trace
opentelemetry_resources
opentelemetry_exporter_in_memory)
gtest_add_tests(
TARGET ${testname}
TEST_PREFIX trace.
TEST_LIST ${testname})
endforeach()
add_executable(sampler_benchmark sampler_benchmark.cc)
target_link_libraries(
sampler_benchmark benchmark::benchmark ${CMAKE_THREAD_LIBS_INIT}
opentelemetry_trace opentelemetry_resources opentelemetry_exporter_in_memory)

@lalitb
Copy link
Member

lalitb commented May 16, 2022

old cmake compatible example can be found here:

foreach(
testname
tracer_provider_test
span_data_test
simple_processor_test
tracer_test
always_off_sampler_test
always_on_sampler_test
parent_sampler_test
trace_id_ratio_sampler_test
batch_span_processor_test)
add_executable(${testname} "${testname}.cc")
target_link_libraries(
${testname}
${GTEST_BOTH_LIBRARIES}
${CMAKE_THREAD_LIBS_INIT}
opentelemetry_common
opentelemetry_trace
opentelemetry_resources
opentelemetry_exporter_in_memory)
gtest_add_tests(
TARGET ${testname}
TEST_PREFIX trace.
TEST_LIST ${testname})
endforeach()
add_executable(sampler_benchmark sampler_benchmark.cc)
target_link_libraries(
sampler_benchmark benchmark::benchmark ${CMAKE_THREAD_LIBS_INIT}
opentelemetry_trace opentelemetry_resources opentelemetry_exporter_in_memory)

This example seems to be using gtest_add_tests at line 21, or am I missing something ?

@owent
Copy link
Member

owent commented May 17, 2022

I vote for upgrade the minimum version of cmake.It will be easier to maintain.

@esigo
Copy link
Member

esigo commented May 17, 2022

@lalitb sorry I was wrong, we have no example for the old Cmake version.
I agree with @owent, I'd vote for upgrading the minimum required cmake.

@lalitb
Copy link
Member

lalitb commented May 18, 2022

To see if we can use the gtest_add_tests() function from cmake module:
https://github.com/Kitware/CMake/blob/908d2cd136be3165d391c18777890798a18f96c3/Modules/GoogleTest.cmake#L293-L294

@lalitb
Copy link
Member

lalitb commented May 18, 2022

There are few more cmake constructs in code not supported in cmake v3.1.0:

if(${CMAKE_VERSION} VERSION_GREATER_EQUAL "3.12")

add_compile_definitions(WITH_CURL)

@owent
Copy link
Member

owent commented May 19, 2022

There are few more cmake constructs in code not supported in cmake v3.1.0:

if(${CMAKE_VERSION} VERSION_GREATER_EQUAL "3.12")

add_compile_definitions(WITH_CURL)

I think add_compile_definitions(WITH_CURL) can be removed now.

@lalitb lalitb self-assigned this Jun 1, 2022
@github-actions
Copy link

github-actions bot commented Aug 1, 2022

This issue was marked as stale due to lack of activity.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working do-not-stale good first issue Good for newcomers help wanted Good for taking. Extra help will be provided by maintainers
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants