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

Removes custom gtest package and uses ament_cmake_gtest instead #717

Merged
merged 9 commits into from
Nov 17, 2020

Conversation

agalbachicar
Copy link
Collaborator

Solves #714

@agalbachicar agalbachicar requested review from scpeters and removed request for scpeters November 9, 2020 15:16
@agalbachicar agalbachicar force-pushed the agalbachicar/#714_use_ament_gtest_gmock branch from 64bff3c to a51a834 Compare November 12, 2020 17:56
@agalbachicar
Copy link
Collaborator Author

It's ready for your review @scpeters

Copy link
Contributor

@scpeters scpeters 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 to me; I would just delete rather than commenting out many of these lines

gtest
gtest_main
# gtest
# gtest_main
Copy link
Contributor

Choose a reason for hiding this comment

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

I would remove these lines rather than comment them out

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Sorry, I missed that


set_tests_properties(${BINARY_NAME} PROPERTIES TIMEOUT 240)
# set_tests_properties(${BINARY_NAME} PROPERTIES TIMEOUT 240)
Copy link
Contributor

Choose a reason for hiding this comment

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

I would remove these lines rather than comment them out

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Sorry, I missed that

@@ -15,7 +17,8 @@ macro(delphyne_build_tests)
string(REGEX REPLACE ".cc" "" BINARY_NAME ${GTEST_SOURCE_file})
set(BINARY_NAME ${TEST_TYPE}_${BINARY_NAME})

add_executable(${BINARY_NAME} ${GTEST_SOURCE_file} ${sources})
# add_executable(${BINARY_NAME} ${GTEST_SOURCE_file} ${sources})
Copy link
Contributor

Choose a reason for hiding this comment

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

I would remove these lines rather than comment them out

@agalbachicar agalbachicar force-pushed the agalbachicar/#714_use_ament_gtest_gmock branch from 32eded2 to 73271ce Compare November 13, 2020 14:26
@scpeters
Copy link
Contributor

CI failing until #721 is merged

@agalbachicar agalbachicar removed the request for review from scpeters November 16, 2020 17:47
…ToyotaResearchInstitute/delphyne into agalbachicar/#714_use_ament_gtest_gmock
@agalbachicar
Copy link
Collaborator Author

@scpeters whenever you have a moment, please take a look at the output of the jobs. You will find the following log:

> Invoking "sudo locale-gen en_US en_US.UTF-8"
Error: Unable to process command '::set-env name=LANG::en_US.UTF-8' successfully.
Error: The `set-env` command is disabled. Please upgrade to using Environment Files or opt into unsecure command execution by setting the `ACTIONS_ALLOW_UNSECURE_COMMANDS` environment variable to `true`. For more information see: https://github.blog/changelog/2020-10-01-github-actions-deprecating-set-env-and-add-path-commands/

and then it continues but with a failure when running:

> Invoking "sudo rosdep init"
  /usr/bin/sudo rosdep init
  Wrote /etc/ros/rosdep/sources.list.d/20-default.list
  Recommended: please run
  
  	rosdep update

In https://github.com/ToyotaResearchInstitute/delphyne/runs/1408073772?check_suite_focus=true it can be seen that adding the suggested variable above made the step successful.

@agalbachicar
Copy link
Collaborator Author

This is affecting all the jobs, so I will add the fix everywhere to restore CI, then we should probably reach out to https://github.com/ros-tooling/setup-ros so they are aware of it.

Copy link
Contributor

@scpeters scpeters left a comment

Choose a reason for hiding this comment

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

I am still investigating why ACTIONS_ALLOW_UNSECURE_COMMANDS is necessary, but I think we can merge this for now

@agalbachicar agalbachicar merged commit 5a6e5fd into master Nov 17, 2020
@agalbachicar agalbachicar deleted the agalbachicar/#714_use_ament_gtest_gmock branch November 17, 2020 17:40
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.

2 participants