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][kinetic] Handling build configuration keywords before passed to SIP #60

Merged
merged 2 commits into from Mar 14, 2019

Conversation

@seanyen
Copy link
Contributor

seanyen commented Feb 1, 2019

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

This comment has been minimized.

Copy link
Member

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

This comment has been minimized.

Copy link
Contributor 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

This comment has been minimized.

Copy link
Member

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

This comment has been minimized.

Copy link
Contributor 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

This comment has been minimized.

Copy link
Contributor 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
set(LIBRARY_DIRS ${${PROJECT_NAME}_LIBRARY_DIRS})
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})

This comment has been minimized.

Copy link
@dirk-thomas

dirk-thomas Mar 14, 2019

Member

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.

This comment has been minimized.

Copy link
@seanyen

seanyen Mar 14, 2019

Author Contributor

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

This comment has been minimized.

Copy link
Member

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

This comment has been minimized.

Copy link
Contributor 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
Projects
None yet
2 participants
You can’t perform that action at this time.