Skip to content

[Windows][kinetic] Handling build configuration keywords before passed to SIP#60

Merged
dirk-thomas merged 2 commits intoros-visualization:kinetic-develfrom
ms-iot:python_qt_binding_windows_fix
Mar 14, 2019
Merged

[Windows][kinetic] Handling build configuration keywords before passed to SIP#60
dirk-thomas merged 2 commits intoros-visualization:kinetic-develfrom
ms-iot:python_qt_binding_windows_fix

Conversation

@seanyen
Copy link
Copy Markdown

@seanyen seanyen commented Feb 1, 2019 •
edited
Loading

In Windows build, the generated CMake Config for package could contain the configuration keywords and the sip_configure.py doesn't expect that. So use catkin_filter_libraries_for_build_configuration to normalize the content before it is passed to SIP.

FYI, here is an example what we saw from ${${PROJECT_NAME}_LIBRARIES}

"qt_gui_cpp;optimized;C:/opt/rosdeps/x64/lib/boost_filesystem-vc141-mt-x64-1_66.lib;debug;C:/opt/rosdeps/x64/lib/boost_filesystem-vc141-mt-gd-x64-1_66.lib;optimized;C:/opt/rosdeps/x64/lib/boost_system-vc141-mt-x64-1_66.lib;debug;C:/opt/rosdeps/x64/lib/boost_system-vc141-mt-gd-x64-1_66.lib;C:/opt/rosdeps/x64/lib/tinyxml.lib"

And without the fix, when it runs into the CMake config with configuration keywords, the linker could fail on the following errors:

        link /NOLOGO /DYNAMICBASE /NXCOMPAT /LIBPATH:C:/rviz_ws/devel_isolated/qt_gui_cpp/lib /DLL /MANIFEST /MANIFESTFILE:"C:/rviz_ws/devel_isolated/qt_gui_cpp/lib/site-packages/qt_gui_cpp\libqt_gui_cpp_sip".pyd.manifest /SUBSYSTEM:WINDOWS /INCREMENTAL:NO /OUT:"C:/rviz_ws/devel_isolated/qt_gui_cpp/lib/site-packages/qt_gui_cpp\libqt_gui_cpp_sip".pyd @C:\Users\seanyen\AppData\Local\Temp\nmA71.tmp
LINK : fatal error LNK1181: cannot open input file 'optimized.lib'
NMAKE : fatal error U1077: '"C:\Program Files (x86)\Microsoft Visual Studio\2017\Enterprise\VC\Tools\MSVC\14.15.26726\bin\HostX64\x64\link.EXE"' : return code '0x49d'

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

@dirk-thomas
Copy link
Copy Markdown
Contributor

dirk-thomas commented Feb 5, 2019

Can you please compare the proposed changes with the code on the crystal-devel branch which is used for ROS 2 and should already support Windows?

@seanyen
Copy link
Copy Markdown
Author

seanyen commented Feb 7, 2019

@dirk-thomas Thank for the feedback. Do you want me to test ROS on Windows melodic distro with the crystal-devel python_qt_binding? Or are you suggesting me to cherry-pick the necessary changes from ROS2? Currently https://aka.ms/ros is still building ROS1 (melodic distro) and focused on upstreaming the minimum changes for melodic.

@dirk-thomas
Copy link
Copy Markdown
Contributor

dirk-thomas commented Feb 7, 2019

Do you want me to test ROS on Windows melodic distro with the crystal-devel python_qt_binding?

No.

are you suggesting me to cherry-pick the necessary changes from ROS2?

Yes, at least you should check the changes applied for ROS 2 and either cherry-pick them if they apply 1-to-1 or use a custom patch like the one in this PR but with a description why it needs to be different.

@seanyen
Copy link
Copy Markdown
Author

seanyen commented Feb 7, 2019

Thanks for the clarification! I will be reworking on that.

@seanyen seanyen changed the title [Windows] python_qt_binding Windows MSVC bring up [Windows] Handling build configuration keywords before passed to SIP Feb 8, 2019
@seanyen
Copy link
Copy Markdown
Author

seanyen commented Feb 8, 2019

@dirk-thomas I separated out cherry-picking the crystal-devel Windows port into #61. This PR will be focused on the build configuration keyword filtering fix.

@seanyen seanyen changed the title [Windows] Handling build configuration keywords before passed to SIP [Windows][kinetic] Handling build configuration keywords before passed to SIP Feb 8, 2019
dirk-thomas
dirk-thomas reviewed Mar 14, 2019
View reviewed changes
cmake/sip_helper.cmake
set(LDFLAGS_OTHER ${${PROJECT_NAME}_LDFLAGS_OTHER})

# SIP configure doesn't handle build configuration keywords
catkin_filter_libraries_for_build_configuration(LIBRARIES ${${PROJECT_NAME}_LIBRARIES})
Copy link
Copy Markdown
Contributor

@dirk-thomas dirk-thomas Mar 14, 2019

Choose a reason for hiding this comment

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

The package needs to declare a buildtool_export dependency on catkin in the manifest as well as export the dependency in CMake in order for this function to be able to rely on that function to be available.

Copy link
Copy Markdown
Author

@seanyen seanyen Mar 14, 2019

Choose a reason for hiding this comment

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

I followed this guide to upgrade the package format to v2 and added buildtool_export_depend on catkin. Let me know if I am doing it correct or wrong.

@dirk-thomas
Copy link
Copy Markdown
Contributor

dirk-thomas commented Mar 14, 2019

Thanks for the patch and for updating it so quickly.

@dirk-thomas dirk-thomas merged commit 0c42506 into ros-visualization:kinetic-devel Mar 14, 2019
@seanyen
Copy link
Copy Markdown
Author

seanyen commented Mar 14, 2019

Thanks for the merge!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

1 more reviewer

@dirk-thomas dirk-thomas dirk-thomas left review comments

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

2 participants

@seanyen @dirk-thomas