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

feat: support ur client library for windows #264

Merged
merged 9 commits into from
Feb 4, 2025

Conversation

wep21
Copy link
Contributor

@wep21 wep21 commented Jan 31, 2025

No description provided.

@wep21 wep21 marked this pull request as draft January 31, 2025 03:18
Signed-off-by: wep21 <[email protected]>
@wep21
Copy link
Contributor Author

wep21 commented Jan 31, 2025

@traversaro @Tobias-Fischer Do you have any idea why msvc compiler doesn't have c++17 support?

 │ │ CMake Error at CMakeModules/DefineCXX17CompilerFlag.cmake:50 (message):
 │ │   The compiler C:/Program Files (x86)/Microsoft Visual
 │ │   Studio/2019/Enterprise/VC/Tools/MSVC/14.29.30133/bin/Hostx64/x64/cl.exe has
 │ │   no C++17 support.  Please use a different C++ compiler.

@traversaro
Copy link
Member

traversaro commented Jan 31, 2025

I can't find that cmake file, but I guess it tests if the compiled supports the -std=c++17 option, while msvc uses -std:c++17 . Anyhow I think we can just set c++17 by setting CMAKE_CXX_STANDARD variable to 17 or other ways to enable c++17 in other ways (like target_compile_features) in a compiler agnostic way.

@traversaro
Copy link
Member

xref potentially interesting PR: UniversalRobots/Universal_Robots_Client_Library#229

@traversaro
Copy link
Member

Anyhow I think we can just set c++17 by setting CMAKE_CXX_STANDARD variable to 17 or other ways to enable c++17 in other ways (like target_compile_features) in a compiler agnostic way.

See also the related PR https://github.com/UniversalRobots/Universal_Robots_Client_Library/pull/244/files . To have a minimal patch that works on 1.5.0, I think we can just do something similar to traversaro/Universal_Robots_Client_Library@a4148d4 .

Signed-off-by: wep21 <[email protected]>
@wep21
Copy link
Contributor Author

wep21 commented Jan 31, 2025

@traversaro Thanks, I've mistakenly looked into v1.6.0.

Signed-off-by: wep21 <[email protected]>
@traversaro
Copy link
Member

traversaro commented Jan 31, 2025

For the jazzy I prepare a patch for windows on top of 1.5.0: https://github.com/traversaro/Universal_Robots_Client_Library/tree/winfix .

vinca_win.yaml Outdated Show resolved Hide resolved
@wep21 wep21 changed the title feat: support ur windows feat: support ur client libraries for windows Feb 1, 2025
@wep21 wep21 changed the title feat: support ur client libraries for windows feat: support ur client library for windows Feb 1, 2025
@wep21
Copy link
Contributor Author

wep21 commented Feb 1, 2025

@traversaro Thank you for your patch. Works fine for ur client library, but I found out other packages also need more patch.
I decided to make only ur client library available in this PR.

@wep21 wep21 marked this pull request as ready for review February 1, 2025 02:58
@traversaro
Copy link
Member

@traversaro Thank you for your patch. Works fine for ur client library, but I found out other packages also need more patch. I decided to make only ur client library available in this PR.

Ok, perfect! I have some more patches for ur-robot-driver in RoboStack/ros-jazzy#19, if you are interested you could look into backporting those to humble.

vinca_win.yaml Outdated Show resolved Hide resolved
- target_link_options(urcl PUBLIC -fsanitize=address)
+
+if(MSVC)
+ include_directories(${CMAKE_CURRENT_SOURCE_DIR}/3rdparty/endian)
Copy link
Member

Choose a reason for hiding this comment

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

I am afraid this does not work as the endian.h is not installed, and it is include in public headers, so any downstream package would fail.

Copy link
Member

Choose a reason for hiding this comment

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@traversaro Thanks. synced patch with jazzy 5b25318.

Co-authored-by: Silvio Traversaro <[email protected]>
@traversaro
Copy link
Member

but I found out other packages also need more patch.

fyi I did several patches for other ur packages, see RoboStack/ros-jazzy#19 .

@wep21 wep21 requested a review from traversaro February 3, 2025 00:16
@traversaro
Copy link
Member

Thanks @wep21 !

@traversaro traversaro merged commit e9557aa into RoboStack:main Feb 4, 2025
5 checks passed
@wep21 wep21 deleted the ur-windows branch February 4, 2025 10:24
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.

2 participants