Page MenuHomePhabricator

[services] Tunnelbroker - Moving CMakeLists file to use for gTest only
ClosedPublic

Authored by max on Oct 20 2022, 7:35 AM.
Tags
None
Referenced Files
F2194194: D5430.id.diff
Fri, Jul 5, 2:19 AM
Unknown Object (File)
Thu, Jul 4, 2:17 AM
Unknown Object (File)
Wed, Jul 3, 2:44 PM
Unknown Object (File)
Tue, Jul 2, 11:19 PM
Unknown Object (File)
Mon, Jul 1, 4:48 PM
Unknown Object (File)
Sun, Jun 30, 12:02 PM
Unknown Object (File)
Sun, Jun 30, 8:39 AM
Unknown Object (File)
Sun, Jun 30, 7:31 AM

Details

Summary

This diff is a part of the flat diffs stack to transform the current Tunnelbroker project into a Rust app by calling the current C++ codebase from Rust. The related transition task is ENG-1072.

This diff moves the current CMakeLists.txt file into the gtest test directory and makes it reduced to build only the gtest app for unit testing purposes only.

Related Linear task and discussion: ENG-2083

CI Notice:
The CI will fail on this and further diffs until the D5461 where the changes to CI build commands are made.
This stack will be landed all in one to prevent CI from failing on diffs out of this stack.

Test Plan
  1. Test current diff CMakeLists test build:

Go to the C++ tests directory: services/tunnelbroker/src/libcpp/test.
Run the CMake build using Nix: nix develop --accept-flake-config --command bash -c "cmake -B build . && make -C build -j".
The expected result is the successfully building of the runTests app for gTests.

  1. Test completed stack:

To test it patch to D5436 using arc patch D5436, run nix develop.
Go to tunnelbroker service directory services/tunnelbroker and run the Cargo building process cargo build or cargo run.
The Rust application will be successfully built.

  1. CI Testing:

These diff changes are included as a part of the stack in the later D5461 diff, where the changes to CI build commands are made. The CI succeded on the D5461 which reflects that this changes are passing the build.

Diff Detail

Repository
rCOMM Comm
Lint
Lint Not Applicable
Unit
Tests Not Applicable

Event Timeline

max held this revision as a draft.
max edited the test plan for this revision. (Show Details)
max added reviewers: varun, marcin. max added 1 blocking reviewer(s): jon.
max published this revision for review.Oct 20 2022, 8:24 AM
jon requested changes to this revision.Oct 21 2022, 9:08 AM

please also update the path used by the tunnel broker tests under .buildkite/

This revision now requires changes to proceed.Oct 21 2022, 9:08 AM
max edited the test plan for this revision. (Show Details)
max added 1 blocking reviewer(s): jon.
This revision now requires review to proceed.Oct 24 2022, 3:16 AM

Fixing shared path, removing of unused commands, changes the stack order.

Removed unused and deprecated requirements.
Fixed GTest include in Docker(Ubuntu) build.

max edited the test plan for this revision. (Show Details)
In D5430#161065, @jon wrote:

please also update the path used by the tunnel broker tests under .buildkite/

Sure, it's done in a further D5461 diff.

cc @atul who is somewhat familiar with CMake

max added a reviewer: atul. max removed 1 blocking reviewer(s): jon.Oct 26 2022, 2:13 AM

Sure, it's done in a further D5461 diff.

Sounds good to me

