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

Can't build with GCC < 11 (RHEL CI build, Ubuntu 20.04...) #949

Closed
roncapat opened this issue Feb 21, 2023 · 14 comments · Fixed by #1257
Closed

Can't build with GCC < 11 (RHEL CI build, Ubuntu 20.04...) #949

roncapat opened this issue Feb 21, 2023 · 14 comments · Fixed by #1257
Labels

Comments

@roncapat
Copy link

If you look at the humble branch, and for example this CI trace, there is an error in compiling hardware_interface because std::from_chars before GCC 11 only supports integral types (no double, as needed in the code instead).

@roncapat roncapat added the bug label Feb 21, 2023
@roncapat
Copy link
Author

This is due to this commit 78bb0ca

@roncapat
Copy link
Author

Doing "git revert 78bb0ca" solved my issue

@destogl
Copy link
Member

destogl commented Feb 21, 2023

The rolling doesn't support 20.04 anymore. The official rolling target is 22.04. So, we will not fix this since it is not a bug.

Is this correct that you are trying to do or I am missing something?

For the RHEL CI there are the multitude of issues – nevertheless we don't care much about them since this build is only to help us resolve some issues on the build farm.

@roncapat
Copy link
Author

Well, I was only talking about reverting that commit for the humble branch, not for the rolling branch. It's ok and well understood from my side that rolling branch should evolve without care of any older distro.

@bmagyar
Copy link
Member

bmagyar commented Mar 4, 2023

I think we could consider reverting that for Humble...

@roncapat
Copy link
Author

roncapat commented Mar 9, 2023

Thank you! I close this one then :)

@roncapat roncapat closed this as completed Mar 9, 2023
@github-project-automation github-project-automation bot moved this from Todo to Done in Bugs & Bugfixes Mar 9, 2023
@bmagyar
Copy link
Member

bmagyar commented Mar 9, 2023

I also pushed a release so it should be available with the next sync

@mory831
Copy link

mory831 commented Sep 30, 2023

Jetson nano (tegra) runs 20.04, which in my case uses hardware_interface::ActuatorInterface is required.

No way to upgrade to 22.04 without loosing GPU acceleration, out of 469 packages of iron-ros on arm64 the only one need to be changed is hardware_interface, just because GCC 9.4 lacks support of std::from_chars double version.

This insignificant function can be easily replaced with strtod, attached patches.

I can prepare pull request, using strtod or std::stod, let me know if this is welcomed

component_parser.diff.txt
generic_system.diff.txt

@olivier-stasse
Copy link
Contributor

Dear @mory831,
the foxy branch is for 20.04 and is not using from_chars.
You are probably using the main branch which is for rolling and hence assume by definition a more advanced GCC (see REP 2000). I suggest that you check your branch.

@mory831
Copy link

mory831 commented Oct 1, 2023

I do not understand your logic, The hardware_interface can run with minor changes (9 lines of code change, see the diffs) on 20.04 or any other system with older GCC.

All other iron packages listed at https://raw.githubusercontent.com/ros2/ros2/iron/ros2.repos and all iron ros_control packages are compiling without any problem on ARM64 GCC 9.4. The dissection to convert to std::from_chars, makes this package unavailable on a non main stream hardware (which some robot are made of) defeating one of the purpose of open source software, legacy support, nothing major is achieved using std::from_chars.

I am using iron AMD64 for development and iron ARM64 for deployment, while using Cuda on both hardware's, I do not want to look back on the EOL foxy, just because one package decide to use a function that does not exist on older GCCs. So I changed the code made it run and shared the code back for others that have older hardware / older compilers but want to use current iron/humble ROS distributions.

@olivier-stasse
Copy link
Contributor

olivier-stasse commented Oct 1, 2023

Dear @mory831 ,
They are at least two technical reasons to use std::from_chars instead of strtod as you suggest:

@traversaro made a point in robotology/idyntree#288 against std::from_chars because it seems to have an issue with trailing +.

In all cases the solution was to use something else than strtod, and at this stage it would be better to factor out what was chosen by the MoveIt team.

@traversaro
Copy link
Contributor

traversaro commented Oct 6, 2023

@traversaro made a point in robotology/idyntree#288 against std::from_chars because it seems to have an issue with trailing +.

Just to clarify: I am super-pro std::from_chars, especially since all the time I have been affected by locale-related issues in the past. Furthermore, any function that is affected by the global C is inherently not thread-safe, as any other thread could be modify the global local while you are using strtod. My comment there is just that if you are using std::from_chars to parse XML doubles, you need to proper account for + , but probably it is sufficient something like (warning, I did not actually test this):

std::from_chars_result from_chars_xml(const char* first,
                                      const char* last, double& value,
                                      std::chars_format fmt = std::chars_format::general)
{
    const char* new_first = first;
    if (first != nullptr && first != last)
    {
        if (*first == '+')
        {
            new_first++;
        }
    }
    return std::from_chars(new_first, last, value, fmt);
}

@francesco-romano was the one originally spotting this, so perhaps in the meanwhile he found other discrepancy between XML and C++.

@olivier-stasse
Copy link
Contributor

Just for sake of clarity I think that @mory831 and @paulm-planted in #921 (comment) do have a real good point for backportability with constrained hardware (even so another part of the problem is the lack of support from the companies providing the hardware) . But putting back strtod does not seems the way to go.

A simple solution would be to use locally the solution from MoveIt 2 with:
https://github.com/ros-planning/moveit2/blob/main/moveit_core/utils/include/moveit/utils/lexical_casts.h

It would not add too much to the code to maintain.

WDYT @bmagyar @destogl ?

@christophfroehlich
Copy link
Contributor

Due to the mentioned changes, ros2_control for Iron is not compatible to Debian Bullseye (11), but should according to REP-2000. Can we find a solution for that?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
Status: Done
Development

Successfully merging a pull request may close this issue.

7 participants