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

Integrate OSRF's sdformat library into drake #6028

Merged
merged 23 commits into from
May 17, 2017

Conversation

clalancette
Copy link
Contributor

@clalancette clalancette commented May 4, 2017

This is a pull request for adding in OSRF's sdformat library for parsing SDF, rather than using the hand-built library. This PR imports sdformat as another bitbucket repository, integrates it into the bazel build system, and adds a simple proof-of-life unit-test.

This PR is a rehash of #4831, but I decided to open a new one to make it a bit easier to review. All comments from the previous version have been taken into account here.


This change is Reviewable

@amcastro-tri
Copy link
Contributor

Review status: 0 of 9 files reviewed at latest revision, 3 unresolved discussions, some commit checks failed.


drake/multibody/parsers/test/sdformat_test.cc, line 8 at r1 (raw file):

namespace drake {
  namespace multibody {

BTW, no indentation according to style guide.


drake/multibody/parsers/test/sdformat_test.cc, line 14 at r1 (raw file):

          GTEST_TEST(SDFormatTest, TestBasic) {
            const std::string sdf_str("<?xml version='1.0'?><sdf version='1.6'><model name='my_model'><link name='link'/></model></sdf>"); // NOLINT

BTW, what about formatting like so:

"<?xml version='1.0'?>"
"<sdf version='1.6'>"
  "<model name='my_model'>"
    "<link name='link'/>"
  "</model>"
"</sdf>"

drake/multibody/parsers/test/sdformat_test.cc, line 25 at r1 (raw file):

            EXPECT_EQ(model_name, std::string("my_model"));

            std::string link_name = model->GetElement("link")->Get<std::string>("name"); // NOLINT

BTW, why not to break the line?


Comments from Reviewable

@jamiesnape
Copy link
Contributor

Following on from the discussion elsewhere, adding CMake support would be less than a 30 minute followup task, basically all boilerplate.


Review status: 0 of 9 files reviewed at latest revision, 5 unresolved discussions, some commit checks failed.


tools/ignition_math.BUILD, line 126 at r1 (raw file):

# Generates the library exported to users.
cc_library(
    name = "lib",

BTW I would keep the library named ignition_math or something descriptive. We end up with lots of artifacts named liblib.a that are not ideal. Not a blocker, though,


tools/sdformat.BUILD, line 117 at r1 (raw file):

    includes = ["include", "src/urdf"],
    visibility = ["//visibility:public"],
    linkopts = ["-lboost_system", "-lboost_filesystem", "-ltinyxml"],

BTW Would it be better to use something like https://github.com/nelhage/rules_boost ?


Comments from Reviewable

@jamiesnape
Copy link
Contributor

Review status: 0 of 9 files reviewed at latest revision, 6 unresolved discussions, some commit checks failed.


setup/ubuntu/16.04/install_prereqs.sh, line 95 at r1 (raw file):

libqt4-opengl-dev
libqwt-dev
libtinyxml-dev

Add brew install boost tinyxml to the Mac instructions.


Comments from Reviewable

@jamiesnape
Copy link
Contributor

Review status: 0 of 9 files reviewed at latest revision, 7 unresolved discussions, some commit checks failed.


setup/ubuntu/16.04/install_prereqs.sh, line 116 at r1 (raw file):

python-vtk
python-yaml
ruby1.9

Why?


Comments from Reviewable

@jamiesnape
Copy link
Contributor

Review status: 0 of 9 files reviewed at latest revision, 7 unresolved discussions, some commit checks failed.


tools/sdformat.BUILD, line 117 at r1 (raw file):

Previously, jamiesnape (Jamie Snape) wrote…

BTW Would it be better to use something like https://github.com/nelhage/rules_boost ?

BTW Also, personally, I would vendor tinyxml since it is so small.


Comments from Reviewable

@clalancette
Copy link
Contributor Author

I've just pushed some additional commits fixing up some of the problems pointed out in this review. We still have a couple of minor open questions, namely the question about the name of the library on disk, how we link boost, and whether or not we vendor tinyxml.


Review status: 0 of 10 files reviewed at latest revision, 7 unresolved discussions.


drake/multibody/parsers/test/sdformat_test.cc, line 8 at r1 (raw file):

Previously, amcastro-tri (Alejandro Castro) wrote…

BTW, no indentation according to style guide.

Ah, got it. Will change.


drake/multibody/parsers/test/sdformat_test.cc, line 14 at r1 (raw file):

Previously, amcastro-tri (Alejandro Castro) wrote…

BTW, what about formatting like so:

"<?xml version='1.0'?>"
"<sdf version='1.6'>"
  "<model name='my_model'>"
    "<link name='link'/>"
  "</model>"
"</sdf>"

Ah, good idea. I've done that now.


drake/multibody/parsers/test/sdformat_test.cc, line 25 at r1 (raw file):

Previously, amcastro-tri (Alejandro Castro) wrote…

BTW, why not to break the line?

Yeah, I'll do that.


setup/ubuntu/16.04/install_prereqs.sh, line 95 at r1 (raw file):

Previously, jamiesnape (Jamie Snape) wrote…

Add brew install boost tinyxml to the Mac instructions.

Will do.


setup/ubuntu/16.04/install_prereqs.sh, line 116 at r1 (raw file):

Previously, jamiesnape (Jamie Snape) wrote…

Why?

Actually, I don't think this is a requirement anymore. I'll remove it.


tools/ignition_math.BUILD, line 126 at r1 (raw file):

Previously, jamiesnape (Jamie Snape) wrote…

BTW I would keep the library named ignition_math or something descriptive. We end up with lots of artifacts named liblib.a that are not ideal. Not a blocker, though,

If I understand you properly, you are talking about the on-disk name of the library, not the bazel dependency name (the latter ends up being @ignition_math//:lib, which I think is pretty descriptive). The problem with naming this something else is that it ends up looking nicer on disk (libignition_math.a, for instance), but then it looks worse in bazel (@ignition_math//:ignition_math). That being said, I don't have a strong preference either way, so just let me know what is preferred.


tools/sdformat.BUILD, line 117 at r1 (raw file):

Previously, jamiesnape (Jamie Snape) wrote…

BTW Also, personally, I would vendor tinyxml since it is so small.

We could use the boost rules from there if we really wanted. Honestly, my goal is to remove boost from sdformat completely, so any solution we choose is temporary. Given that, I'd prefer to leave this "open-coded" linking stage, but let me know if you think differently.

tinyxml is an interesting case. It is already vendored in sdformat itself, but we generally only use that version when building on Windows. Since tinyxml is available as a system dependency in all flavors of Ubuntu and homebrew, I figure we may as well use that version if we can. However, if you feel differently, we can use the version built-in to sdformat. Let me know what you think.


Comments from Reviewable

@jamiesnape
Copy link
Contributor

Review status: 0 of 10 files reviewed at latest revision, 5 unresolved discussions, some commit checks failed.


tools/sdformat.BUILD, line 117 at r1 (raw file):

Previously, clalancette (Chris Lalancette) wrote…

We could use the boost rules from there if we really wanted. Honestly, my goal is to remove boost from sdformat completely, so any solution we choose is temporary. Given that, I'd prefer to leave this "open-coded" linking stage, but let me know if you think differently.

tinyxml is an interesting case. It is already vendored in sdformat itself, but we generally only use that version when building on Windows. Since tinyxml is available as a system dependency in all flavors of Ubuntu and homebrew, I figure we may as well use that version if we can. However, if you feel differently, we can use the version built-in to sdformat. Let me know what you think.

The idea is for Bazel builds is to be as hermetic as possible, so you would either use the vendored version in sdformat or add it to the WORKSPACE. Bringing in Boost at some point is probably inevitable given #3413, so it would not hurt to bring in rules_boost, but I would not block this PR for that.


Comments from Reviewable

@jwnimmer-tri
Copy link
Collaborator

tools/sdformat.BUILD, line 117 at r1 (raw file):

Previously, jamiesnape (Jamie Snape) wrote…

The idea is for Bazel builds is to be as hermetic as possible, so you would either use the vendored version in sdformat or add it to the WORKSPACE. Bringing in Boost at some point is probably inevitable given #3413, so it would not hurt to bring in rules_boost, but I would not block this PR for that.

For things that are stock Ubuntu and OSX and basically don't change (so tinyxml counts), I think it's fine to refer to them directly and not vendor them or add to WORKSPACE until there's an acute need. We already have a wide system-dependency footprint (as shown by .../16.04/setup.sh) so I think adding one more thing there for now is okay.


Comments from Reviewable

@jamiesnape
Copy link
Contributor

Review status: 0 of 10 files reviewed at latest revision, 5 unresolved discussions, some commit checks failed.


tools/ignition_math.BUILD, line 126 at r1 (raw file):

Previously, clalancette (Chris Lalancette) wrote…

If I understand you properly, you are talking about the on-disk name of the library, not the bazel dependency name (the latter ends up being @ignition_math//:lib, which I think is pretty descriptive). The problem with naming this something else is that it ends up looking nicer on disk (libignition_math.a, for instance), but then it looks worse in bazel (@ignition_math//:ignition_math). That being said, I don't have a strong preference either way, so just let me know what is preferred.

The other advantage with :ignition_math is that @ignition_math is shorthand for @ignition_math//:ignition_math, which I think is the best of both worlds.


Comments from Reviewable

@jwnimmer-tri
Copy link
Collaborator

tools/ignition_math.BUILD, line 126 at r1 (raw file):

Previously, jamiesnape (Jamie Snape) wrote…

The other advantage with :ignition_math is that @ignition_math is shorthand for @ignition_math//:ignition_math, which I think is the best of both worlds.

FYI I agree to avoid using :lib. I'd fostered that pattern to match how we were bringing pkg-config libraries into the WORKSPACE, but now I think it confuses more than it helps, and we should migrate all externals away from using :lib, and not use it in new code.


Comments from Reviewable

@clalancette
Copy link
Contributor Author

Review status: 0 of 9 files reviewed at latest revision, 5 unresolved discussions.


drake/multibody/parsers/test/sdformat_test.cc, line 8 at r1 (raw file):

Previously, clalancette (Chris Lalancette) wrote…

Ah, got it. Will change.

Done.


drake/multibody/parsers/test/sdformat_test.cc, line 14 at r1 (raw file):

Previously, clalancette (Chris Lalancette) wrote…

Ah, good idea. I've done that now.

Done.


drake/multibody/parsers/test/sdformat_test.cc, line 25 at r1 (raw file):

Previously, clalancette (Chris Lalancette) wrote…

Yeah, I'll do that.

Done.


tools/ignition_math.BUILD, line 126 at r1 (raw file):

Previously, jwnimmer-tri (Jeremy Nimmer) wrote…

FYI I agree to avoid using :lib. I'd fostered that pattern to match how we were bringing pkg-config libraries into the WORKSPACE, but now I think it confuses more than it helps, and we should migrate all externals away from using :lib, and not use it in new code.

Ah, I didn't know about the shortcut @ignition_math for @ignition_math://ignition_math . In that case, I agree that what you propose is nicer. I'll change the dependency to look like that, thanks.


tools/sdformat.BUILD, line 117 at r1 (raw file):

Previously, jwnimmer-tri (Jeremy Nimmer) wrote…

For things that are stock Ubuntu and OSX and basically don't change (so tinyxml counts), I think it's fine to refer to them directly and not vendor them or add to WORKSPACE until there's an acute need. We already have a wide system-dependency footprint (as shown by .../16.04/setup.sh) so I think adding one more thing there for now is okay.

I'm tempted to agree with Jeremy on this one, since there are other system dependencies for drake. So for now, I'm going to leave this as-is.


Comments from Reviewable

@jamiesnape
Copy link
Contributor

Review status: 0 of 9 files reviewed at latest revision, 5 unresolved discussions.


tools/sdformat.BUILD, line 117 at r1 (raw file):

Previously, clalancette (Chris Lalancette) wrote…

I'm tempted to agree with Jeremy on this one, since there are other system dependencies for drake. So for now, I'm going to leave this as-is.

In which case you will need to create a ticket to update the system dependencies on the pre-built CI images. Assign to @BetsyMcPhail.


Comments from Reviewable

@clalancette
Copy link
Contributor Author

Review status: 0 of 9 files reviewed at latest revision, 5 unresolved discussions.


tools/sdformat.BUILD, line 117 at r1 (raw file):

Previously, jamiesnape (Jamie Snape) wrote…

In which case you will need to create a ticket to update the system dependencies on the pre-built CI images. Assign to @BetsyMcPhail.

Done: #6058


Comments from Reviewable

@jamiesnape
Copy link
Contributor

Review status: 0 of 9 files reviewed at latest revision, 8 unresolved discussions.


drake/doc/mac.rst, line 23 at r2 (raw file):

    brew upgrade
    brew install autoconf automake bazel boost clang-format cmake doxygen gcc \
      glib graphviz gtk+ jpeg libpng libtool libyaml mpfr ninja numpy python \

We explicitly need glib?


drake/doc/ubuntu_trusty.rst, line 87 at r2 (raw file):

      autoconf automake bison doxygen freeglut3-dev git graphviz \
      libgtk2.0-dev libhtml-form-perl libjpeg-dev libmpfr-dev libpng-dev \
      libterm-readkey-perl libtinyxml-dev libtool libvtk5-dev libwww-perl make \

Add boost system dependencies?


setup/ubuntu/16.04/install_prereqs.sh, line 87 at r2 (raw file):

libboost-dev
libboost-filesystem-dev
libboost-iostreams-dev
libboost-program-options-dev
libboost-regex-dev
libboost-system-dev


Comments from Reviewable

@jamiesnape
Copy link
Contributor

Review status: 0 of 9 files reviewed at latest revision, 8 unresolved discussions, some commit checks failed.


setup/ubuntu/16.04/install_prereqs.sh, line 87 at r2 (raw file):

libboost-program-options-dev
libboost-regex-dev
libboost-system-dev

You definitely need all these boost libraries?


Comments from Reviewable

@clalancette
Copy link
Contributor Author

Review status: 0 of 9 files reviewed at latest revision, 8 unresolved discussions.


drake/doc/mac.rst, line 23 at r2 (raw file):

Previously, jamiesnape (Jamie Snape) wrote…

We explicitly need glib?

That one I don't know about, since it was already there. I only added boost and tinyxml to the brew list, but I ended up reformatting the whole thing so that it still fit in 80 columns.


drake/doc/ubuntu_trusty.rst, line 87 at r2 (raw file):

Previously, jamiesnape (Jamie Snape) wrote…

Add boost system dependencies?

Oh yeah, good point, I totally missed that. I'll add it, thanks.


setup/ubuntu/16.04/install_prereqs.sh, line 87 at r2 (raw file):

Previously, jamiesnape (Jamie Snape) wrote…

You definitely need all these boost libraries?

Actually, good point, no. I can reduce that set; I'll do so.


Comments from Reviewable

@jamiesnape
Copy link
Contributor

Review status: 0 of 9 files reviewed at latest revision, 8 unresolved discussions, some commit checks failed.


drake/doc/mac.rst, line 23 at r2 (raw file):

Previously, clalancette (Chris Lalancette) wrote…

That one I don't know about, since it was already there. I only added boost and tinyxml to the brew list, but I ended up reformatting the whole thing so that it still fit in 80 columns.

I see, I missed the reformatting. We probably need it for something like LCM.


Comments from Reviewable

@jamiesnape
Copy link
Contributor

@drake-jenkins-bot linux-xenial-unprovisioned-gcc-bazel-experimental please
@drake-jenkins-bot linux-xenial-unprovisioned-clang-bazel-experimental please

@jamiesnape
Copy link
Contributor

@clalancette The CI images will be updated shortly, but until then you can use the two jobs that I requested in the previous comment to test this PR. Looks like you have build failures at present.

@stonier
Copy link
Contributor

stonier commented May 9, 2017

drake/doc/ubuntu_trusty.rst, line 87 at r2 (raw file):

Previously, clalancette (Chris Lalancette) wrote…

Oh yeah, good point, I totally missed that. I'll add it, thanks.

I thought boost filesystem was removed in https://bitbucket.org/osrf/sdformat/pull-requests/335/remove-boost-11?


Comments from Reviewable

@BetsyMcPhail
Copy link
Contributor

The Trusty, Xenial and Mac CI images have been updated.

@clalancette
Copy link
Contributor Author

Review status: 0 of 9 files reviewed at latest revision, 4 unresolved discussions, some commit checks failed.


drake/doc/ubuntu_trusty.rst, line 87 at r2 (raw file):

Previously, stonier (Daniel Stonier) wrote…

I thought boost filesystem was removed in https://bitbucket.org/osrf/sdformat/pull-requests/335/remove-boost-11?

Yeah, it was. I opened this PR before that was merged up, which is why it is still in there. I guess I'll go ahead and update this to use a newer commit from sdformat instead.


Comments from Reviewable

@clalancette
Copy link
Contributor Author

Is there a way I can modify those Jenkins build jobs to show me the gcc command-lines that are being used? The builds are failing in ways that I don't understand, and that I don't see anywhere else.

@jamiesnape
Copy link
Contributor

external/sdformat/src/urdf/urdf_parser/urdf_parser.h:47:24: fatal error: exportdecl.h: No such file or directory
#include "exportdecl.h"

@jamiesnape
Copy link
Contributor

(but to answer your direct question, no)

@clalancette
Copy link
Contributor Author

Yeah, I saw that. The weird thing is that exportdecl.h is in the same directory as the file that is #include'ing it, so it really shouldn't throw that error. I'm wondering if bazel is setting up the gcc command-line in a way where it can't find in-the-same-directory includes somehow.

I guess for now I can try pushing a couple of different things and letting the auto-builder pick it up.

Signed-off-by: Chris Lalancette <[email protected]>
Signed-off-by: Chris Lalancette <[email protected]>
Based on review, change the dependency names to their
proper names, instead of just "lib".  This makes it nicer
on disk (libsdformat.a instead of liblib.a), and we can
use the shorthand of @sdformat in the bazel rules, so it
is better all around.

Signed-off-by: Chris Lalancette <[email protected]>
And add them to the trusty instructions.

Signed-off-by: Chris Lalancette <[email protected]>
We can just reference CMakeLists.txt directly.

Signed-off-by: Chris Lalancette <[email protected]>
@clalancette
Copy link
Contributor Author

All right, I've rebased the whole thing and added the fix. Hopefully this will make the builds pass now :).

@jwnimmer-tri
Copy link
Collaborator

Reviewed 1 of 1 files at r11, 1 of 1 files at r12.
Review status: all files reviewed at latest revision, all discussions resolved.


Comments from Reviewable

@jamiesnape
Copy link
Contributor

Reviewed 1 of 2 files at r9, 1 of 1 files at r10, 1 of 1 files at r11, 1 of 1 files at r12.
Review status: all files reviewed at latest revision, all discussions resolved.


Comments from Reviewable

@jwnimmer-tri
Copy link
Collaborator

I think stonier has been encouraging us to do reviewer-merge instead of author-merge; I'll do that now.

@jwnimmer-tri jwnimmer-tri merged commit 7f2dee1 into RobotLocomotion:master May 17, 2017
@clalancette
Copy link
Contributor Author

Excellent, thanks to everyone for review and for the merge.

@stonier
Copy link
Contributor

stonier commented May 18, 2017

👍

@SeanCurtis-TRI
Copy link
Contributor

@drake-jenkins-bot linux-xenial-gcc-bazel-continuous-release please

@clalancette clalancette deleted the sdformat-again branch July 20, 2017 21:44
amcastro-tri pushed a commit to amcastro-tri/drake that referenced this pull request Oct 24, 2017
* Add in compilation of sdformat library.

Signed-off-by: Chris Lalancette <[email protected]>

* Fix a bug in cmake_configure_file.py

The greedy portion of the regular expression could make
it so that lines never saw a carriage return.  This, in turn,
made it so that certain defines were never seen after generation.
To avoid this, make the regex have a + on the \r\n at the end,
to ensure that we always *must* have at least one newline
character.

Signed-off-by: Chris Lalancette <[email protected]>

* Fix up indentation in sdformat_test.cc

Signed-off-by: Chris Lalancette <[email protected]>

* Fix up more indentation.

Signed-off-by: Chris Lalancette <[email protected]>

* Last indentation fix.

Signed-off-by: Chris Lalancette <[email protected]>

* Remove unnecessary URDF_GE_0P3 define.

This is no longer used or necessary in sdformat.

Signed-off-by: Chris Lalancette <[email protected]>

* NOLINT one more long line.

Signed-off-by: Chris Lalancette <[email protected]>

* Remove unnecessary ruby dependency.

Signed-off-by: Chris Lalancette <[email protected]>

* Re-indent the sdformat test to conform with the rules.

Signed-off-by: Chris Lalancette <[email protected]>

* Fix up a trailing whitespace.

Signed-off-by: Chris Lalancette <[email protected]>

* Change the dependency names from "lib" to the library name.

Based on review, change the dependency names to their
proper names, instead of just "lib".  This makes it nicer
on disk (libsdformat.a instead of liblib.a), and we can
use the shorthand of @sdformat in the bazel rules, so it
is better all around.

Signed-off-by: Chris Lalancette <[email protected]>

* Reduce the set of boost libraries.

And add them to the trusty instructions.

Signed-off-by: Chris Lalancette <[email protected]>

* Update sdformat to the next version without boost-filesystem.

Signed-off-by: Chris Lalancette <[email protected]>

* Remove linking to boost-filesystem in sdformat.

Signed-off-by: Chris Lalancette <[email protected]>

* Make sure to put exportdecl.h in the private headers.

Signed-off-by: Chris Lalancette <[email protected]>

* Respond to a bunch of review comments.

Signed-off-by: Chris Lalancette <[email protected]>

* Remove indirection through cmakelists_with_version.

We can just reference CMakeLists.txt directly.

Signed-off-by: Chris Lalancette <[email protected]>

* Move private headers into srcs instead of hdrs.

Signed-off-by: Chris Lalancette <[email protected]>

* Update all of the comments to be more correct.

Signed-off-by: Chris Lalancette <[email protected]>

* Fix up indentation in sdformat_test.cc

Signed-off-by: Chris Lalancette <[email protected]>

* Fix up some comments pointed out in review.

Signed-off-by: Chris Lalancette <[email protected]>

* Add a couple more TODO comments into sdformat.BUILD.

Signed-off-by: Chris Lalancette <[email protected]>

* Add SplinePrivate.cc to the ignition_math src file list.

Signed-off-by: Chris Lalancette <[email protected]>
@jamiesnape jamiesnape removed their assignment Jun 22, 2021
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.

8 participants