services/tunnelbroker/src/libcpp/test/CMakeLists.txt
5 ↗(On Diff #17863)

should be done at the target level

https://github.com/CommE2E/comm/blob/e7b0a922126f415741569eb78edbd7a65f8826b0/native/cpp/CommonCpp/grpc/CMakeLists.txt#L49-L53

I don't think tests are all that worthwhile, as running the tests can be done from the build directory. But I would be happy to be proven wrong.

services/tunnelbroker/src/libcpp/test/CMakeLists.txt
5 ↗(On Diff #17863)

I don't think tests are all that worthwhile

I meant to say, all that worthwhile to declare install output paths, as we will likely be running the tests from the build directory.

atul added 1 blocking reviewer(s): jon.

cc @atul who is somewhat familiar with CMake

Looks good to me. Accepting and setting @jon as blocking

services/tunnelbroker/src/libcpp/test/CMakeLists.txt
7–9 ↗(On Diff #17863)

Can we add a quick comment explaining what the CMP0003 policy is? Just a sentence and a link to https://cmake.org/cmake/help/latest/policy/CMP0003.html should suffice.

Adding a comment for CMP0003 policy

max added inline comments.
services/tunnelbroker/src/libcpp/test/CMakeLists.txt
5 ↗(On Diff #17863)

should be done at the target level

https://github.com/CommE2E/comm/blob/e7b0a922126f415741569eb78edbd7a65f8826b0/native/cpp/CommonCpp/grpc/CMakeLists.txt#L49-L53

I don't think tests are all that worthwhile, as running the tests can be done from the build directory. But I would be happy to be proven wrong.

Yes, we are running the tests app from the build directory (in D5461).

What's the benefits to use of install TARGETS... here instead of setting a simple CMAKE_RUNTIME_OUTPUT_DIRECTORY ?

For me, it makes sense for the blob as we are exporting headers for the client and etc. In this case, just build and run the tests and we can go with the simple solution.

7–9 ↗(On Diff #17863)

Can we add a quick comment explaining what the CMP0003 policy is? Just a sentence and a link to https://cmake.org/cmake/help/latest/policy/CMP0003.html should suffice.

I think the simple one would be enough. I've added it.

services/tunnelbroker/src/libcpp/test/CMakeLists.txt
5 ↗(On Diff #17863)

What's the benefits to use of install TARGETS... here instead of setting a simple CMAKE_RUNTIME_OUTPUT_DIRECTORY ?

If you do add_subdirectory and target this file, then you're not going to clobber the global definition of what should go where.

It's considered best practice to avoid the old global behavior options of cmake, and instead encapsulate behavior on a per-target basis.

For me, it makes sense for the blob as we are exporting headers for the client and etc. In this case, just build and run the tests and we can go with the simple solution.

I'm pretty sure if you remove the line, the default is that it will build runTests in ${CMAKE_CURRENT_BINARY_DIR}/bin/ anyway. I would just like to avoid footguns, as the GLOB RECURSIVE has been bitting us as well lately.

max added inline comments.
services/tunnelbroker/src/libcpp/test/CMakeLists.txt
5 ↗(On Diff #17863)

What's the benefits to use of install TARGETS... here instead of setting a simple CMAKE_RUNTIME_OUTPUT_DIRECTORY ?

If you do add_subdirectory and target this file, then you're not going to clobber the global definition of what should go where.

It's considered best practice to avoid the old global behavior options of cmake, and instead encapsulate behavior on a per-target basis.

For me, it makes sense for the blob as we are exporting headers for the client and etc. In this case, just build and run the tests and we can go with the simple solution.

I'm pretty sure if you remove the line, the default is that it will build runTests in ${CMAKE_CURRENT_BINARY_DIR}/bin/ anyway. I would just like to avoid footguns, as the GLOB RECURSIVE has been bitting us as well lately.

Thanks for the clarification @jon!
Despite the CMake documentation saying that the default dir for the RUNTIME is bin when I remove the CMAKE_RUNTIME_OUTPUT_DIRECTORY and add:

install(TARGETS runTests
  RUNTIME DESTINATION ${CMAKE_INSTALL_BINDIR}
  LIBRARY DESTINATION ${CMAKE_INSTALL_LIBDIR}
  ARCHIVE DESTINATION ${CMAKE_INSTALL_LIBDIR}
)

the binary is in the build directory as well if I use just install(TARGETS runTests RUNTIME DESTINATION bin) which must use build/bin directory. But it's not and puts the binary into the build directory.

I don't mind removing the CMAKE_RUNTIME_OUTPUT_DIRECTORY and just using the build for the binary. I'll adjust the commands for the build directory in D5461 as well if we will go with it and this will be accepted.

max marked an inline comment as done.

Removing of the CMAKE_RUNTIME_OUTPUT_DIRECTORY.

the binary is in the build directory as well if I use just install(TARGETS runTests RUNTIME DESTINATION bin) which must use build/bin directory. But it's not and puts the binary into the build directory.

The INSTALL(TARGETS ...) cmake options only come into play when you do something like cmake .. && make install. Otherwise it doesn't do much.

This revision is now accepted and ready to land.Nov 4 2022, 10:57 AM
In D5430#163958, @jon wrote:

the binary is in the build directory as well if I use just install(TARGETS runTests RUNTIME DESTINATION bin) which must use build/bin directory. But it's not and puts the binary into the build directory.

The INSTALL(TARGETS ...) cmake options only come into play when you do something like cmake .. && make install. Otherwise it doesn't do much.

Right 🤦‍♂️ I've missed it. Thanks, @jon.

Rebasing on parent changes.

This revision was landed with ongoing or failed builds.Nov 7 2022, 6:00 AM
This revision was automatically updated to reflect the committed changes.