Skip to content

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 .

@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.

- 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