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

Add rclc_parameter Quality Declaration #144

Merged
merged 8 commits into from
Aug 13, 2021
Merged

Conversation

pablogs9
Copy link
Member

Closes #137

@pablogs9 pablogs9 requested a review from JanStaschulat July 20, 2021 05:59
@pablogs9
Copy link
Member Author

@mergify backport galactic

@mergify
Copy link
Contributor

mergify bot commented Jul 20, 2021

Command backport galactic: pending

Waiting for the pull request to get merged

Hey, I reacted but my real name is @Mergifyio

@codecov-commenter
Copy link

codecov-commenter commented Jul 20, 2021

Codecov Report

Merging #144 (f76cce6) into master (6ae73ca) will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #144      +/-   ##
==========================================
  Coverage   31.31%   31.31%              
==========================================
  Files         128       32      -96     
  Lines       15776     3944   -11832     
  Branches     7900     1975    -5925     
==========================================
- Hits         4940     1235    -3705     
+ Misses       4128     1032    -3096     
+ Partials     6708     1677    -5031     
Impacted Files Coverage Δ
...src/j5mjh33xjwg/rclc/rclc/test/rclc/test_timer.cpp
...67c7n/rclc/rclc_examples/src/example_client_node.c
ros_ws/src/bpfru1rf0t8/rclc/rclc/src/rclc/client.c
...c/rclc_examples/src/example_executor_convenience.c
.../rclc/rclc_examples/src/example_parameter_server.c
...rc/ul88c67c7n/rclc/rclc/src/rclc/executor_handle.c
...bpfru1rf0t8/rclc/rclc/test/rclc/test_publisher.cpp
...clc_parameter/src/rclc_parameter/parameter_utils.c
...rclc_lifecycle/src/rclc_lifecycle/rclc_lifecycle.c
...rq/rclc/rclc_examples/src/example_lifecycle_node.c
... and 150 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 6ae73ca...f76cce6. Read the comment docs.

Copy link
Contributor

@JanStaschulat JanStaschulat left a comment

Choose a reason for hiding this comment

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

@pablogs9 @norro We need to test rclc_parameter on Windows first before we can add this quality declaration for this package.

@norro
Copy link
Collaborator

norro commented Aug 12, 2021

With the branch feature/parameter_quality, I get the following errors when building in Windows 10:
grafik

@pablogs9
Copy link
Member Author

Thanks, @norro, can you copy-paste the output?

@JanStaschulat that seems to be in the executor test also...

@norro
Copy link
Collaborator

norro commented Aug 12, 2021

Sure!

