Fix use of Backward on macOS #67

azeey · 2021-10-18T19:04:02Z

🦟 Bug fix

Summary

SDFormat CI (eg. https://build.osrfoundation.org/job/sdformat-ci-sdformat10-homebrew-amd64/48/) is failing on macOS because ign fails to find libignition-tools-backward. The problem is that $<TARGET_FILE> yields the full path of the library in the build
directory, which will likely not be available in binary distributions of ign-tools. This instead uses the install path of the library.

Checklist

Signed all commits for DCO
Added tests
Updated documentation (as needed)
Updated migration guide (as needed)
codecheck passed (See contributing)
All tests passed (See test coverage)
While waiting for a review on your PR, please help review another open pull request to support the maintainers

Note to maintainers: Remember to use Squash-Merge

`$<TARGET_FILE>` yields the full path of the library in the build directory, which will likely not be available in binary distributions of ign-tools. This instead uses the install path of the library. Signed-off-by: Addisu Z. Taddese <addisu@openrobotics.org>

scpeters · 2021-10-18T19:11:02Z

in downstream packages, we typically configure/generate two cmd*.rb scripts, one for testing and one for installation:

https://github.com/ignitionrobotics/ign-transport/blob/ignition-transport11_11.0.0/src/cmd/CMakeLists.txt#L63-L108

I think we should consider doing that here and adding a test that invokes ign

Signed-off-by: Addisu Z. Taddese <addisu@openrobotics.org>

azeey · 2021-10-19T17:37:01Z

I think we should consider doing that here and adding a test that invokes ign

I've added tests that use cmake to invoke ign, but I don't know how to tell Jenkins about the test results. Do you have any recommendations? The other option is to add gtest and call the tests from an executable built with gtest.

scpeters · 2021-10-20T08:49:59Z

I think we should consider doing that here and adding a test that invokes ign

I've added tests that use cmake to invoke ign, but I don't know how to tell Jenkins about the test results. Do you have any recommendations? The other option is to add gtest and call the tests from an executable built with gtest.

here's an example from ign-cmake2 that configures junit files for pass and failure. It configures the failure case to the test_results folder, then copies the junit_pass file if the test succeeds:

it's a little clunky, but it doesn't need gtest

scpeters

thanks for the quick fix here; I have two small comments about the implementation

scpeters · 2021-10-20T09:40:21Z

src/ign.in

 rescue SharedLibInterface::DLError
-  puts "Library error: #{backward_library_name} not found. Improved backtrace"\
+  puts 'Library error: @backward_library_name@ not found. Improved backtrace'\


I think we can use STDERR.puts here, which might also help with the test failures that have expectations on the stdout

Done in 311d588. I used warn instead because codecheck didn't like STDERR.puts.

scpeters · 2021-10-20T09:41:48Z

src/CMakeLists.txt

+  set(cmd_script_generated_test "${PROJECT_BINARY_DIR}/test/lib/$<CONFIG>/ruby/ignition/cmd${IGN_DESIGNIATION}_TEST.rb")
+
+  configure_file(
+    "cmd${IGN_DESIGNIATION}_TEST.rb.in"


I don't see any @ symbols in this file that need configuring. I think we can just add it as cmdtools_TEST.rb

Done in 311d588.

scpeters · 2021-10-20T09:54:33Z

src/CMakeLists.txt

+    OUTPUT "${yaml_output_generated_test}"
+    INPUT "${yaml_output_configured_test}")
+
+  add_test(NAME UNIT_ign_TEST COMMAND ruby ${ign_script_generated_test} tools_TEST)


calling ruby like this will work if it's in the PATH. In some cases, we may need to find_program and refer to the cmake variable here, but we can address that once we see CI failures

Sounds good.

Signed-off-by: Addisu Z. Taddese <addisu@openrobotics.org>

azeey · 2021-10-22T19:47:06Z

The changes in 311d588 output test results that I thought were consumable by Jenkins based on the examples listed in #67 (comment). But Jenkins still says no tests found.

Also, I see a pending ign_tools-pr-win build as well a completed ignition_tools-ci-pr_any-windows7-amd64 in the list of checks. Is that related to gazebo-tooling/release-tools#537 @scpeters?

scpeters

just one small suggestion; looks good

scpeters · 2021-10-22T22:14:34Z

src/CMakeLists.txt

+# Used for the installed version.
+if(APPLE)
+  # On macOS, the full path to the library since DYLD_LIBRARY_PATH may not work.
+  set(backward_library_name ${CMAKE_INSTALL_PREFIX}/${LIB_INSTALL_DIR}/$<TARGET_FILE_NAME:backward>)


someone recently submitted gazebosim/gazebo-classic#3076 to change ${CMAKE_INSTALL_PREFIX}/${LIB_INSTALL_DIR} to ${CMAKE_INSTALL_FULL_LIBDIR}, so I think we could do the same here

Signed-off-by: Addisu Z. Taddese <addisu@openrobotics.org>

scpeters · 2021-10-22T22:54:32Z

The changes in 311d588 output test results that I thought were consumable by Jenkins based on the examples listed in #67 (comment). But Jenkins still says no tests found.

there's a DSL change needed to expect tests from ign-tools

https://github.com/ignition-tooling/release-tools/blob/master/jenkins-scripts/dsl/ignition.dsl#L41

Also, I see a pending ign_tools-pr-win build as well a completed ignition_tools-ci-pr_any-windows7-amd64 in the list of checks. Is that related to ignition-tooling/release-tools#537 @scpeters?

yes it's related to gazebo-tooling/release-tools#537. I'll enable tests for ign-tools in that PR and then request your review

scpeters · 2021-10-22T23:01:50Z

@osrf-jenkins run tests please

scpeters · 2021-10-22T23:04:12Z

the ign_tools-pr-win build won't work until gazebo-tooling/release-tools#537 is merged since it can only run off the master branch of release-tools

scpeters · 2021-10-22T23:04:39Z

tests are working on ubuntu: https://build.osrfoundation.org/job/ignition_tools-ci-pr_any-ubuntu_auto-amd64/191/testReport/(root)/

azeey · 2021-10-25T17:52:12Z

@osrf-jenkins run tests please

scpeters · 2021-10-25T17:55:53Z

there's one more change needed in release-tools: gazebo-tooling/release-tools#540

I had already restarted a CI job and saw its failure

scpeters · 2021-10-25T18:02:25Z

there's one more change needed in release-tools: ignition-tooling/release-tools#540

I had already restarted a CI job and saw its failure

this has been merged and CI is now working

scpeters · 2021-10-25T18:32:36Z

@osrf-jenkins run tests please

Blast545 · 2021-10-26T17:02:06Z

FYI @azeey, the error is still appearing in the buildfarm: build.osrfoundation.org/sdformat-ci-sdformat9-homebrew-amd64#81/. Not sure if I'm missing something.

azeey · 2021-10-26T17:07:12Z

We need to make a patch release for macOS to fix it there. The plan is to make a release for all platforms today pending gazebo-release/gz-tools-release#2. If that doesn't get merged today, we'll do a patch release just for macOS. \cc @j-rivero

azeey requested a review from caguero as a code owner October 18, 2021 19:04

github-actions bot added 🌱 garden Ignition Garden 🏢 edifice Ignition Edifice 🏯 fortress Ignition Fortress 🏰 citadel Ignition Citadel 🔮 dome Ignition Dome labels Oct 18, 2021

azeey added 4 commits October 19, 2021 11:10

Add test

967c891

Signed-off-by: Addisu Z. Taddese <addisu@openrobotics.org>

Codecheck

99f23c0

Signed-off-by: Addisu Z. Taddese <addisu@openrobotics.org>

Add option to disable test

77e0f98

Signed-off-by: Addisu Z. Taddese <addisu@openrobotics.org>

Add ctest

cb9c102

Signed-off-by: Addisu Z. Taddese <addisu@openrobotics.org>

scpeters reviewed Oct 20, 2021

View reviewed changes

azeey added 2 commits October 22, 2021 13:48

Improve test reliability, output test results consumable by Jenkins

311d588

Signed-off-by: Addisu Z. Taddese <addisu@openrobotics.org>

Fix style

4d59184

Signed-off-by: Addisu Z. Taddese <addisu@openrobotics.org>

scpeters reviewed Oct 22, 2021

View reviewed changes

Change to CMAKE_INSTALL_FULL_LIBDIR

abb4da5

Signed-off-by: Addisu Z. Taddese <addisu@openrobotics.org>

scpeters mentioned this pull request Oct 22, 2021

ignition.dsl: colcon for ign-tools on windows gazebo-tooling/release-tools#537

Merged

chapulina added the macOS macOS support label Oct 25, 2021

azeey mentioned this pull request Oct 25, 2021

👨‍🌾 test regressions sdformat-ci-sdformat9-homebrew-amd64 buildfarm gazebosim/sdformat#735

Closed

scpeters approved these changes Oct 25, 2021

View reviewed changes

azeey merged commit 500b760 into gazebosim:ign-tools1 Oct 25, 2021

azeey deleted the fix_backward_macos branch October 25, 2021 19:02

traversaro mentioned this pull request Oct 29, 2021

libignition-tools v1.4.1 conda-forge/libignition-tools-feedstock#12

Merged

3 tasks

Tobias-Fischer mentioned this pull request Dec 1, 2021

libsdformat v12.2.0 conda-forge/libsdformat-feedstock#52

Merged

3 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix use of Backward on macOS #67

Fix use of Backward on macOS #67

azeey commented Oct 18, 2021

scpeters commented Oct 18, 2021

azeey commented Oct 19, 2021

scpeters commented Oct 20, 2021

scpeters left a comment

scpeters Oct 20, 2021

azeey Oct 22, 2021

scpeters Oct 20, 2021

azeey Oct 22, 2021

scpeters Oct 20, 2021

azeey Oct 22, 2021

azeey commented Oct 22, 2021

scpeters left a comment

scpeters Oct 22, 2021

scpeters commented Oct 22, 2021

scpeters commented Oct 22, 2021

scpeters commented Oct 22, 2021

scpeters commented Oct 22, 2021

azeey commented Oct 25, 2021

scpeters commented Oct 25, 2021

scpeters commented Oct 25, 2021

scpeters commented Oct 25, 2021

Blast545 commented Oct 26, 2021

azeey commented Oct 26, 2021

Fix use of Backward on macOS #67

Fix use of Backward on macOS #67

Conversation

azeey commented Oct 18, 2021

🦟 Bug fix

Summary

Checklist

scpeters commented Oct 18, 2021

azeey commented Oct 19, 2021

scpeters commented Oct 20, 2021

scpeters left a comment

Choose a reason for hiding this comment

scpeters Oct 20, 2021

Choose a reason for hiding this comment

azeey Oct 22, 2021

Choose a reason for hiding this comment

scpeters Oct 20, 2021

Choose a reason for hiding this comment

azeey Oct 22, 2021

Choose a reason for hiding this comment

scpeters Oct 20, 2021

Choose a reason for hiding this comment

azeey Oct 22, 2021

Choose a reason for hiding this comment

azeey commented Oct 22, 2021

scpeters left a comment

Choose a reason for hiding this comment

scpeters Oct 22, 2021

Choose a reason for hiding this comment

scpeters commented Oct 22, 2021

scpeters commented Oct 22, 2021

scpeters commented Oct 22, 2021

scpeters commented Oct 22, 2021

azeey commented Oct 25, 2021

scpeters commented Oct 25, 2021

scpeters commented Oct 25, 2021

scpeters commented Oct 25, 2021

Blast545 commented Oct 26, 2021

azeey commented Oct 26, 2021