Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[Windows][melodic-devel] Use boost::filesystem::path::string() to return std::string. #1571

Merged
merged 1 commit into from
Jul 19, 2019

Conversation

seanyen
Copy link
Contributor

@seanyen seanyen commented Jul 18, 2019

On Windows, boost::filesystem::path::native() will return std::wstring while the code expects std::string, thus I replaced the native() usage with string() to accommodate it.

@welcome
Copy link

welcome bot commented Jul 18, 2019

Thanks for helping in improving MoveIt!

@seanyen
Copy link
Contributor Author

seanyen commented Jul 18, 2019

@rhaschke Hi Robert! Before I send other Windows-related pull requests, I'd like to check with you first how would you love to proceed the Windows related PRs? Do you want to follow the similar way we did for rivz? Like you create a branch to stage all Windows changes first?

And you can take a quick glance of this diff to feel the change scope: https://github.com/ros-planning/moveit/compare/melodic-devel...ms-iot:windows_fix?expand=1

Copy link
Contributor

@rhaschke rhaschke left a comment

Choose a reason for hiding this comment

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

LGTM. Thanks for your efforts bringing MoveIt to Windows!

@rhaschke
Copy link
Contributor

rhaschke commented Jul 18, 2019

Do you like you create a branch to stage all Windows changes first?

Yes. Indeed, I would prefer to have a windows-port branch for this. I have created an appropriate branch (feature-windows-port). As you originally targeted melodic-devel, please rebase your work onto master and direct PRs to the feature-windows-port branch. We will probably not back-port those changes to melodic-devel.

@rhaschke
Copy link
Contributor

@seanyen, maybe you can join our next maintainer meeting and discuss with all of us how to proceed?
@davetcoleman, could you officially invite him and provide exact times? I'm not sure I will join, because I'm in holidays...

@davetcoleman
Copy link
Member

@seanyen I just invited you, but also, its public now: https://discourse.ros.org/t/moveit-maintainer-meeting-all-invited-july-25th/9899/3

@v4hn
Copy link
Contributor

v4hn commented Jul 19, 2019

For all patches that add overhead, such as #ifdefs, please use the separate feature-windows-support branch.

However, for all patches that amount to fixing API problems in MoveIt (e.g. not using the correct cmake installation variables, assuming native() always returns string, ...) I don't see a reason to scope them together with "windows-support" as they might also break at some point for other systems.
I also don't see a reason not to backport such changes to melodic as bug-fixes,
so I will merge this request and forward-port to master

@v4hn v4hn merged commit 895ded3 into moveit:melodic-devel Jul 19, 2019
@welcome
Copy link

welcome bot commented Jul 19, 2019

Congrats on getting your first MoveIt! pull request merged and improving open source robotics!

@rhaschke
Copy link
Contributor

@v4hn, we wanted to merge this into windows-port related feature branch...

@v4hn
Copy link
Contributor

v4hn commented Jul 19, 2019

@rhaschke please see my reasoning above.

Also this request got two approvals.

@rhaschke
Copy link
Contributor

I see. My reasoning was to group all related changes together... This could facilitate future adaptions in other contexts/projects, because we can just look what kind of changes were required here.

@v4hn
Copy link
Contributor

v4hn commented Jul 19, 2019

That would make sense too. I expected the motivation to be an easy way to revert all windows-specific overhead (by reverting the merge commit) if necessary / of interest for a project.
In that case API fixes and cleanups should still stay...

@v4hn
Copy link
Contributor

v4hn commented Jul 19, 2019

I would instead propose to track a list of such patches elsewhere (maybe in a separate issue)?

@seanyen seanyen deleted the windows_fix_string branch August 20, 2019 09:11
sjahr pushed a commit to sjahr/moveit that referenced this pull request Jun 21, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants