Skip to content

Conversation

@cmcfarlen
Copy link
Contributor

This also simplifies linking hwloc

@cmcfarlen cmcfarlen added the CMake work related to CMakes scripts or issues label Aug 25, 2023
@cmcfarlen cmcfarlen added this to the 10.0.0 milestone Aug 25, 2023
@cmcfarlen cmcfarlen self-assigned this Aug 25, 2023

TEST_CASE("Huffmancode Random", "[proxy][huffman]")
{
// This doesn't check anything ...
Copy link
Contributor

Choose a reason for hiding this comment

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

Then why does it exist?

CMakeLists.txt Outdated
Comment on lines 310 to 311
add_library(catch::catch INTERFACE IMPORTED)
target_include_directories(catch::catch INTERFACE ${CMAKE_SOURCE_DIR}/include ${CATCH_INCLUDE_DIR})
Copy link
Contributor

Choose a reason for hiding this comment

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

I thought there was already a target? catch2::catch2 maybe? Their own official target last I checked is Catch2::Catch2WithMain, which we don't use (but maybe should for consistency).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We vendor the catch header file so we don't use the find package. I just wanted the prettier way of getting the include 😄

Copy link
Contributor

Choose a reason for hiding this comment

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

lib/CMakeLists.txt#L35-36

Copy link
Contributor Author

Choose a reason for hiding this comment

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

oh, that's not in the convenient spot!

unit_tests/unit_test_main.cc
)
target_link_libraries(test_proxy_hdrs PRIVATE ts::hdrs ts::tscore ts::inkevent catch::catch)
add_test(NAME test_proxy_hdrs COMMAND $<TARGET_FILE:test_proxy_hdrs>)
Copy link
Contributor

Choose a reason for hiding this comment

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

Is the generator expression necessary? My understanding of the add_test docs was that if the command is the name of a target, it automatically uses that targets binary output.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I don't think it is necessary but I was just following suit from other invocations of add_test

@cmcfarlen cmcfarlen mentioned this pull request Aug 25, 2023
9 tasks
@bryancall bryancall self-requested a review August 28, 2023 22:31
Copy link
Contributor

@JosiahWI JosiahWI left a comment

Choose a reason for hiding this comment

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

I wonder whether it's possible to put all the hwloc stuff in one place instead of sprinkling conditional code around in multiple places.

Comment on lines 37 to 40
if(TS_USE_HWLOC)
target_link_libraries(traffic_layout PRIVATE hwloc::hwloc)
endif()

Copy link
Contributor

Choose a reason for hiding this comment

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

info.cc needs to get the API version from hwloc if it's enabled, so hwloc is a usage requirement.

Comment on lines 52 to 55

if(TS_USE_HWLOC)
target_link_libraries(inkevent PRIVATE hwloc::hwloc)
endif()
Copy link
Contributor

Choose a reason for hiding this comment

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

UnixEventProcessor.cc uses symbols from hwloc, so it is a usage requirement.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I removed this because its pulled in transitively

Copy link
Contributor

Choose a reason for hiding this comment

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

If the hwloc related code is that tightly coupled, it probably shouldn't be in 2 different components. If that's not the case, then imagine a scenario where we no longer need the hwloc code in tscore. Eventsystem will fail to compile, and the author will have to patch the inkevent CMakeLists as well, requiring a change in 2 places instead of 1.

unit_tests/test_XPACK.cc
)
target_include_directories(test_proxy_hdrs_xpack PRIVATE ${CMAKE_CURRENT_SOURCE_DIR})
target_link_libraries(test_proxy_hdrs_xpack PRIVATE ts::tscore libswoc catch2::catch2)
Copy link
Contributor

@JosiahWI JosiahWI Aug 29, 2023

Choose a reason for hiding this comment

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

Also depends on tscpputil. Doesn't depend on libswoc (that I can find).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

tscore needs libswoc but doesn't have it PUBLIC

Copy link
Contributor

Choose a reason for hiding this comment

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

It almost certainly should be PUBLIC.

@cmcfarlen cmcfarlen force-pushed the cmake-test-proxy-hdrs branch from 6447fb3 to 83b331c Compare August 29, 2023 22:13

if(TS_USE_HWLOC)
target_link_libraries(traffic_layout PRIVATE hwloc::hwloc)
target_link_libraries(inkevent PRIVATE hwloc::hwloc)
Copy link
Contributor

Choose a reason for hiding this comment

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

Should be traffic_layout, not inkevent.

Copy link
Contributor

@JosiahWI JosiahWI left a comment

Choose a reason for hiding this comment

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

Looks good, thanks!

@cmcfarlen cmcfarlen merged commit bb1d435 into apache:master Aug 30, 2023
@cmcfarlen cmcfarlen deleted the cmake-test-proxy-hdrs branch August 30, 2023 21:58
cmcfarlen pushed a commit to cmcfarlen/trafficserver that referenced this pull request Jun 3, 2024
* asf/master:
  Make bad disk detection more robust (apache#10317)
  Update Proxy Verifier to v2.10.1 (apache#10322)
  Fix lua plugin build (apache#10319)
  cmake: Add support for building benchmarks (apache#10316)
  cmake: add tests for proxy/http3 (apache#10310)
  Cmake: Add tests in proxy/http proxy/http2 (apache#10305)
  add tests for src/records (apache#10302)
  cmake: add unit tests for proxy/logging (apache#10301)
  setup default install path, runtime user and group (apache#10299)
  cmake: add tests for proxy/hdrs (apache#10283)
  Fix ip_allow optional methods specification (apache#10246)
  Plugin promotions, deprecations and deletions (apache#10303)
  Use Dbg() for debug output in both core and plugins. (apache#9732)
  Remove in_addr forward declaration from experimental.h. (apache#10309)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

CMake work related to CMakes scripts or issues

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants