search

RobotLocomotion / drake

Model-based design and verification for robotics.
https://drake.mit.edu
Other
3.25k stars 1.25k forks source link

Stop using Path.h.in #2174

Closed jwnimmer-tri closed 8 years ago

jwnimmer-tri commented 8 years ago

The drake/drake/core/Path.h.in implementation should be replaced, due to its many problems:

I am working on a PR with some fixes:

Work in progress will be in my getDrakePath-fixes branch.

jwnimmer-tri commented 8 years ago

This will depend on #2172 to get the common directory structure booted up.

liangfok commented 8 years ago

What does "non-POD" mean in this context?

jwnimmer-tri commented 8 years ago

Ah, yes; "POD" is confusing since we have pod-build as a concept. My use here is not related to pod-build.

POD in this case means "Plain Old Data", a C++ term of art. Globals that are not Plain Old Data are disallowed by googlestyle, see Static and Global Variables.

jwnimmer-tri commented 8 years ago

This may also block on #2182 to decide where libdrakeCommonTest should live within the source tree.

jwnimmer-tri commented 8 years ago

Both prereqs are done now, so I will be resuming work on this.

patmarion commented 8 years ago

I made a comment here about APIs for getting run time paths (without hard coded directories):

https://github.com/RobotLocomotion/drake/issues/1295#issuecomment-136156776

Drake has a lot of resource files, so it seems reasonable to me that Drake should have an API for getting paths to these resources. Why restrict that API to tests? Why not also use it in the examples and in user code?

jwnimmer-tri commented 8 years ago

Yup, I'd be in support of having a runtime API for "where to load my resources" from (and to teach it how to find the installed resource files -- so that it can be used in released, and not just sources builds).

However, the current getDrakePath is not that; it is the drake-distro/drake source tree path, for all and evermore. My goal here is to make it the code and docs more true to that goal, not to add a new capability.

I think my "GetDrakePath will only be available to tests" statement was misleading. I am porting the examples programs to link against libdrakeCommonTest, since I view them more as in-tree acceptance tests, rather than programs a user wants in their $bindir.

patmarion commented 8 years ago

I think examples may serve as starting points for user code. So if GetDrakePath() is used in examples, then we should make it available to user code.

Note, the swig python bindings use getDrakePath() (paging @rdeits)

https://github.com/RobotLocomotion/drake/blob/master/drake/bindings/python/pydrake/__init__.py

That functionality is used by drake-visualizer to locate Drake resources. If the functionality is not to be supported by drake outside of tests and examples, then downstream projects like drake-visualizer will have to re-implement it.

jwnimmer-tri commented 8 years ago

I guess the question is this: do we want projects using Drake as an external to have tendrils back into the Drake source tree, or not? Long term, it seems like a bad idea to me. I could live with having Drake provide a hook into its installed path, but not its source path.

I guess my main purpose is to highlight when code is reaching back into Drake. I could live with putting this function into libdrakeSourcePath instead of libdrakeCommonTest. To be clear, I do not plan on removing this current "feature" in any case. I don't even know how to make libdrakeCommonTest be used in-tree but not published out-of-tree.

patmarion commented 8 years ago

Yes, and that is one of the issues that I brought up in my old comment:

https://github.com/RobotLocomotion/drake/issues/1295#issuecomment-136156776

Currently, drake resource files (urdfs, meshes, config files) are embedded in the source and not installed. At this point in time, the drake source is part of the distribution. In the future, perhaps drake will have a more standard install layout, using bin/ lib/ and share/ locations, for example. The point of GetDrakePath() is to allow programs to find standard drake resource files without worrying about the details of the filesystem layout.

jwnimmer-tri commented 8 years ago

@patmarion If I keep pydrake.getDrakePath() working, is that good enough for now? Or would it be also helpful to keep a drake/Path.h shim installed as a transition aid?

(It looks like util/getDrakePath.m would be unaffected, since it doesn't call the C++ version.)

jwnimmer-tri commented 8 years ago

Oh, actually, pydrake.getDrakePath() also doesn't call into C++. So I guess the question is, how much are we tied to the current C++ API? If I rename it and give it linkage (instead of inline-header), how troublesome is that?

patmarion commented 8 years ago

That's fine for now. I don't know of any downstream projects using the C++ getDrakePath() function. There's lots of use of the matlab function getDrakePath(), and drake-visualizer uses pydrake.getDrakePath().

I still would say that I don't quite understand why we should discourage the use of c++ GetDrakePath() while still making it available. I get that you want to discourage reliance on the full source directory. I'll reference my old comment one last time, where I suggested the API be focused around getting the resource files (mostly located in the examples dir). Perhaps the problem is the contract that GetDrakePath() returns the source directory, when most people really just need the functionality of something like GetDrakeExamplesPath() in order to locate resources.

jwnimmer-tri commented 8 years ago

Currently, drake resource files (urdfs, meshes, config files) are embedded in the source and not installed. At this point in time, the drake source is part of the distribution.

Actually, I find this quite convincing. Right now, the only way to use Drake is headers + libraries + associated source tree. My intent here was a small PR, so I shouldn't try to upend the cart by moving away from that.

I think I will de-scope this to just moving to a generated *.cc file that obeys the style guide, and leave any more principled fixes to the future.