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] add missing DLL exports. #1335

Merged
merged 3 commits into from
Mar 5, 2019
Merged

[Windows][melodic] add missing DLL exports. #1335

merged 3 commits into from
Mar 5, 2019

Conversation

seanyen
Copy link
Contributor

@seanyen seanyen commented Feb 13, 2019

Define DLL exports for rviz (http://wiki.ros.org/win_ros/Contributing/Dll%20Exports).

This change is verified by https://aka.ms/ros project.

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.

I suggest to use cmake's functions generate_export_header and rename rviz_macros.h to rviz_export.h, because the include only deals with export definitions.

src/rviz/rviz_macros.h Outdated Show resolved Hide resolved
@seanyen
Copy link
Contributor Author

seanyen commented Feb 17, 2019

I suggest to use cmake's functions generate_export_header.

The primary reason is we saw that many ROS projects have been already following this ROS Wiki to define the exports header, and figured we are not going to use another idiom to diverge from this existing one for ROS1 community. What do you think?

@rhaschke
Copy link
Contributor

The ROS wiki page was created back in 2013, when cmake didn't yet provided this neat function.
I think, you should update the wiki and let cmake do it's job. It doesn't make sense to write such boiler-plate code manually.

@seanyen
Copy link
Contributor Author

seanyen commented Feb 18, 2019

The ROS wiki page was created back in 2013, when cmake didn't yet provided this neat function.
I think, you should update the wiki and let cmake do it's job. It doesn't make sense to write such boiler-plate code manually.

Understood. I switched the implementation to make use of CMake built-in generate_export_header.

@seanyen
Copy link
Contributor Author

seanyen commented Feb 18, 2019

catkin_lint seems not to get along with dynamically generated files?

catkin_lint /root/rviz
rviz: src/rviz/CMakeLists.txt(183): error: install() needs missing file '${PROJECT_BUILD_DIR}/src/rviz/rviz/rviz_export.h'
rviz: src/rviz/default_plugin/CMakeLists.txt(87): error: install() needs missing file '${PROJECT_BUILD_DIR}/src/rviz/default_plugin/rviz/default_plugin/rviz_default_plugin_export.h'

@rhaschke
Copy link
Contributor

catkin_lint seems not to get along with dynamically generated files?

Yes, I know. Please file a bug at https://github.com/fkie/catkin_lint.
There is the possibility to ignore issues once.

@seanyen
Copy link
Contributor Author

seanyen commented Feb 19, 2019

@rhaschke Thanks. #catkin_lint: ignore_once missing_file does the trick.

@seanyen
Copy link
Contributor Author

seanyen commented Feb 21, 2019

Looking into this error...

Errors     << rviz:make /root/ws_moveit/logs/rviz/build.make.000.log
make[3]: warning: jobserver unavailable: using -j1.  Add '+' to parent make rule.
In file included from panel_dock_widget.sip:7:0:
/root/ws_moveit/src/rviz/src/rviz/panel_dock_widget.h:34:10: fatal error: rviz/rviz_export.h: No such file or directory
 #include "rviz/rviz_export.h"
          ^~~~~~~~~~~~~~~~~~~~
compilation terminated.

@seanyen
Copy link
Contributor Author

seanyen commented Feb 21, 2019

travis CI errors are fixed.

@seanyen
Copy link
Contributor Author

seanyen commented Mar 1, 2019

@rhaschke this is ready for another round of review. Let me know if any feedbacks, thanks!

@rhaschke rhaschke merged commit a3abb63 into ros-visualization:wip-windows-port Mar 5, 2019
rhaschke pushed a commit that referenced this pull request Mar 5, 2019
rhaschke pushed a commit that referenced this pull request Mar 6, 2019
@seanyen seanyen deleted the windows_port_rviz_decl branch April 12, 2019 20:52
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants