Conversation
|
@jpardeiro #183 made a similar change on master which I think should cover this case as well as Clang on Linux, even though the Clang version on Eloquent's supported Linux platforms won't have this deprecation message yet. I think backporting that change would make more sense than creating this parallel one. Would you be willing to cherry pick that PR against Eloquent locally and test that it works for you? |
|
@nuclearsandwich it definitely makes more sense :). I will test it and go back to you as soon as I have the results. |
|
@nuclearsandwich I cherry picked the PR you mentioned and recompiled my eloquent distro workspace without any issue. Should I just decline this PR? Or rebase it with the integration of the changes from #183 ? |
|
@jpardeiro if you want to close this and open a PR with the cherry-picked commit as an Eloquent backport that would be very much appreciated. |
* Fix filesystem linking on clang9 Signed-off-by: Emerson Knapp <emerson.b.knapp@gmail.com> * Fix comment Signed-off-by: Emerson Knapp <emerson.b.knapp@gmail.com> * Fix extras also Signed-off-by: Emerson Knapp <emerson.b.knapp@gmail.com> * Fix my silly syntax error that i should have caught locally Signed-off-by: Emerson Knapp <emerson.b.knapp@gmail.com> * Need to set up the filesystem lib on new gcc also Signed-off-by: Emerson Knapp <emerson.b.knapp@gmail.com> * Suppress std::experimental::filesystem deprecation warning. Clang 9 and 10 have deprecated std::experimental::filesystem and will be removing it in Clang 11, recommending C++17's std::filesystem instead. However as ROS 2 currently targets C++14 we cannot guarantee availability on all compilers. Since the deprecation notice is shown even when using Clang 10 with C++14, suppressing it seems like a reasonable thing to do for the Foxy release cycle. Signed-off-by: Steven! Ragnarök <steven@nuclearsandwich.com> Co-authored-by: Steven! Ragnarök <steven@nuclearsandwich.com>
|
@nuclearsandwich I rebased the branch so now it is only the cherry-pick merge commit. If I should decline this and create a new PR, it is not a problem. |
|
@nuclearsandwich I just realised this PR has been opened during quite some time. Am I missing something? :) |
No I've just been starved for maintenance time. I've put this issue on the Eloquent patch release board and here's CI for it.
FYI @mjcarroll |
4 participants
Tested in Mac OS X 10.15.4