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

Switch to generate YARP idl files at every build to fix compatibility with YARP master #131

Merged
merged 2 commits into from
Nov 23, 2021

Conversation

traversaro
Copy link
Member

@traversaro traversaro commented Nov 23, 2021

While working on robotology/robotology-superbuild#900, I had problems with the idl generated files, as the one generated with YARP 3.5 (I guess) had problems to be compiled with YARP master. To avoid the problem at the root, I switched to generate the idl files at each build (instead of committing them manually) and I removed the autogenerated files that at the moment were committed in the repo. This was done by just generating the idl files in the build directory instead of the source directory.

@traversaro
Copy link
Member Author

Example of failure with latest YARP master without this change (from https://github.com/robotology/robotology-superbuild/runs/4297622640):

2021-11-23T10:22:43.2450517Z [210/248] Performing build step for 'whole-body-estimators'
2021-11-23T10:22:43.2452638Z FAILED: src/whole-body-estimators/CMakeFiles/YCMStamp/whole-body-estimators-build /__w/robotology-superbuild/robotology-superbuild/build/src/whole-body-estimators/CMakeFiles/YCMStamp/whole-body-estimators-build 
2021-11-23T10:22:43.2455777Z cd /__w/robotology-superbuild/robotology-superbuild/build/src/whole-body-estimators && /usr/bin/cmake --build . && /usr/bin/cmake -E touch /__w/robotology-superbuild/robotology-superbuild/build/src/whole-body-estimators/CMakeFiles/YCMStamp/whole-body-estimators-build
2021-11-23T10:22:43.2458229Z [1/37] Building CXX object idl/wholeBodyDynamicsSettings/CMakeFiles/wholeBodyDynamicsSettings.dir/autogenerated/src/KinematicSourceType.cpp.o
2021-11-23T10:22:43.2460315Z FAILED: idl/wholeBodyDynamicsSettings/CMakeFiles/wholeBodyDynamicsSettings.dir/autogenerated/src/KinematicSourceType.cpp.o 
2021-11-23T10:22:43.2467020Z /usr/bin/c++ -D_USE_MATH_DEFINES -I/__w/robotology-superbuild/robotology-superbuild/src/whole-body-estimators/idl/wholeBodyDynamicsSettings/autogenerated/include -isystem /__w/robotology-superbuild/robotology-superbuild/build/install/include -g -fPIC -std=gnu++1z -MD -MT idl/wholeBodyDynamicsSettings/CMakeFiles/wholeBodyDynamicsSettings.dir/autogenerated/src/KinematicSourceType.cpp.o -MF idl/wholeBodyDynamicsSettings/CMakeFiles/wholeBodyDynamicsSettings.dir/autogenerated/src/KinematicSourceType.cpp.o.d -o idl/wholeBodyDynamicsSettings/CMakeFiles/wholeBodyDynamicsSettings.dir/autogenerated/src/KinematicSourceType.cpp.o -c /__w/robotology-superbuild/robotology-superbuild/src/whole-body-estimators/idl/wholeBodyDynamicsSettings/autogenerated/src/KinematicSourceType.cpp
2021-11-23T10:22:43.2473066Z In file included from /__w/robotology-superbuild/robotology-superbuild/src/whole-body-estimators/idl/wholeBodyDynamicsSettings/autogenerated/src/KinematicSourceType.cpp:13:0:
2021-11-23T10:22:43.2475629Z /__w/robotology-superbuild/robotology-superbuild/src/whole-body-estimators/idl/wholeBodyDynamicsSettings/autogenerated/include/KinematicSourceType.h:25:1: error: expected class-name before '{' token
2021-11-23T10:22:43.2476969Z  {
2021-11-23T10:22:43.2477250Z  ^
2021-11-23T10:22:43.2479534Z /__w/robotology-superbuild/robotology-superbuild/src/whole-body-estimators/idl/wholeBodyDynamicsSettings/autogenerated/include/KinematicSourceType.h:27:9: error: 'int KinematicSourceTypeVocab::fromString(const string&)' marked 'override', but does not override
2021-11-23T10:22:43.2481322Z      int fromString(const std::string& input) override;
2021-11-23T10:22:43.2481780Z          ^~~~~~~~~~
2021-11-23T10:22:43.2483985Z /__w/robotology-superbuild/robotology-superbuild/src/whole-body-estimators/idl/wholeBodyDynamicsSettings/autogenerated/include/KinematicSourceType.h:28:17: error: 'std::__cxx11::string KinematicSourceTypeVocab::toString(int) const' marked 'override', but does not override
2021-11-23T10:22:43.2485761Z      std::string toString(int input) const override;
2021-11-23T10:22:43.2486207Z                  ^~~~~~~~
2021-11-23T10:22:43.2487222Z [2/37] Building CXX object idl/wholeBodyDynamicsSettings/CMakeFiles/wholeBodyDynamicsSettings.dir/autogenerated/src/Gravity.cpp.o
2021-11-23T10:22:43.2488719Z [3/37] Building CXX object libraries/ctrlLibRT/CMakeFiles/ctrlLibRT.dir/src/filters.cpp.o

@HosameldinMohamed
Copy link
Collaborator

To understand the issue, before, the auto-generated files are pulled from the remote repo. So CMake with the variable CMAKE_CURRENT_SOURCE_DIR would find them then doesn't generate them if they are the same.

The problem could appear when the compiled YARP version and the YARP version in which the auto-generated files are different.

So the solution is to add the auto-generated files in the build directory to avoid such mismatch?

I guess it might happen also if the user compiles with one YARP version and then switches YARP version and tries to compile again? but at least it won't happen out-of-the-box!

I know I just rephrased what you said above! I am just making sure I understood it correctly!

@traversaro
Copy link
Member Author

I guess it might happen also if the user compiles with one YARP version and then switches YARP version and tries to compile again? but at least it won't happen out-of-the-box!

Yes, that is correct. However, if you switch some version of the dependencies and you do not cleanup the build, problems are kind of expected, so this should not be a big problem.

I know I just rephrased what you said above! I am just making sure I understood it correctly!

Yes, this is a good idea. I think your recap is correct.

Copy link
Collaborator

@HosameldinMohamed HosameldinMohamed left a comment

Choose a reason for hiding this comment

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

Thanks! I'll update the CHANGELOG with the fix and then merge it!

@traversaro
Copy link
Member Author

Thanks! I'll update the CHANGELOG with the fix and then merge it!

Good point, I forgot it.

@traversaro
Copy link
Member Author

@HosameldinMohamed if it is ok for you, we can merge to unblock robotology/robotology-superbuild#926, thanks!

@HosameldinMohamed HosameldinMohamed merged commit 7453ac4 into master Nov 23, 2021
@HosameldinMohamed HosameldinMohamed deleted the fixYARPmaster branch November 23, 2021 16:00
@HosameldinMohamed
Copy link
Collaborator

Sorry I waited few minutes for the CI after committing then I forgot to merge it!

@HosameldinMohamed
Copy link
Collaborator

@HosameldinMohamed if it is ok for you, we can merge to unblock robotology/robotology-superbuild#926, thanks!

you mean robotology/robotology-superbuild#900?

@traversaro
Copy link
Member Author

Sorry I waited few minutes for the CI after committing then I forgot to merge it!

Ah ok, no problem!

@traversaro
Copy link
Member Author

@HosameldinMohamed if it is ok for you, we can merge to unblock robotology/robotology-superbuild#926, thanks!

you mean robotology/robotology-superbuild#900?

robotology/robotology-superbuild#926 is the PR that should fix robotology/robotology-superbuild#900 .

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