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.

Sign up for GitHub

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

Jump to bottom

Regression in yarp_add_idl behaviour in YARP 3.3.0 w.r.t 3.2 #2142

Closed
traversaro opened this issue Dec 5, 2019 · 11 comments
Closed

Regression in yarp_add_idl behaviour in YARP 3.3.0 w.r.t 3.2 #2142

traversaro opened this issue Dec 5, 2019 · 11 comments
Labels
Affects: YARP v3.3.0 This is a known issue affecting YARP v3.3.0 Component: IDL Resolution: Wontfix Type: Regression This is related to something that worked in a previous version

Comments

@traversaro
Copy link
Member

traversaro commented Dec 5, 2019 •
edited
Loading

Describe the bug
A clear and concise description of what the bug is.

To Reproduce
Clone the repo https://github.com/robotology/wearables (that depends just on YARP), configure and build it.
The compilation fails with:

2019-12-04T11:55:58.5536237Z FAILED: wrappers/IXsensMVNControl/CMakeFiles/IXsensMVNControlWrapper.dir/src/IXsensMVNControlWrapper.cpp.o 
2019-12-04T11:55:58.5537590Z /usr/bin/c++  -DIXsensMVNControlWrapper_EXPORTS -DUSING_DEPRECATED_UPPERCASE_YARP_OS_TARGET -I/home/runner/work/robotology-superbuild/robotology-superbuild/robotology/wearables/wrappers/IXsensMVNControl/include -Imsgs -I/home/runner/work/robotology-superbuild/robotology-superbuild/robotology/wearables/interfaces/IXsensMVNControl -isystem /home/runner/work/robotology-superbuild/robotology-superbuild/build/install/include -g -fPIC   -std=gnu++14 -MD -MT wrappers/IXsensMVNControl/CMakeFiles/IXsensMVNControlWrapper.dir/src/IXsensMVNControlWrapper.cpp.o -MF wrappers/IXsensMVNControl/CMakeFiles/IXsensMVNControlWrapper.dir/src/IXsensMVNControlWrapper.cpp.o.d -o wrappers/IXsensMVNControl/CMakeFiles/IXsensMVNControlWrapper.dir/src/IXsensMVNControlWrapper.cpp.o -c /home/runner/work/robotology-superbuild/robotology-superbuild/robotology/wearables/wrappers/IXsensMVNControl/src/IXsensMVNControlWrapper.cpp
2019-12-04T11:55:58.5538482Z /home/runner/work/robotology-superbuild/robotology-superbuild/robotology/wearables/wrappers/IXsensMVNControl/src/IXsensMVNControlWrapper.cpp:12:10: fatal error: XsensSuitControlService.h: No such file or directory
2019-12-04T11:55:58.5538999Z  #include "XsensSuitControlService.h"
2019-12-04T11:55:58.5539270Z           ^~~~~~~~~~~~~~~~~~~~~~~~~~~
2019-12-04T11:55:58.5539532Z compilation terminated.
2019-12-04T11:55:58.5539895Z [50/53] Building CXX object wrappers/IXsensMVNControl/CMakeFiles/IXsensMVNControlWrapper.dir/yarp_plugin_ixsensmvncontrol_wrapper.cpp.o
2019-12-04T11:55:58.5540234Z [51/53] Building CXX object wrappers/IWear/CMakeFiles/IWearWrapper.dir/src/IWearWrapper.cpp.o
2019-12-04T11:55:58.5540510Z ninja: build stopped: subcommand failed.

Expected behavior
The compilation should be succssful.

Screenshots
If applicable, add screenshots to help explain your problem.

Configuration (please complete the following information):

  • OS: Ubuntu 18.04
  • yarp version: 3.3.0
  • compiler: gcc 7.4.0

Additional context
This is the same content of #2118 (comment), I just opened a new issue to have a better visibility.
In https://github.com/robotology/robotology-superbuild/runs/332925188 you can find an example of failed CI run.
@traversaro
Copy link
Member Author

traversaro commented Dec 5, 2019 •
edited
Loading

Interestingly, the project compiles fine for the development 3.2.10* YARP versions before PR #2136 , thanks to the workaround added in https://github.com/robotology/wearables/pull/65/files .

@traversaro traversaro mentioned this issue Dec 5, 2019
Closed
@drdanz
Copy link
Member

drdanz commented Dec 5, 2019

Is it using yarp_idl_to_dir or yarp_add_idl?

I reverted some changes to yarp_add_idl that shouldn't have been there in the first place, to make it compatible with YARP 3.2, but I don't think anything changed in the yarp_idl_to_dir function

See also this comment
#2118 (comment)

@traversaro
Copy link
Member Author

Is it using yarp_idl_to_dir or yarp_add_idl?

Sorry, it is using yarp_add_idl (see https://github.com/robotology/wearables/blob/aa991c502895e7c954e302839f992bfdbcdb7325/msgs/thrift/CMakeLists.txt#L53), I used the wrong function in the issue title, thanks for noticing it.

@traversaro traversaro changed the title Regression in yarp_idl_to_dir behaviour in YARP 3.3.0 w.r.t 3.2 Regression in yarp_add_idl behaviour in YARP 3.3.0 w.r.t 3.2 Dec 5, 2019
@drdanz
Copy link
Member

drdanz commented Dec 5, 2019

I suggest to revert robotology/wearables#65
At some point I think that yarp_add_idl should use the namespace instead of the path, but when it will happen I'll try to add some deprecation method and/or some warning

@Nicogene Nicogene mentioned this issue Dec 5, 2019
Closed
@traversaro
Copy link
Member Author

I suggest to revert robotology/wearables#65
At some point I think that yarp_add_idl should use the namespace instead of the path, but when it will happen I'll try to add some deprecation method and/or some warning

Ok, I guess we can try, but just to clarify: the current master branch of wearables (after the changes in robotology/wearables#65) compiles fine with YARP 3.2 .

@drdanz
Copy link
Member

drdanz commented Dec 6, 2019

But did it compile with YARP 3.2 without those changes?

@traversaro
Copy link
Member Author

But did it compile with YARP 3.2 without those changes?

Yes.

@lrapetti lrapetti mentioned this issue Dec 16, 2019
Merged
@lrapetti
Copy link
Member

I suggest to revert robotology/wearables#65
At some point I think that yarp_add_idl should use the namespace instead of the path, but when it will happen I'll try to add some deprecation method and/or some warning

reverting the PR seems to fix the compilation problem (robotology/wearables#71).

@traversaro
Copy link
Member Author

I do not know if #1092 is related.

traversaro added a commit to robotology/idyntree that referenced this issue Jan 13, 2020
@traversaro
Remove thrift from the include path of thrift-generated files
5801e38 
It is not clear why this is necessary, see: 
* robotology/yarp#1092 
* robotology/yarp#2142

However, the new style (without the prepended "thrift") seems to be working fine on both YARP 3.3 and master, 
so given that iDynTree 1.0 now requires YARP 3.3 as the minimum version, I think it is ok to just fix the issue by 
removing the "thrift" directory.
@S-Dafarra S-Dafarra mentioned this issue Jan 13, 2020
Merged
@drdanz drdanz added the Type: Regression This is related to something that worked in a previous version label Jul 14, 2020
@drdanz
Copy link
Member

drdanz commented Jul 14, 2020

@traversaro @lrapetti sorry, I don't remember if we decided something regarding this issue... Did we decide to fix it or leave it like it is now?

@drdanz drdanz added Component: IDL Affects: YARP v3.3.0 This is a known issue affecting YARP v3.3.0 labels Jul 14, 2020
@traversaro
Copy link
Member Author

traversaro commented Jul 14, 2020 •
edited
Loading

@traversaro @lrapetti sorry, I don't remember if we decided something regarding this issue... Did we decide to fix it or leave it like it is now?

I think that the regression (i.e. the change of behaviour) is indeed there, but we already fixed our downstream projects and it is related to a corner case, so I think we can simply close the issue.

@drdanz drdanz closed this as completed Jul 14, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
Affects: YARP v3.3.0 This is a known issue affecting YARP v3.3.0 Component: IDL Resolution: Wontfix Type: Regression This is related to something that worked in a previous version
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
3 participants
@drdanz @traversaro @lrapetti