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

Confusion over WORKING_DIRECTORY #406

Closed
icolwell-as opened this issue Oct 20, 2022 · 4 comments
Closed

Confusion over WORKING_DIRECTORY #406

icolwell-as opened this issue Oct 20, 2022 · 4 comments

Comments

@icolwell-as
Copy link

Hi There, I recently spent a lot of time trying to understand why some of my unit tests that worked fine in ROS2 Dashing were failing in ROS2 Foxy. I hope the below helps other people who may come across the same issue.

According to Dashing, Foxy, and Humble documentation, the following is stated about the default WORKING_DIRECTORY for the ament_add_gtest() macro.

The default working directory otherwise is the CMAKE_SOURCE_DIR, which will be evaluated to the directory of the top-level CMakeLists.txt.

However, this only seems to be true for Dashing (maybe the intermediate releases too, I didn't use them). PR #206 changed the default behaviour from CMAKE_SOURCE_DIR to CMAKE_CURRENT_BINARY_DIR.

This leaves me with a few questions:

  1. What is the best way to use the old behaviour? In my case, I have test data/files in the source repo but that data isn't available in CMAKE_CURRENT_BINARY_DIR. I also have about 10-15 ament_add_gtest() calls. Is there some way to set the default once instead of adding WORKING_DIRECTORY "${CMAKE_SOURCE_DIR}" to every ament_add_gtest() call? Or is it better practice to copy test data into the build directory only, but not the install directory?
  2. Is there a better source of documentation for ament/ament_cmake? or is docs.ros.org the best place to be looking?

I guess there aren't any action items for this repo, but I suppose I could open a documentation ticket that references this one.
I think Foxy was the first release to have this guide? I'm not sure.

@clalancette
Copy link
Contributor

However, this only seems to be true for Dashing (maybe the intermediate releases too, I didn't use them). PR #206 changed the default behaviour from CMAKE_SOURCE_DIR to CMAKE_CURRENT_BINARY_DIR.

Yes, very good call.

  1. What is the best way to use the old behaviour? In my case, I have test data/files in the source repo but that data isn't available in CMAKE_CURRENT_BINARY_DIR. I also have about 10-15 ament_add_gtest() calls. Is there some way to set the default once instead of adding WORKING_DIRECTORY "${CMAKE_SOURCE_DIR}" to every ament_add_gtest() call? Or is it better practice to copy test data into the build directory only, but not the install directory?

I think our current practice is to just specify it for every test. I don't know that we have a way to set it once. See https://github.com/ros2/rcl/blob/rolling/rcl_yaml_param_parser/CMakeLists.txt , for example.

  • Is there a better source of documentation for ament/ament_cmake? or is docs.ros.org the best place to be looking?

docs.ros.org is the right place. If you would open a pull request to fix up https://github.com/ros2/ros2_documentation/blob/rolling/source/How-To-Guides/Ament-CMake-Documentation.rst , I'd be happy to review it.

@icolwell-as
Copy link
Author

Thank you very much @clalancette for the fast response!

I'll open a ticket in the documentation repo and get a PR up within the next couple of days.

The example you provided is now making me wonder if we should revert #206? I understand it's nice to follow cmake behaviour, but if the majority of the ROS community is running tests with data/files, maybe CMAKE_SOURCE_DIR is a more appropriate default behaviour?

@clalancette
Copy link
Contributor

The example you provided is now making me wonder if we should revert #206? I understand it's nice to follow cmake behaviour, but if the majority of the ROS community is running tests with data/files, maybe CMAKE_SOURCE_DIR is a more appropriate default behaviour?

I will say that at least in the core, we only need to set WORKING_DIRECTORY in a few places; most places don't need it. Looking through all of the packages released into Rolling, it looks like only ~22% need to use it[1]. Additionally, I don't think it is a good idea to switch back and forth all of the time; that just causes churn in the ecosystem.

Given all of that, my preference would be to leave things as-is and fix up the documentation.

[1] I calculated this number by pulling all of the sources of packages released into Rolling, counting the number of ament_add_gtest instances (558), counting the number of WORKING_DIRECTORY instances that used SOURCE (126), and then dividing. This won't be an exact number; some of the instances of ament_add_gtest are going to be the actual definition of it, so the actual number is lower. Also, I'm not sure that all of these tests pass. Still, I think this is good enough for an estimate.

@icolwell-as
Copy link
Author

Thanks @clalancette, I agree, probably best to leave it as-is for now.
It can always be re-addressed in the future if there truly comes a need for it.

Thanks for your thoughts! I'll close this out and try to update the documentation soon.

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

No branches or pull requests

2 participants