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

Link against frameTransformRC via target_link_libraries #2637

Closed
wants to merge 1 commit into from

Conversation

PeterBowman
Copy link
Member

On latest YARP master (unreleased 3.5), a CMake 3.12 configure run throws the following error (logs):

CMake Error at src/devices/frameTransformClient/CMakeLists.txt:44 (target_sources):
  Error evaluating generator expression:

    $<TARGET_OBJECTS:frameTransformRC>

  Objects of target "frameTransformRC" referenced but is not an OBJECT
  library.

The offending line is:

target_sources(yarp_frameTransformClient PRIVATE $<TARGET_OBJECTS:frameTransformRC>)

According to CMake docs, $<TARGET_OBJECTS:...> requires an OBJECT target. However, the target created by cmrc_add_resource_library a few lines earlier is a STATIC library (ref). For some reason, this generator expression does accept STATIC targets in recent CMake releases (tested at CMake 3.16.3), but it fails as expected for the minimum required CMake 3.12 version (tested at CMake 3.12.4).

I believe this instruction was somewhat inspired by the way "FrameTransformUtils" was referenced at line 36, which is in fact an OBJECT library. I think the simplest way to handle "frameTransformRC" is via target_link_libraries. That being said, this target does not register any interface include directory (regarding the now-deleted and superseded target_include_directories command).

Follows up #2624 (cc @elandini84). I'm also removing the NAMESPACE option to cmrc_add_resource_library since it defaults to the target name anyway ("frameTransformRC", ref, docs).

The frameTransformRC target is a `STATIC` library, hence it is not
possible to use `$<TARGET_OBJECTS:xxx>` (although it seems to work
for some reason in recent CMake releases, it does not in 3.12).
Also, remove redundant `NAMESPACE` option which defaults to the
first parameter to `cmrc_add_resource_library`.
@PeterBowman PeterBowman requested a review from drdanz as a code owner July 3, 2021 11:30
@update-docs
Copy link

update-docs bot commented Jul 3, 2021

Thanks for opening this pull request! The maintainers of this repository would appreciate it if you would update the release notes by adding a file in doc/release/<target_branch>, based on your changes.

@PeterBowman PeterBowman temporarily deployed to code-analysis July 3, 2021 11:32 Inactive
@PeterBowman PeterBowman temporarily deployed to code-analysis July 3, 2021 11:32 Inactive
@PeterBowman PeterBowman temporarily deployed to code-analysis July 3, 2021 11:32 Inactive
@PeterBowman
Copy link
Member Author

Static builds are failing with:

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

I wonder if this was the reason behind using $<TARGET_OBJETS:...>. See also https://gitlab.kitware.com/cmake/cmake/-/issues/17357.

@PeterBowman
Copy link
Member Author

Another approach I considered before applying the target_link_libraries solution was:

target_sources(yarp_frameTransformClient PRIVATE $<TARGET_PROPERTY:frameTransformRC,SOURCES>)
# as stated earlier, it should be safe to remove the `target_include_directories` line that follows

The SOURCES property comprises all XML files passed to cmrc_add_resource_library and an auto-generated lib_.cpp source file. The silly thing here is that frameTransformRC would be needlessly compiled and linked into a static library - we'd reuse its sources and compile them again for yarp_frameTransformClient.

@drdanz
Copy link
Member

drdanz commented Jul 5, 2021

Thanks @PeterBowman.
We made this change because of the failing STATIC builds, since we would have to export the extra library, which it does not make any sense to export... See also vector-of-bool/cmrc#28
If this fails on CMake 3.12, I think the best option is to ship a patched version of the CMakeRC module, which produces OBJECT libraries instead of STATIC... I opened a PR vector-of-bool/cmrc#30, I'll import it in YCM asap

@PeterBowman
Copy link
Member Author

PeterBowman commented Jul 5, 2021

Thank you, @drdanz. I'll re-run our CI after either #2638 or #2639 is merged.

@PeterBowman PeterBowman closed this Jul 5, 2021
@sonarqubecloud
Copy link

sonarqubecloud bot commented Jul 5, 2021

Kudos, SonarCloud Quality Gate passed!

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 0 Code Smells

No Coverage information No Coverage information
No Duplication information No Duplication information

@PeterBowman
Copy link
Member Author

I'll re-run our CI after either #2638 or #2639 is merged.

I can confirm YARP master builds fine now with CMake 3.12: CI workflow run. Apparently this issue didn't show up on your own CMake 3.12-specific builds since Eigen is not installed, hence no YARP_math library, which is in turn a dependency for the frame transform devices.

@PeterBowman PeterBowman deleted the cmrc-ft-client branch July 7, 2021 16:57
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants