Ambiguous error in rmf_fleet_adapter::agv::parse_graph, no file found when given nav_graph

[JWCS]

rmf_ros2/rmf_fleet_adapter/src/rmf_fleet_adapter/agv/parse_graph.cpp

Line 30 in 3c17991

const YAML::Node graph_config = YAML::LoadFile(graph_file);

Passing in a valid path, of the form ~/path/to/nav_graph/0.yaml to parse_graph resulted in a raised exception from yaml-cpp that no file was found.
This was tested on self-compiled main, ubuntu 20, ros foxy, using the full_control fleet adapter launch file.
The solution/realization for me was that yaml-cpp was not performing tilde expansion, so a form of /home/myself/path/to/nav_graph/0.yaml was sufficient.

The main problem is the cryptic-ness of not accepting a shell-valid filepath, when it may get passed in via the shell cmdline args.
I've only tested/suffered the tilde expansion, but symbolic links and relative paths are also common cmdline path inputs.

Maybe this could be short-term fixed with either a comment by the line, though a cleaner long-term interface would resolve the filepath before passing to yaml-cpp, &/or catching the raised no-file-found to give a more specific error msg.

On a deeper note, this seems symptomatic of relying on yaml-cpp to do input (file-format) sanitization, but not doing input (path) sanitization to yaml-cpp. If there's any other deep-ish files that take raw user input, identifying them and listing them somewhere so that they can be checked for appropriate input sanitization would be nice project enhancement.

[mxgrey]

This is a vexing behavior and I certainly sympathize with your frustration, but I'm not totally convinced that having RMF internally interpret a leading ~ as the home directory is the right move. I say that because the use of a leading ~ to mean a home directory is a platform-specific behavior. Once the path information is moving around inside of RMF, it's simply a string, and it's not obvious to me that RMF should take on the role of interpreting that string in special ways. It would technically be valid (albeit pathological) for a user to create a directory with the name ~ and use it in a relative path name.

In general if I need to refer to the home directory in a path name that I'm passing into an application, I'll use the $HOME environment variable. Inside of a launch file you can use environment variables in launch arguments like this:

<arg name="my_file_path" value="$(env HOME)/my/directory" />

So my current stance on this is "won't fix" but I'll leave this issue open for now in case people think my stance is unreasonable and want to insist that the behavior is changed.

[JWCS]

Oh, I agree, I don't think you should add any hacky parsing to rmf for paths.
I think it'd be great if one of the three happened:

1. A comment by that line explaining that it only takes resolved absolute paths, documenting this, so people don't have to go digging into yaml-cpp to know why it's failing.
2. Try/catch that line; since the two exceptions are part of the api of the function, catching them and re-throwing them with the path/string in question acknowledges this behavior as intentional and not a bug.

/**
 * Loads the input file as a single YAML document.
 *
 * @throws {@link ParserException} if it is malformed.
 * @throws {@link BadFile} if the file cannot be loaded.
 */
YAML_CPP_API Node LoadFile(const std::string& filename);

3. Use some library/shell to resolve path names. But I think it's easier just to fail fast and resolve it outside, since C++ doesn't have any good cross platform libraries for more than relative path expansion. Maybe most ideally as an arg $(eval ...) in the launch file, for python's cross-platform resolution, similar to what you suggest, but the ros2 eval is buggy.

I'd be willing to make a PR, if there's a way you prefer.

[mxgrey]

I definitely like option (2). It's always good to be loud and clear about failure modes. Option (1) would be good to do in addition to (2).

If you're willing to open a PR for that I'll be more than happy to give it a quick review and merge.