Skip to content

create libyaml_vendor#1

Merged
mikaelarguedas merged 5 commits intomasterfrom
create_libyaml_vendor
May 29, 2018
Merged

create libyaml_vendor#1
mikaelarguedas merged 5 commits intomasterfrom
create_libyaml_vendor

Conversation

@mikaelarguedas
Copy link
Copy Markdown
Member

@mikaelarguedas mikaelarguedas commented May 11, 2018

This adds a vendor package that clones and build the libyaml 1.8.0rc.
This is the first version of libyaml providing a CMake module but the release is not tagged/released yet

Required by ros2/ros2#491 and ros2/rcl#235

Connects to ros2/rcl#235

@mikaelarguedas mikaelarguedas added ready Work is about to start (Kanban column) in progress Actively being worked on (Kanban column) in review Waiting for review (Kanban column) and removed ready Work is about to start (Kanban column) in progress Actively being worked on (Kanban column) labels May 11, 2018
@mikaelarguedas mikaelarguedas self-assigned this May 11, 2018
@mikaelarguedas mikaelarguedas added in progress Actively being worked on (Kanban column) and removed in review Waiting for review (Kanban column) labels May 11, 2018
@mikaelarguedas mikaelarguedas added in review Waiting for review (Kanban column) and removed in progress Actively being worked on (Kanban column) labels May 11, 2018
@mikaelarguedas
Copy link
Copy Markdown
Member Author

  • Linux Build Status
  • Linux-aarch64 Build Status
  • macOS Build Status
  • Windows Build Status

@mikaelarguedas
Copy link
Copy Markdown
Member Author

This cannot be merged as is as libyaml_vendor doesnt generate/install yaml.lib ATM: ros2/rcl#235 (comment)

@mikaelarguedas mikaelarguedas added in progress Actively being worked on (Kanban column) and removed in review Waiting for review (Kanban column) labels May 15, 2018
@mikaelarguedas mikaelarguedas force-pushed the create_libyaml_vendor branch from 621f4a9 to b633b0c Compare May 15, 2018 22:27
@sloretz
Copy link
Copy Markdown
Contributor

sloretz commented May 24, 2018

I wasn't able to locally reproduce the issue of yaml.lib not getting installed on windows using --merge-install. Running windows CI to see if there is a difference Build Status

Repos file uses this branch + rcl branch yaml_param_parser_shane_review_2

@mikaelarguedas
Copy link
Copy Markdown
Member Author

I wasn't able to locally reproduce the issue of yaml.lib not getting installed on windows using --merge-install.

This issue has been fixed by fiddling with the way we pass cmake args to the external project.

The remaining issue is the that Fast-RTPS fails to build if libyaml_vendor build started before Fast-RTPS's (happens only on windows, in a merged install space).

I ran another job with your repos file to reproduce the issue Build Status

@sloretz
Copy link
Copy Markdown
Contributor

sloretz commented May 25, 2018

@mikaelarguedas re-running the job from your comment with b3d4bf7. Build Status

libyaml creates and installs a file config.h. Fast-RTPS generates a file of the same name that defines HAVE_SECURITY. When doing a --merge-install the compiler found the one in the install space before the build space, which meant HAVE_SECURITY wasn't defined when building SecurityManager.cpp

@mikaelarguedas
Copy link
Copy Markdown
Member Author

mikaelarguedas commented May 25, 2018

Thanks @sloretz for looking into it.

the compiler found the one in the install space before the build space

This looks like an include path order issue to me.

It's a bit odd though that Fast-RTPS relies on it's own generated config.h file at build/link time as I would expect this file to be made for and used by downstream packages exclusively.

As we rely on the Fast-RTPS config file to be available in the install space when building other packages such as rmw_fastrtps_cpp this seems to bee the right temporary fix to me 👍.

Though we should report it to eProsima as this seems more of a hack that a proper solution. The install location do not collide as the files are installed to different subdirectories. Do you agree?

@sloretz
Copy link
Copy Markdown
Contributor

sloretz commented May 25, 2018

This looks like an include path order issue to me

Sort of. I don't think we have control of when system headers are searched. As far as I can tell this can't be specified in cmake (ex: there are no calls to target_include_directories(tgt /usr/include)). I opened an issue to use the project name in the build space include path eProsima/Fast-DDS#221

Though we should report it to eProsima as this seems more of a hack that a proper solution

I'm happy with this fix long term. libyaml installing a file as generic as <config.h> seems like the real issue to me.

The install location do not collide as the files are installed to different subdirectories. Do you agree?

Correct there is no collision. Fast-RTPS installs <fastrtps/config.h>, where libyaml installs <config.h>

@mikaelarguedas mikaelarguedas added in review Waiting for review (Kanban column) and removed in progress Actively being worked on (Kanban column) labels May 29, 2018
@mikaelarguedas mikaelarguedas merged commit aeead7a into master May 29, 2018
@mikaelarguedas mikaelarguedas deleted the create_libyaml_vendor branch May 29, 2018 16:06
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

in review Waiting for review (Kanban column)

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants