Drake: SDF_SHARE_PATH and SDF_VERSION_PATH are incorrect in installed SDFormat

[jamiesnape]

In sdf_config.h

#define SDF_SHARE_PATH "external/sdformat/sdf/1.6/"
#define SDF_VERSION_PATH "external/sdformat/sdf/1.6/sdformat/"

Do we actually need to install the SDFormat library and its data files? SDFormat headers are not used by any Drake headers.

(In other words, do we need to fix the header or can we just make this a static library with hidden visibility?)

[jamiesnape]

FYI @clalancette @stonier.

[clalancette]

@jamiesnape I'll need to look at the details again, but I believe that we have to install the data files. The data files are used to do internal processing as well as be available for external processing, so I think certain functions of sdformat won't really work at all without the data files being installed.

I think we can get away with not installing the library or the headers, though.

[jamiesnape]

@jamiesnape I'll need to look at the details again, but I believe that we have to install the data files. The data files are used to do internal processing as well as be available for external processing, so I think certain functions of sdformat won't really work at all without the data files being installed.

So one other question is, are those features currently broken when SDFormat functionality incorporated into Drake is used from the install tree (since the data files are not where the definitions suggest they are)?

[jamiesnape]

@clalancette If it turns out the library is not needed, would it be possible to amend

https://bitbucket.org/osrf/sdformat/src/450c4aa37413bb5ecbd51985d8b56c215ef6ad83/include/sdf/system_util.hh?at=default&fileviewer=file-view-default#system_util.hh-44:51

to add support for defining something (e.g., SDFORMAT_STATIC_DEFINE) that forces both SDFORMAT_VISIBLE and SDFORMAT_HIDDEN to be empty (or better still use CMake GenerateExportHeader)? Should be a pretty trivial change.

[jamiesnape]

Potentially https://github.com/RobotLocomotion/drake/blob/c84bceb37a9fa9b01f23413733446495bf843725/multibody/parsing/sdf_parser.cc#L245
and
https://github.com/RobotLocomotion/drake/blob/c84bceb37a9fa9b01f23413733446495bf843725/examples/double_pendulum/sdf_helpers.cc#L380
could be failing to find files looking at the function calls internally. Whether they do in practice, I haven't looked yet.

[clalancette]

OK, so I went back over the details inside of sdformat here. Now I can speak more intelligently about it :).

What happens is that during the open of some SDF XML in sdformat/src/parser.cc, it uses the filesystem-installed root.sdf to add a bunch of "global" properties to the eventually parsed SDF file that is passed in (there are other instances of something similar in sdformat, but I'll just talk about parser.cc here). In order to find that root.sdf file, sdformat goes rooting around in the filesystem at various locations, with roughly the following algorithm, bailing out after the first successful match:

1. Look through some run-time settable URIs (settable by the addURIPath method)
2. Look at the value hard-coded at build time (in the SDF_SHARE_PATH macro)
3. Look at the "versioned" value hard-coded at build time (essentially the above, but prepended by a version string)
4. Lookup in the SDF_PATH environment variable
5. Look in the local directory (essentialy .)
6. Call a custom search function (installable via setFindCallback)

There are several things to quibble about in that algorithm, and it is not my intention to talk about that here. Importantly for the drake build of sdformat, we currently rely on the hard-coded SDF_SHARE_PATH that is set at build-time to find the root.sdf file. This works for the tests because the path that we set is external/sdformat/sdf/1.6, which is where things get copied/symlinked to in the bazel directory hierarchy. For the installation, though, this won't work, because the path that we need is something different.

There are some escape hatches that we could potentially use for the installed version of Drake, generally centering around setting either the SDF_PATH environment variables, or calling addURIPath or setFindCallback in the code. I agree these are not ideal, but they could be used as a workaround for now.

Given the above, my proposal is two-fold:

1. We get #7905 in; even though the installation of the data files isn't sufficient, it is necessary.
2. I had proposed a while ago to embed the "default" data files into the sdformat library directly so it wouldn't be necessary to grub around in the filesystem to get at it. This could be done in a user-friendly way by first examining the user-specified paths, and then if all of those fail, look into the built-in versions of the files. While that proposal was well-received, I have not had time to implement it, so it is still on the back-burner for now. If this is important, I could see about freeing up some time to do this.

[jwnimmer-tri]

From the sidelines: thanks for the write up, its very clear and I completely agree.

With my Drake maintainer hat on, I do like the embed idea the best, but addURIPath seems plausible in the interm (as a std::call_once hook in the multibody/parsing/sdf_parser.cc) and I think compatible with Drake's install philosophy. I will defer to @jamiesnape on the final answer though.

[jamiesnape]

With my Drake maintainer hat on, I do like the embed idea the best, but addURIPath seems plausible in the interm (as a std::call_once hook in the multibody/parsing/sdf_parser.cc) and I think compatible with Drake's install philosophy. I will defer to @jamiesnape on the final answer though.

I concur. I imagine something like a variation of the find_resource.cc along with addURIPath or setFindCallback will tide us over. I would like to see the embedding sooner rather than later given it moves the testing and maintenance burden upstream, not to mention our testing of the install packages is somewhat sparse at present.

[jamiesnape]

7905 merged. Assigning the remaining fix across to +@clalancette.

[stonier]

@jamiesnape - @nkoenig has indicated he will look at moving on doing something about embedding the information in the upstream repository.

[jamiesnape]

Cool. Thanks.

[clalancette]

@nkoenig As promised last week, I ended up pushing the work I've done for embedding the SDF XML files into a branch here: https://bitbucket.org/osrf/sdformat/branch/embed-sdf . As-is, that branch probably does not compile, but the first commit on the branch shows the work I've done so far towards generating C++ files that contain the embedded XML. Next up there should be to plumb it into the rest of the system so that it can be used. Let me know if you have any questions about it.

[avalenzu]

Now that #8687 has landed, this is becoming more pressing because it prevents parsing of SDF files in downstream projects that use Drake.

[avalenzu]

Pinging @nkoenig and @clalancette

[jwnimmer-tri]

The embed-sdf branch seems stalled. In the meantime then, it seems like a Drake developer will have to itself implement one of the addURIPath or setFindCallback work-arounds.

[clalancette]

The work in upstream sdformat is progressing, but may take some time to finish. In the meantime, I think the fallback addURIPath or setFindCallback seems like a reasonable way to go, with a TODO comment to remove once the upstream embedded sdformat stuff lands.

[amcastro-tri]

Great seeing some activity here!. @clalancette, am I right to I assume you are the person who'd implement these work-arounds?

[nkoenig]

I'm taking over this task from @clalancette.

[nkoenig]

Here is a PR that embeds the SDF specification files in libsdformat. https://bitbucket.org/osrf/sdformat/pull-requests/434

The problem is the above PR generates a special header via CMake and a Ruby script, but Drake doesn't know how to build this. Any Bazel experts out there who can offer advice?

[jwnimmer-tri]

If nobody else volunteers, you can assign to me to take a look. I suspect I'd just re-implement the ruby script's logic python and/or skylark, rather than add a build-dependency on Ruby.

[nkoenig]

Thanks Jeremy. I'll update this issue when the SDF pull request has been merged.

[EricCousineau-TRI]

Just got to seeing this; from my perspective, it'd be best to intercept the current sdformat PR since it is still open, and just rewrite the script to Python there, as there seem to be other Python scripts used within sdformat (there are 4 Python scripts, and only 1 other Ruby script). I am willing to try and rewrite this script briefly, see if I can test it with sdformat, and post on the current PR with this fix.

I will start on this now, and report back in 30min or an hour.

[EricCousineau-TRI]

Rewrote it to Python: https://github.com/EricCousineau-TRI/sdformat/commit/4bad9c20555a7350e46f629e7a0fb5222a4260f1
Will post this as a follow-up in the PR.

EDIT: Done: https://bitbucket.org/osrf/sdformat/pull-requests/434/embed-sdf/diff#comment-65236228

[jwnimmer-tri]

@nkoenig What's up with the sdformat PR? This issue is biting us hard and I would like to either know the upstream PR is making forward progress, or else I will fire drill the Drake work-around.

[clalancette]

@jwnimmer-tri FYI Nate is on vacation this week; he should be back next week.

[EricCousineau-TRI]

Per convo here: https://github.com/RobotLocomotion/drake/issues/8761#issuecomment-396730159

@amcastro-tri:
A fix was merged into sdformat, #434.
I believe for this to work for us we need some python magic? @EricCousineau-TRI?

Will address this on Friday.

[jwnimmer-tri]

If the macOS support for ruby works our of the box, my current thinking is that we add the build-dep on ruby and call the upstream script. I will give that a spin in the next day or two.