Skip to content

Windows contains older version of std filesystem (still experimental)#92

Closed
bfjelds wants to merge 2 commits intoros:ros2from
bfjelds:ros_ros2
Closed

Windows contains older version of std filesystem (still experimental)#92
bfjelds wants to merge 2 commits intoros:ros2from
bfjelds:ros_ros2

Conversation

@bfjelds
Copy link
Copy Markdown

@bfjelds bfjelds commented Dec 13, 2017

No description provided.

@mikaelarguedas
Copy link
Copy Markdown
Member

mikaelarguedas commented Dec 14, 2017

Thanks @bfjelds for the contribution.

I have 2 questions regarding filesystem on Windows:

  • The MSDN filesystem page shows a v1 namespace under the filesystem namespace: using namespace std::experimental::filesystem::v1;. Do you know if this is still necessary (we're targeting VS2015 update 3 and MSVC 19.0.24215.1 at the moment) ?
  • Is including <filesystem> necessary of is <experimental/filesystem> enough ? I see that we're including only the latter in other places of the codebase that we will need to update accordingly if Windows requires to include both.

I'll let @wjwwood review this as well as he's been the one implementing this filesystem helper.

@bfjelds
Copy link
Copy Markdown
Author

bfjelds commented Dec 14, 2017

These are great questions...I was focused on getting pluginlib to build 8n the context of implementing laser_filters, but was using VS2017.

I'll investigate further in about a week and see if I can't get some definitive answers :)

@bfjelds
Copy link
Copy Markdown
Author

bfjelds commented Dec 21, 2017

The v1 is an inline namespace (new to me!) ... if we wanted to refer specifically to v1, we could, but it isn't required.

<experimental/filesystem> is a wrapper around at this point, so either would work. I went with experimental/filesystem to be consistent with the namespace.

This was all verified on VS2015 Update 3 and VS2017.

mikaelarguedas
mikaelarguedas reviewed Dec 22, 2017
View reviewed changes
Copy link
Copy Markdown
Member

@mikaelarguedas mikaelarguedas left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks @bfjelds for checking this.

The code looks good to me, I have one comment regarding the MSC_VER check.

After looking a bit into it, it seems that we need this special case for MSVC only because VS2015 doesn't support __has_include whereas VS2017, gcc and clang implement the feature. I think it failed to build for you with VS2017 beccause both <filesystem> and <experimental/filesystem> are present, and as we check the presence of <filesystem> first you ended up with namespace fs = std::filesystem; while the Windows implementation is under the namespace std::experimental::filesystem.

include/pluginlib/impl/filesystem_helper.hpp
#define PLUGINLIB__IMPL__FILESYSTEM_HELPER_HPP_

#if defined(__has_include)
#if defined(_MSC_VER)
Copy link
Copy Markdown
Member

@mikaelarguedas mikaelarguedas Dec 22, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it would make sense to compare the value to a minimal version of compiler as some Windows platforms with an older compiler will have the variable defined but not the filesystem header. I didnt track down the first version of MSVC implementing it but as we target VS2015 Update3 we should be safe by requiring _MSC_VER >= 1900

@mikaelarguedas
Copy link
Copy Markdown
Member

mikaelarguedas commented Dec 23, 2017

I rebased this branch and added the change I suggested regarding the version check on this branch https://github.com/ros/pluginlib/tree/ros_ros2_mikael.
@bfjelds would you mind confirming that this will allow you to build your plugins ? If so I'll push the changes to this branch and merge it

@bfjelds
Copy link
Copy Markdown
Author

bfjelds commented Dec 23, 2017

Confirmed that it builds...looks great, thanks!!

@mikaelarguedas mikaelarguedas mentioned this pull request Jan 8, 2018
Merged
@mikaelarguedas
Copy link
Copy Markdown
Member

mikaelarguedas commented Jan 8, 2018 •
edited
Loading

Thanks @bfjelds for testing it!
I opened #96 with the suggested change on top of the current ros2 branch to avoid us to have to resolve the conflicts manually. Thanks again for reporting this and submitting a fix.

Note to self: this should be closed once #96 merged

@wjwwood
Copy link
Copy Markdown
Member

wjwwood commented Jan 8, 2018

@mikaelarguedas it would be nice to keep @bfjelds's commits as the foundation of the pr, to preserve his attribution, in #92 you can't tell that he had a hand in it from the commits. I'll try to fix up on of these pr's so that it is preserved.

@wjwwood
Copy link
Copy Markdown
Member

wjwwood commented Jan 8, 2018

Closing in favor of #96, for some reason I could edit this pr on the website, but I couldn't push to your branch @bfjelds.

@wjwwood wjwwood closed this Jan 8, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@mikaelarguedas mikaelarguedas mikaelarguedas left review comments

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@bfjelds @mikaelarguedas @wjwwood