Add drake::filesystem via new ghc::filesystem dependency #12106
Conversation
|
@drake-jenkins-bot mac-mojave-clang-bazel-experimental-everything-release please |
jwnimmer-tri
force-pushed
the
filesystem
branch
from
56db94e to
5f4b4a8
Compare
|
@drake-jenkins-bot mac-mojave-clang-bazel-experimental-release please => https://drake-jenkins.csail.mit.edu/job/mac-mojave-clang-bazel-experimental-release/20/ |
|
Low priority. +@jamiesnape and @EricCousineau-TRI for feature review, please. |
There was a problem hiding this comment.
Reviewed 8 of 13 files at r1, 5 of 5 files at r2.
Reviewable status: 1 unresolved discussion, LGTM missing from assignees jamiesnape,sherm1(platform), labeled "do not merge" (waiting on @jamiesnape, @jwnimmer-tri, and @sherm1)
common/filesystem.h, line 16 at r2 (raw file):
#include <filesystem> namespace drake { namespace filesystem = std::filesystem; } #else
nit It's hard to parse this... For readability, could there be some extra newlines sprinkled after / before the #if .. #else #endif?
common/filesystem.h, line 32 at r2 (raw file):
/** Returns true iff the given path is a file. */ DRAKE_DEPRECATED("2019-11-01", "Use drake::filesystem directly.")
BTW I'd be OK if we didn't deprecate this, and just fix forward on Anzu...
There was a problem hiding this comment.
Reviewable status: 1 unresolved discussion, LGTM missing from assignees jamiesnape,sherm1(platform), labeled "do not merge" (waiting on @jwnimmer-tri and @sherm1)
common/filesystem.h, line 32 at r2 (raw file):
Previously, EricCousineau-TRI (Eric Cousineau) wrote…
BTW I'd be OK if we didn't deprecate this, and just fix forward on Anzu...
Anzu is not the only consumer of Drake (I hope).
There was a problem hiding this comment.
Reviewable status: 1 unresolved discussion, LGTM missing from assignees jamiesnape,sherm1(platform), labeled "do not merge" (waiting on @jwnimmer-tri and @sherm1)
common/filesystem.h, line 32 at r2 (raw file):
Previously, jamiesnape (Jamie Snape) wrote…
Anzu is not the only consumer of Drake (I hope).
The key point here is that these methods are in internal namespace, which means they are not for downstream use and not subject to deprecation conventions. However, adding the deprecation was so easy that I opted to do it anyway.
jwnimmer-tri
force-pushed
the
filesystem
branch
from
5f4b4a8 to
c02c902
Compare
There was a problem hiding this comment.
Reviewable status: LGTM missing from assignees jamiesnape,sherm1(platform), labeled "do not merge" (waiting on @EricCousineau-TRI and @sherm1)
common/filesystem.h, line 32 at r2 (raw file):
Previously, jwnimmer-tri (Jeremy Nimmer) wrote…
The key point here is that these methods are in
internalnamespace, which means they are not for downstream use and not subject to deprecation conventions. However, adding the deprecation was so easy that I opted to do it anyway.
OK. I think we should just remove the cruft then.
common/filesystem.cc, line 6 at r2 (raw file):
// Keep this sequence in sync with drake/common/filesystem.h. #if __has_include(<filesystem>) && !defined(__APPLE__)
BTW Depending on how through you want to be (now), we can narrow this down to macOS version (I can give you the code).
jwnimmer-tri
force-pushed
the
filesystem
branch
from
c02c902 to
118bab5
Compare
There was a problem hiding this comment.
Reviewable status: LGTM missing from assignees jamiesnape,sherm1(platform), labeled "do not merge" (waiting on @EricCousineau-TRI and @sherm1)
common/filesystem.h, line 32 at r2 (raw file):
Previously, jamiesnape (Jamie Snape) wrote…
OK. I think we should just remove the cruft then.
Done.
common/filesystem.cc, line 6 at r2 (raw file):
Previously, jamiesnape (Jamie Snape) wrote…
BTW Depending on how through you want to be (now), we can narrow this down to macOS version (I can give you the code).
My plan is to wait until we have CI in place to start to tune that.
There was a problem hiding this comment.
Reviewed 7 of 13 files at r1, 2 of 5 files at r2, 4 of 4 files at r3.
Reviewable status: LGTM missing from assignee sherm1(platform), labeled "do not merge" (waiting on @sherm1)
There was a problem hiding this comment.
Platform
Reviewed 7 of 13 files at r1, 2 of 5 files at r2, 4 of 4 files at r3.
Reviewable status: labeled "do not merge"
|
@drake-jenkins-bot mac-mojave-clang-bazel-experimental-release please |
Relates #12104.
This change is