C:\Users\compass\ros2_ws\src\rclc\rclc\test\rclc\test_executor.cpp(1710,10): warning C4477: 'printf' : format string '%ld' requires an argument of type 'long', but variadic argument 2 has type 'uint64_t' [C:\Users\compass\ros2_ws\build\rclc\rclc_test.vcxproj]
C:\Users\compass\ros2_ws\src\rclc\rclc\test\rclc\test_executor.cpp(1710,10): message : consider using '%lld' in the format string [C:\Users\compass\ros2_ws\build\rclc\rclc_test.vcxproj]
C:\Users\compass\ros2_ws\src\rclc\rclc\test\rclc\test_executor.cpp(1710,10): message : consider using '%Id' in the format string [C:\Users\compass\ros2_ws\build\rclc\rclc_test.vcxproj]
C:\Users\compass\ros2_ws\src\rclc\rclc\test\rclc\test_executor.cpp(1710,10): message : consider using '%I64d' in the format string [C:\Users\compass\ros2_ws\build\rclc\rclc_test.vcxproj]
test_executor.obj : error LNK2019: unresolved external symbol rclc_executor_add_subscription_with_context referenced in function "private: virtual void __cdecl TestDefaultExecutor_executor_add_subscription_with_context_Test::TestBody(void)" (?TestBody@TestDefaultExecutor_executor_add_subscription_with_context_Test@@EEAAXXZ) [C:\Users\compass\ros2_ws\build\rclc\rclc_test.vcxproj]
test_executor.obj : error LNK2019: unresolved external symbol rclc_executor_remove_subscription referenced in function "private: virtual void __cdecl TestDefaultExecutor_executor_remove_subscription_Test::TestBody(void)" (?TestBody@TestDefaultExecutor_executor_remove_subscription_Test@@EEAAXXZ) [C:\Users\compass\ros2_ws\build\rclc\rclc_test.vcxproj]
test_executor.obj : error LNK2019: unresolved external symbol rclc_executor_remove_timer referenced in function "private: virtual void __cdecl TestDefaultExecutor_executor_remove_timer_Test::TestBody(void)" (?TestBody@TestDefaultExecutor_executor_remove_timer_Test@@EEAAXXZ) [C:\Users\compass\ros2_ws\build\rclc\rclc_test.vcxproj]
test_executor.obj : error LNK2019: unresolved external symbol rclc_executor_remove_client referenced in function "private: virtual void __cdecl TestDefaultExecutor_executor_remove_client_Test::TestBody(void)" (?TestBody@TestDefaultExecutor_executor_remove_client_Test@@EEAAXXZ) [C:\Users\compass\ros2_ws\build\rclc\rclc_test.vcxproj]
test_executor.obj : error LNK2019: unresolved external symbol rclc_executor_remove_service referenced in function "private: virtual void __cdecl TestDefaultExecutor_executor_remove_service_Test::TestBody(void)" (?TestBody@TestDefaultExecutor_executor_remove_service_Test@@EEAAXXZ) [C:\Users\compass\ros2_ws\build\rclc\rclc_test.vcxproj]
test_executor.obj : error LNK2019: unresolved external symbol rclc_executor_remove_guard_condition referenced in function "private: virtual void __cdecl TestDefaultExecutor_executor_test_remove_guard_condition_Test::TestBody(void)" (?TestBody@TestDefaultExecutor_executor_test_remove_guard_condition_Test@@EEAAXXZ) [C:\Users\compass\ros2_ws\build\rclc\rclc_test.vcxproj]
C:\Users\compass\ros2_ws\build\rclc\Release\rclc_test.exe : fatal error LNK1120: 6 unresolved externals [C:\Users\compass\ros2_ws\build\rclc\rclc_test.vcxproj]
Failed   <<< rclc [44.0s, exited with code 1]

Summary: 0 packages finished [44.4s]
  1 package failed: rclc

@pablogs9
Copy link
Member Author

Those functions are not related to rclc_parameter, we should check the Windows compatibility of the rest of the package @JanStaschulat

@JanStaschulat
Copy link
Contributor

JanStaschulat commented Aug 12, 2021

Hmm, all these functions rclc_executor_remove_*have been added later. I'll check, I remember from my first port of rclc to Windows this has to do with visibility control.

The macro RCLC_PUBLIC is missing in front of those functions. I'll update that.

@JanStaschulat JanStaschulat force-pushed the feature/parameter_quality branch from 5885006 to 126a762 Compare August 13, 2021 06:26
@norro
Copy link
Collaborator

norro commented Aug 13, 2021

Build of rclc_parameters in 126a762 fails on Windows 10 with

