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] Windows\MSVC port #239

Open
wants to merge 1 commit into
base: melodic-devel
Choose a base branch
from

Conversation

seanyen
Copy link

@seanyen seanyen commented Jun 7, 2019

This is a change to enable descartes to build by MSVC toolchains with https://aka.ms/ros installation.

  • Fixed installation location to align with catkin guide.
  • Added necessary dllexport/dllimport for public interfaces.
  • Conditionally include TR1 headers by checking C++ standard version supported by compilers.
  • Favor C++11 attributes [[gnu::unused]].
  • Conditionally add compiler options -std=c++11 -Wall -Wextra by checking what compiler is in use.
  • Added missing eigen_conversions dependency since some cpp files consuming it. (e.g., ikfast_moveit_state_adapter.cpp.)

@seanyen seanyen marked this pull request as ready for review June 7, 2019 20:56
@seanyen seanyen changed the title WIP: [Windows][melodic-devel] Windows\MSVC port [Windows][melodic-devel] Windows\MSVC port Jun 7, 2019
@seanyen
Copy link
Author

seanyen commented Jun 11, 2019

The CI failure seems not to do with the change. The error message are as followed:

: GPG error: http://packages.ros.org/ros-shadow-fixed/ubuntu bionic InRelease: The following signatures couldn't be verified because the public key is not available: NO_PUBKEY F42ED6FBAB17C654
E: The repository 'http://packages.ros.org/ros-shadow-fixed/ubuntu bionic InRelease' is not signed.
The command '/bin/sh -c sed -i "/^# deb.*multiverse/ s/^# //" /etc/apt/sources.list     && apt-get update -qq     && apt-get -qq install --no-install-recommends -y         build-essential         python-catkin-tools         python-pip         python-rosdep         python-wstool         ros-melodic-catkin         ssh-client     && apt-get clean     && rm -rf /var/lib/apt/lists/*' returned a non-zero code: 100
<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<
Function 'prepare_docker_image' returned with code '100' after 0 min 25 sec
The command "source .ci_config/travis.sh" exited with 100.

@seanyen
Copy link
Author

seanyen commented Jun 11, 2019

@jrgnicho Hope I am pinging the correct owner. Any feedback would be appreciated!

@gavanderhoorn
Copy link
Member

I've restarted the build.

This is most likely due to the recent key change.

@seanyen
Copy link
Author

seanyen commented Jun 28, 2019

@gavanderhoorn @jrgnicho Sorry to ping again. Any chance to take a look and make this PR move forward?

@gavanderhoorn
Copy link
Member

@seanyen: apologies, I was not notified of this for some reason.

I'm actually not a maintainer here. Most changes look like they're simple enough, so a review should be easy.

@jrgnicho: ping.

@@ -190,7 +190,7 @@ bool PlanningGraph::calculateJointSolutions(const TrajectoryPtPtr* points, const
bool success = true;

#pragma omp parallel for shared(success)
for (std::size_t i = 0; i < count; ++i)
for (int i = 0; i < count; ++i)
Copy link
Contributor

Choose a reason for hiding this comment

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

Why change the iterator datatype here?

@@ -26,7 +28,7 @@ struct IdGenerator;
* for the unique 'state'. Zero is reserved as a special value
*/
template <>
struct IdGenerator<uint64_t>
struct DESCARTES_CORE_DECL IdGenerator<uint64_t>
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm not too familiar with MSVC toolchains so could you explain why this is needed?

@@ -64,7 +64,7 @@ bool PlanningGraph::insertGraph(const std::vector<TrajectoryPtPtr>& points)

// now we have a graph with data in the 'rungs' and we need to compute the edges
#pragma omp parallel for
for (std::size_t i = 0; i < graph_.size() - 1; ++i)
for (int i = 0; i < graph_.size() - 1; ++i)
Copy link
Contributor

Choose a reason for hiding this comment

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

Why change the iterator type?

if(NOT MSVC)
add_compile_options(-std=c++11 -Wall -Wextra)
else()
add_definitions(-D__PRETTY_FUNCTION__=__FUNCTION__)
Copy link
Contributor

Choose a reason for hiding this comment

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

What does this definition do?

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.

3 participants