Fix static builds and optimize test compilation #1343

azeey · 2023-11-01T23:40:13Z

🦟 Bug fix

Alternative to #1309

Summary

As alluded to in #1309, building sdformat with `-DBUILD_SHARED_LIBS=OFF ..' results in the following error

CMake Error: install(EXPORT "sdformat13" ...) includes target "sdformat13" which requires target "using_parser_urdf" that is not in any export set.
CMake Error in src/CMakeLists.txt:
  export called with target "sdformat13" which requires target
  "using_parser_urdf" that is not in any export set.

This is because we are creating the using_parser_urdf target and linking it against the core library target, which is then exported. When the core library is built as a static library, CMake requires that other targets linked against it are exported as well (see https://stackoverflow.com/a/71080574/283225 case (2)).

The solution in this PR is to compile the URDF sources directly into the core library instead of creating an extra target. While working on this, I realized we could create a static library that includes all the extra sources needed for tests instead of building each test with its own extra sources. I believe this will marginally improve compilation time, but more importantly, I think it cleans up the CMake file and will make adding tests easier.

Checklist

Signed all commits for DCO
Added tests
Updated documentation (as needed)
Updated migration guide (as needed)
Consider updating Python bindings (if the library has them)
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 and edit the commit message to match the pull request summary while retaining Signed-off-by messages.

Signed-off-by: Addisu Z. Taddese <[email protected]>

codecov · 2023-11-02T03:54:08Z

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (7a9dad5) 92.14% compared to head (2caaadb) 92.14%.
Report is 1 commits behind head on sdf12.

❗ Current head 2caaadb differs from pull request most recent head 639d070. Consider uploading reports for the commit 639d070 to get more accurate results

Additional details and impacted files

@@           Coverage Diff           @@
##            sdf12    #1343   +/-   ##
=======================================
  Coverage   92.14%   92.14%           
=======================================
  Files          79       79           
  Lines       13126    13126           
=======================================
  Hits        12095    12095           
  Misses       1031     1031

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

traversaro · 2023-11-02T09:11:28Z

src/CMakeLists.txt

-      ${CMAKE_CURRENT_SOURCE_DIR}
-      ${PROJECT_SOURCE_DIR}/test
-  )
+if (WIN32)


Suggested change

if (WIN32)

if (WIN32 AND USE_INTERNAL_URDF)

Unless I miss something, we do not need this if we are using the external urdfdom.

Yup, that makes sense. 5437306

Signed-off-by: Addisu Z. Taddese <[email protected]>

azeey · 2023-11-09T00:42:47Z

There's a cmake warning related to psutil which is not present in the stable CI builds. Comparing homebrew stable CI job with this PR jojb, I noticed the stable CI job has PIP_PACKAGES_NEEDED='psutil pytest' while the PR job has PIP_PACKAGES_NEEDED=' pytest' right before pip3 install. @j-rivero Did the recent changes in release-toolls cause this?

scpeters · 2023-11-14T21:49:48Z

There's a cmake warning related to psutil which is not present in the stable CI builds. Comparing homebrew stable CI job with this PR jojb, I noticed the stable CI job has PIP_PACKAGES_NEEDED='psutil pytest' while the PR job has PIP_PACKAGES_NEEDED=' pytest' right before pip3 install. @j-rivero Did the recent changes in release-toolls cause this?

yes, see #1346 (comment)

j-rivero · 2023-11-15T16:47:11Z

yes, see #1346 (comment)

Merged and deployed the change. Relaunched the brew -ci-pr_any- job.

scpeters

two small nits

scpeters · 2023-11-27T20:02:49Z

src/CMakeLists.txt

-      TINYXML2::TINYXML2)
-    target_sources(UNIT_XmlUtils_TEST PRIVATE XmlUtils.cc)
-  endif()
+  # Link against the publically and privately linked libraries of the core library


Suggested change

# Link against the publically and privately linked libraries of the core library

# Link against the publicly and privately linked libraries of the core library

nit: spelling

scpeters · 2023-11-27T20:03:51Z

src/CMakeLists.txt

+    LIB_DEPS
+      library_for_tests
+    TEST_LIST
+      test_targets


nit: I don't think test_targets is used anywhere

oops. I saw this right after I merged. See #1347.

Move tests

284c8ad

Signed-off-by: Addisu Z. Taddese <[email protected]>

github-actions bot added the 🏯 fortress Ignition Fortress label Nov 1, 2023

azeey self-assigned this Nov 1, 2023

azeey force-pushed the fix_static branch 2 times, most recently from e4f081e to fc2090b Compare November 2, 2023 03:22

azeey force-pushed the fix_static branch from fc2090b to 617f579 Compare November 2, 2023 04:01

traversaro reviewed Nov 2, 2023

View reviewed changes

azeey force-pushed the fix_static branch from 617f579 to 87af702 Compare November 3, 2023 04:35

Fix static builds and optimize test compilation

cbe4600

Signed-off-by: Addisu Z. Taddese <[email protected]>

azeey force-pushed the fix_static branch from 87af702 to cbe4600 Compare November 3, 2023 05:55

azeey added 2 commits November 8, 2023 16:13

Use the STATIC_DEFINE flag

02dac75

Signed-off-by: Addisu Z. Taddese <[email protected]>

Define URDFDOM_STATIC only when using the internal URDF

5437306

Signed-off-by: Addisu Z. Taddese <[email protected]>

azeey marked this pull request as ready for review November 8, 2023 23:05

azeey requested a review from scpeters as a code owner November 8, 2023 23:05

azeey mentioned this pull request Nov 9, 2023

[vcpkg] install using_parser_urdf target #1309

Closed

Merge branch 'sdf12' into fix_static

639d070

mjcarroll approved these changes Nov 27, 2023

View reviewed changes

scpeters approved these changes Nov 27, 2023

View reviewed changes

azeey merged commit ffd7e0d into gazebosim:sdf12 Nov 27, 2023
6 checks passed

azeey mentioned this pull request Nov 27, 2023

Address review feedback from PR 1343 #1347

Merged

azeey deleted the fix_static branch November 27, 2023 20:10

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix static builds and optimize test compilation #1343

Fix static builds and optimize test compilation #1343

azeey commented Nov 1, 2023

codecov bot commented Nov 2, 2023 •

edited

Loading

traversaro Nov 2, 2023

azeey Nov 8, 2023

azeey commented Nov 9, 2023

scpeters commented Nov 14, 2023

j-rivero commented Nov 15, 2023

scpeters left a comment

scpeters Nov 27, 2023

scpeters Nov 27, 2023

azeey Nov 27, 2023

	# Link against the publically and privately linked libraries of the core library
	# Link against the publicly and privately linked libraries of the core library

Fix static builds and optimize test compilation #1343

Fix static builds and optimize test compilation #1343

Conversation

azeey commented Nov 1, 2023

🦟 Bug fix

Summary

Checklist

codecov bot commented Nov 2, 2023 • edited Loading

Codecov Report

traversaro Nov 2, 2023

Choose a reason for hiding this comment

azeey Nov 8, 2023

Choose a reason for hiding this comment

azeey commented Nov 9, 2023

scpeters commented Nov 14, 2023

j-rivero commented Nov 15, 2023

scpeters left a comment

Choose a reason for hiding this comment

scpeters Nov 27, 2023

Choose a reason for hiding this comment

scpeters Nov 27, 2023

Choose a reason for hiding this comment

azeey Nov 27, 2023

Choose a reason for hiding this comment

codecov bot commented Nov 2, 2023 •

edited

Loading