test_parameter.obj : error LNK2019: unresolved external symbol __imp_rclc_parameter_server_init_default referenced in function "private: virtual void __cdecl Test_rclc_node_init_default_Test::TestBody(void)" (?TestBody@Test_rclc_node_init_default_Test@@EEAAXXZ) [C:\Users\compass\ros2_ws\build\rclc_parameter\rclc_parameter_test.vcxproj]
test_parameter.obj : error LNK2019: unresolved external symbol __imp_rclc_executor_add_parameter_server referenced in function "private: virtual void __cdecl Test_rclc_node_init_default_Test::TestBody(void)" (?TestBody@Test_rclc_node_init_default_Test@@EEAAXXZ) [C:\Users\compass\ros2_ws\build\rclc_parameter\rclc_parameter_test.vcxproj]
test_parameter.obj : error LNK2019: unresolved external symbol __imp_rclc_add_parameter referenced in function "private: virtual void __cdecl Test_rclc_node_init_default_Test::TestBody(void)" (?TestBody@Test_rclc_node_init_default_Test@@EEAAXXZ) [C:\Users\compass\ros2_ws\build\rclc_parameter\rclc_parameter_test.vcxproj]
test_parameter.obj : error LNK2019: unresolved external symbol __imp_rclc_parameter_set_bool referenced in function "private: virtual void __cdecl Test_rclc_node_init_default_Test::TestBody(void)" (?TestBody@Test_rclc_node_init_default_Test@@EEAAXXZ) [C:\Users\compass\ros2_ws\build\rclc_parameter\rclc_parameter_test.vcxproj]
test_parameter.obj : error LNK2019: unresolved external symbol __imp_rclc_parameter_set_int referenced in function "private: virtual void __cdecl Test_rclc_node_init_default_Test::TestBody(void)" (?TestBody@Test_rclc_node_init_default_Test@@EEAAXXZ) [C:\Users\compass\ros2_ws\build\rclc_parameter\rclc_parameter_test.vcxproj]
test_parameter.obj : error LNK2019: unresolved external symbol __imp_rclc_parameter_set_double referenced in function "private: virtual void __cdecl Test_rclc_node_init_default_Test::TestBody(void)" (?TestBody@Test_rclc_node_init_default_Test@@EEAAXXZ) [C:\Users\compass\ros2_ws\build\rclc_parameter\rclc_parameter_test.vcxproj]
test_parameter.obj : error LNK2019: unresolved external symbol __imp_rclc_parameter_get_bool referenced in function "private: virtual void __cdecl Test_rclc_node_init_default_Test::TestBody(void)" (?TestBody@Test_rclc_node_init_default_Test@@EEAAXXZ) [C:\Users\compass\ros2_ws\build\rclc_parameter\rclc_parameter_test.vcxproj]
test_parameter.obj : error LNK2019: unresolved external symbol __imp_rclc_parameter_get_int referenced in function "private: virtual void __cdecl Test_rclc_node_init_default_Test::TestBody(void)" (?TestBody@Test_rclc_node_init_default_Test@@EEAAXXZ) [C:\Users\compass\ros2_ws\build\rclc_parameter\rclc_parameter_test.vcxproj]
test_parameter.obj : error LNK2019: unresolved external symbol __imp_rclc_parameter_get_double referenced in function "private: virtual void __cdecl Test_rclc_node_init_default_Test::TestBody(void)" (?TestBody@Test_rclc_node_init_default_Test@@EEAAXXZ) [C:\Users\compass\ros2_ws\build\rclc_parameter\rclc_parameter_test.vcxproj]
C:\Users\compass\ros2_ws\build\rclc_parameter\Release\rclc_parameter_test.exe : fatal error LNK1120: 9 unresolved externals [C:\Users\compass\ros2_ws\build\rclc_parameter\rclc_parameter_test.vcxproj]
Failed   <<< rclc_parameter [6.84s, exited with code 1]

Summary: 2 packages finished [18.8s]
  1 package failed: rclc_parameter

@pablogs9
Copy link
Member Author

Those new symbols have the RCLC_PARAMETER_PUBLIC set. Any idea on how to solve it @JanStaschulat? Thanks!

@pablogs9
Copy link
Member Author

Maybe e5bb567? Can you test it again @norro? Sorry for the inconvenience...

@JanStaschulat
Copy link
Contributor

not yet:

  cl /c /IC:\dev\ros2_galactic\src\gtest_vendor\include /IC:\Users\compass\ros2_ws\src\rclc\rclc_parameter\include /IC:\Users\compass\ros2_ws\src\rclc\rclc_parameter\src /IC:\dev\ros2_galactic\include /IC:\Users\compass\ros2_ws\install\include /W3 /WX- /diagnostics:column /O2 /Ob2 /D WIN32 /D _WINDOWS /D NDEBUG /D RCLC_PARAMETER_MAX_STRING_LENGTH=50 /D _CRT_SECURE_NO_WARNINGS /D DEFAULT_RMW_IMPLEMENTATION=rmw_cyclonedds_cpp /D _SILENCE_ALL_CXX17_DEPRECATION_WARNINGS /D "CMAKE_INTDIR=\"Release\"" /D _MBCS /Gm- /EHsc /MD /GS /fp:precise /Zc:wchar_t /Zc:forScope /Zc:inline /GR /std:c++17 /Fo"rclc_parameter_test.dir\Release\\" /Fd"rclc_parameter_test.dir\Release\vc142.pdb" /external:env:EXTERNAL_INCLUDE /external:W3 /Gd /TP /errorReport:queue C:\Users\compass\ros2_ws\src\rclc\rclc_parameter\test\rclc_parameter\test_parameter.cpp
  test_parameter.cpp
test_parameter.obj : error LNK2019: unresolved external symbol __imp_rclc_parameter_server_init_default referenced in function "private: virtual void __cdecl Test_rclc_node_init_default_Test::TestBody(void)" (?TestBody@Test_rclc_node_init_default_Test@@EEAAXXZ) [C:\Users\compass\ros2_ws\build\rclc_parameter\rclc_parameter_test.vcxproj]
test_parameter.obj : error LNK2019: unresolved external symbol __imp_rclc_executor_add_parameter_server referenced in function "private: virtual void __cdecl Test_rclc_node_init_default_Test::TestBody(void)" (?TestBody@Test_rclc_node_init_default_Test@@EEAAXXZ) [C:\Users\compass\ros2_ws\build\rclc_parameter\rclc_parameter_test.vcxproj]
test_parameter.obj : error LNK2019: unresolved external symbol __imp_rclc_add_parameter referenced in function "private: virtual void __cdecl Test_rclc_node_init_default_Test::TestBody(void)" (?TestBody@Test_rclc_node_init_default_Test@@EEAAXXZ) [C:\Users\compass\ros2_ws\build\rclc_parameter\rclc_parameter_test.vcxproj]
test_parameter.obj : error LNK2019: unresolved external symbol __imp_rclc_parameter_set_bool referenced in function "private: virtual void __cdecl Test_rclc_node_init_default_Test::TestBody(void)" (?TestBody@Test_rclc_node_init_default_Test@@EEAAXXZ) [C:\Users\compass\ros2_ws\build\rclc_parameter\rclc_parameter_test.vcxproj]
test_parameter.obj : error LNK2019: unresolved external symbol __imp_rclc_parameter_set_int referenced in function "private: virtual void __cdecl Test_rclc_node_init_default_Test::TestBody(void)" (?TestBody@Test_rclc_node_init_default_Test@@EEAAXXZ) [C:\Users\compass\ros2_ws\build\rclc_parameter\rclc_parameter_test.vcxproj]
test_parameter.obj : error LNK2019: unresolved external symbol __imp_rclc_parameter_set_double referenced in function "private: virtual void __cdecl Test_rclc_node_init_default_Test::TestBody(void)" (?TestBody@Test_rclc_node_init_default_Test@@EEAAXXZ) [C:\Users\compass\ros2_ws\build\rclc_parameter\rclc_parameter_test.vcxproj]
test_parameter.obj : error LNK2019: unresolved external symbol __imp_rclc_parameter_get_bool referenced in function "private: virtual void __cdecl Test_rclc_node_init_default_Test::TestBody(void)" (?TestBody@Test_rclc_node_init_default_Test@@EEAAXXZ) [C:\Users\compass\ros2_ws\build\rclc_parameter\rclc_parameter_test.vcxproj]
test_parameter.obj : error LNK2019: unresolved external symbol __imp_rclc_parameter_get_int referenced in function "private: virtual void __cdecl Test_rclc_node_init_default_Test::TestBody(void)" (?TestBody@Test_rclc_node_init_default_Test@@EEAAXXZ) [C:\Users\compass\ros2_ws\build\rclc_parameter\rclc_parameter_test.vcxproj]
test_parameter.obj : error LNK2019: unresolved external symbol __imp_rclc_parameter_get_double referenced in function "private: virtual void __cdecl Test_rclc_node_init_default_Test::TestBody(void)" (?TestBody@Test_rclc_node_init_default_Test@@EEAAXXZ) [C:\Users\compass\ros2_ws\build\rclc_parameter\rclc_parameter_test.vcxproj]
C:\Users\compass\ros2_ws\build\rclc_parameter\Release\rclc_parameter_test.exe : fatal error LNK1120: 9 unresolved externals [C:\Users\compass\ros2_ws\build\rclc_parameter\rclc_parameter_test.vcxproj]
Failed   <<< rclc_parameter [16.2s, exited with code 1]

Summary: 0 packages finished [16.5s]
  1 package failed: rclc_parameter

c:\Users\compass\ros2_ws>

@pablogs9
Copy link
Member Author

No idea what is happening. Can you provide some tips @JanStaschulat?

@JanStaschulat
Copy link
Contributor

JanStaschulat commented Aug 13, 2021

we have been working on it.
The macro in CMakeLists.txt is not taken over into the visibility_control.h.
We experimented a lot, this fix works:

target_compile_definitions(${PROJECT_NAME}
  PUBLIC "RCLC_PARAMETER_BUILDING_LIBRARY")

also this one would

add_definitions(-DRCLC_PARAMETER_BUILDING_LIBRARY)

We don't know, why the previous configuration with PRIVATE works for rclc and rclc_lifecycle but not for rclc_parameter

@JanStaschulat JanStaschulat force-pushed the feature/parameter_quality branch from 587dbf8 to b101b3e Compare August 13, 2021 10:26
@pablogs9
Copy link
Member Author

Thanks a lot

@JanStaschulat JanStaschulat force-pushed the feature/parameter_quality branch from b101b3e to a9a572d Compare August 13, 2021 10:36
@JanStaschulat
Copy link
Contributor

@pablogs9 can you sign your commit? I tried to signoff the last three commits, but Github expects you to sign your commit.
https://github.com/ros2/rclc/pull/144/checks?check_run_id=3321167322

@pablogs9 pablogs9 force-pushed the feature/parameter_quality branch 2 times, most recently from ec36b74 to 9836e9c Compare August 13, 2021 11:25
JanStaschulat referenced this pull request in ros2/rcl Aug 13, 2021
* Add tracing instrumentation for rcl_take

Signed-off-by: Christophe Bedard <[email protected]>

* Remove redundant publisher field in rcl_publish tracepoint

Signed-off-by: Christophe Bedard <[email protected]>

* Re-add publisher handle field in rcl_publish tracepoint

Signed-off-by: Christophe Bedard <[email protected]>
@pablogs9 pablogs9 force-pushed the feature/parameter_quality branch from 9836e9c to 5f31fbf Compare August 13, 2021 11:26
pablogs9 and others added 3 commits August 13, 2021 13:26
Signed-off-by: Jan Staschulat <[email protected]>

Signed-off-by: Jan Staschulat <[email protected]>
Signed-off-by: Jan Staschulat <[email protected]>

Signed-off-by: Jan Staschulat <[email protected]>
@pablogs9 pablogs9 force-pushed the feature/parameter_quality branch from 5f31fbf to 6e82daa Compare August 13, 2021 11:26
@pablogs9
Copy link
Member Author

I don't understand this:

image

I'm clicking Set DCO to pass

@JanStaschulat
Copy link
Contributor

JanStaschulat commented Aug 13, 2021

There is a new commit on rcl which leads to a failing github-ci job:
https://github.com/ros2/rclc/runs/3321167476?check_suite_focus=true

I commented on this problem here: ros2/rcl commit.

@JanStaschulat
Copy link
Contributor

This version of rclc_parameters compiles and links on Windows. The units tests are all successfull. From my side, you can merge this pull request.

@pablogs9
Copy link
Member Author

Nice, thanks a lot for the help. Merging.

@pablogs9 pablogs9 merged commit 6f9f9f4 into master Aug 13, 2021
@pablogs9 pablogs9 deleted the feature/parameter_quality branch August 13, 2021 11:37
@pablogs9
Copy link
Member Author

@mergify backport galactic

mergify bot pushed a commit that referenced this pull request Aug 13, 2021
* Add rclc_parameter Quality Declaration

Signed-off-by: Jan Staschulat <[email protected]>

* Update rclc_parameter/QUALITY_DECLARATION.md

Co-authored-by: Jan Staschulat <[email protected]>
Signed-off-by: Jan Staschulat <[email protected]>

* added RCL_PUBLIC to function declarations in executor.h

Signed-off-by: Jan Staschulat <[email protected]>

* updated printf format string to %lld

Signed-off-by: Jan Staschulat <[email protected]>

* corrected printf format for 'unsigned long'

Signed-off-by: Jan Staschulat <[email protected]>

* Modify building library flag in params

Signed-off-by: Pablo Garrido  <[email protected]>

* fix target_compile_definitions for visibility macro

Signed-off-by: Jan Staschulat <[email protected]>

Signed-off-by: Jan Staschulat <[email protected]>

* ament_uncrustify

Signed-off-by: Jan Staschulat <[email protected]>

Signed-off-by: Jan Staschulat <[email protected]>

Co-authored-by: Jan Staschulat <[email protected]>
(cherry picked from commit 6f9f9f4)
@mergify
Copy link
Contributor

mergify bot commented Aug 13, 2021

Command backport galactic: success

Backports have been created

@mergify
Copy link
Contributor

mergify bot commented Aug 13, 2021

Command backport galactic: success

Backports have been created

Hey, I reacted but my real name is @Mergifyio

@JanStaschulat
Copy link
Contributor

JanStaschulat commented Aug 13, 2021

Backport to Foxy we have to do more selectivly, I guess?

@pablogs9
Copy link
Member Author

pablogs9 commented Aug 13, 2021

We do not have parameters in Foxy, but the other changes yes...

JanStaschulat added a commit that referenced this pull request Aug 13, 2021
Signed-off-by: Jan Staschulat <[email protected]>
@JanStaschulat
Copy link
Contributor

JanStaschulat commented Aug 13, 2021

Created a selected backport for Foxy #182 to resolve missing visibility of new functions.

JanStaschulat added a commit that referenced this pull request Aug 13, 2021
JanStaschulat added a commit that referenced this pull request Aug 17, 2021
* Add rclc_parameter Quality Declaration (#144)

* Add rclc_parameter Quality Declaration

Signed-off-by: Jan Staschulat <[email protected]>

* Update rclc_parameter/QUALITY_DECLARATION.md

Co-authored-by: Jan Staschulat <[email protected]>
Signed-off-by: Jan Staschulat <[email protected]>

* added RCL_PUBLIC to function declarations in executor.h

Signed-off-by: Jan Staschulat <[email protected]>

* updated printf format string to %lld

Signed-off-by: Jan Staschulat <[email protected]>

* corrected printf format for 'unsigned long'

Signed-off-by: Jan Staschulat <[email protected]>

* Modify building library flag in params

Signed-off-by: Pablo Garrido  <[email protected]>

* fix target_compile_definitions for visibility macro

Signed-off-by: Jan Staschulat <[email protected]>

Signed-off-by: Jan Staschulat <[email protected]>

* ament_uncrustify

Signed-off-by: Jan Staschulat <[email protected]>

Signed-off-by: Jan Staschulat <[email protected]>

Co-authored-by: Jan Staschulat <[email protected]>
(cherry picked from commit 6f9f9f4)

* fixed dependency of rcl on galactic branch

Signed-off-by: Jan Staschulat <[email protected]>

Co-authored-by: Pablo Garrido <[email protected]>
Co-authored-by: Jan Staschulat <[email protected]>
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.

[rclc_parameter] Add quality declaration statement
4 participants