Expose directory contents iteration

[emersonknapp]

It would be nice to be able to iterate over the contents of rcpputils::fs::path (to use https://en.cppreference.com/w/cpp/filesystem/directory_iterator) - it could wrap the rcutils function https://github.com/ros2/rcutils/blob/master/include/rcutils/filesystem.h#L274

[fujitatomoya]

@emersonknapp

quick question. according to REP 2000, from Galactic or later we are allowed to use C++17, if i am not mistaken. and https://github.com/ros2/rosbag2/pull/641 seems to be a new feature for Galactic. So why don't we just use C++17 instead of introducing the same API to rcpputils?

[emersonknapp]

That's a good point - we could. We just haven't updated the rosbag2 C++ target yet.

More generally, does this mean that the rcpputils::fs namespace should be deprecated in Galactic in favor of using std::filesystem?

[Karsten1987]

I am not sure we have documented that somewhere more central, but the change to c++17 and specifically the change to std::filesystem entails a bit more than just bumping up the c++ standard. std::filesystem is not supported on OSX version lower than Catalina (10.15.x) and I believe the current OSX version targeted for Galactic is still Mojave. That means by replacing rcpputils::fs with std::filesytem we'll essentially break the OSX builds. IMO it would be great if we could at least keep the OSX builds to compile.

/cc @clalancette

[clalancette]

There's a couple of different things that would have to happen to actually move to C++17.

First, some of our code elicits new warnings from C++17 on MSVC. I've been (slowly) fixing those, but they aren't all gone yet. You can see a recent-ish build on Windows with C++17 turned on (just in geometry2) here: https://ci.ros2.org/job/ci_windows/13669/

Second, @Karsten1987 is absolutely right in that on Mojave, you cannot use std::filesystem. And while macOS is not going to be Tier 1 for Galactic, I agree that we should at least keep it compiling. I've been playing around with Catalina, and we can indeed get everything compiling there (with some warnings).

In my opinion, we have three paths in front of us:

1. Deprecate rcpputils::fs for Galactic. To do that, we'll have to:
   1. Fix the warnings from C++17 support on Windows
   2. Switch to macOS Catalina or Big Sur. This involves fixing the warnings and upgrading/adding some machines to CI.
   3. Deprecate rcpputils::fs, and start using std::filesystem
2. Wrap rcutils_dir_iter_* functions in rcpputils::fs.
3. Do nothing. That is, don't wrap the rcutils_dir_iter_* functions at all.

Given everything else that is going on for Galactic, I'm skeptical that we have time to do all of the steps for the first one. That being said, I think it is worthwhile to work towards it; it would be really nice to allow C++17 on leaf packages for Galactic.

For this particular issue, my suggestion would be to do nothing for Galactic. That is, the functionality is available through the rcutils API, so users can use that. Once we have fixed all of the issues surrounding C++17 (probably after Galactic), we can then revisit the decision.

[fujitatomoya]

@clalancette

For this particular issue, my suggestion would be to do nothing for Galactic. That is, the functionality is available through the rcutils API, so users can use that. Once we have fixed all of the issues surrounding C++17 (probably after Galactic), we can then revisit the decision.

yeah, sounds reasonable to me 😃 since this is maintenance we can revisit there later, in the mean time we can just go ahead to support rcpputils.

thanks for the info ❗ @emersonknapp @Karsten1987

[emersonknapp]

@clalancette how do you think rcpputils::fs deprecation in favor of std::filesystem looks in a post-Humble world? I notice REP 2000 doesn't have an explicit OSX version for support - but I suspect that at this point we do have C++17 support there.

[clalancette]

@clalancette how do you think rcpputils::fs deprecation in favor of std::filesystem looks in a post-Humble world? I notice REP 2000 doesn't have an explicit OSX version for support - but I suspect that at this point we do have C++17 support there.

Newer versions of macOS support C++17 fine; it was only our last fully supported version (Mojave) that had a problem. So I think going ahead with the deprecation would be fine.

[fujitatomoya]

i think we move away from rcpputils::fs, https://github.com/ros2/rcpputils/issues/164 closes this issue.