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

Add support for YAML-CPP 0.5+. #7

Merged
merged 1 commit into from
Apr 28, 2014
Merged

Add support for YAML-CPP 0.5+. #7

merged 1 commit into from
Apr 28, 2014

Conversation

cottsay
Copy link
Contributor

@cottsay cottsay commented Apr 25, 2014

The new yaml-cpp API removes the "node >> outputvar;" operator, and it has a new way of loading documents. There's no version hint in the library's headers, so I'm getting the version number from pkg-config.

bwi_planning also directly references yaml-cpp code in src/libbwi_planning/cost_learner.cpp but does not depend on it like it should, so that was added. I think it didn't cause a build failure because an upstream library had already linked against it.

The new yaml-cpp API removes the "node >> outputvar;" operator, and it
has a new way of loading documents. There's no version hint in the
library's headers, so I'm getting the version number from pkg-config.
@cottsay cottsay changed the title Added support for YAML-CPP 0.5+. Add support for YAML-CPP 0.5+. Apr 25, 2014
@jack-oquin
Copy link
Member

Thanks for both patches, we appreciate your contributions.

These are experimental packages being developed by the Building-Wide Intelligence project of the Learning Agents Research Group at the University of Texas at Austin.

You are, of course, welcome to use them, but I am curious why you are building them and on what platform. Are you building everything released to Hydro?

@cottsay
Copy link
Contributor Author

cottsay commented Apr 25, 2014

As these packages are officially released as part of Hydro, they are on my list to release into Fedora. This particular PR isn't blocking anything outside of bwi_planning and bwi, but the PR against bwi_common is blocking a series of segbot packages as well. I'd hate to seen them, and all of their downstream excluded from Fedora packaging when there is a fix.

This bug will block Arch Linux as well, and will block Ubuntu Trusty.

With these patches present, your packages would join the other 96% of Hydro packages that are successfully packaged on Fedora: http://csc.mcs.sdsmt.edu/ros/rpmbuild/hydro.html

Please let me know if you have any other questions or concerns.

Thanks!

@jack-oquin
Copy link
Member

Just curious. I figured you were doing something like that. It's certainly a worthwhile activity.

I presume you are pulling from the release tags, so you probably would like a new release with these fixes some time soon?

@cottsay
Copy link
Contributor Author

cottsay commented Apr 25, 2014

The plan going forward is that the rosrpm generation will be turned on, and that developers will begin generating RPM spec files along with the debian infrastructure when they use Bloom. Until then, I will keep forking the release repos for testing.

Long story short, I've already patched my fork of the release repos for these packages, and that will be fine for now. My activities don't require an imminent release, but @bchretien may want one for Arch (not sure if you plan on packaging these).

As long as it is in your next release (whenever that may be), I'm happy.

Thanks for working with me on this :)

@bchretien
Copy link

@cottsay I usually release what I need or what people request, and if I'm bored, also what feels straightforward with our automatic tools for Arch. If this is fixed for Fedora in the official release, it should also go smoothly on Arch.

@piyushk
Copy link
Member

piyushk commented Apr 25, 2014

Thanks for the patch! I'll test the code today or tomorrow and merge it in.

@jack-oquin
Copy link
Member

@piyushk: please merge and test utexas-bwi/bwi_common#3 at the same time


if(NOT ${YAML_CPP_VERSION} VERSION_LESS "0.5")
add_definitions(-DHAVE_NEW_YAMLCPP)
endif(NOT ${YAML_CPP_VERSION} VERSION_LESS "0.5")
Copy link
Member

Choose a reason for hiding this comment

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

@cottsay Minor note. catkin_lint produces the following warning for endif arguments, since they aren't necessary.

CMakeLists.txt(17): error: extra arguments in endif()

Don't worry about modifying this PR though, I'm resolving other catkin_lint issues as well and will fix it.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Interesting! So they want it to simply be:

endif()

or what?

Copy link
Member

Choose a reason for hiding this comment

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

Yes. I only realized that endif() works when we started running catkin_lint on our packages.

@piyushk
Copy link
Member

piyushk commented Apr 28, 2014

This builds correctly, however I'm running into issues testing the modified binary for unrelated reasons. However since I don't expect any problems, I'm going to merge this one, and test later.

piyushk added a commit that referenced this pull request Apr 28, 2014
Add support for YAML-CPP 0.5+.
@piyushk piyushk merged commit 5e236d5 into utexas-bwi:master Apr 28, 2014
@piyushk piyushk mentioned this pull request Apr 28, 2014
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.

None yet

4 participants