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

various improvements #275

Merged
merged 7 commits into from
Mar 24, 2023
Merged

various improvements #275

merged 7 commits into from
Mar 24, 2023

Conversation

v4hn
Copy link
Member

@v4hn v4hn commented Mar 6, 2023

Spring cleaning of local patches we collected on our robot over time.

@knorth55 @davefeilseifer Could you please provide feedback/review?

I believe this workaround probably fixes issues reported here as well (though that could have referred to many other problems too).

v4hn and others added 6 commits March 6, 2023 14:22
This is the most likely one to break because it runs the actual nodelets!
Respawning this one will automatically trigger the nodelet nodes to respawn as well
due to the nodelet-bond communication.
To avoid the item showing up as missing in the dashboard
…ge100 multi-configurator. It was harmless here, but if you copy it, you can get into trouble."

It's not harmless, it removed an error. wtf...

This reverts commit 81b70cf.
It's not worth raising an error just because the server
connection was not sufficiently quick once.
We see this on our machines every now and then.

If this is a permanent error condition it could escalate at some point
but that just adds additional complexity here.
A warning should alert you enough if it doesn't go away and you observe
unusual behavior.
@v4hn
Copy link
Member Author

v4hn commented Mar 6, 2023

The last patch resolves some annoying build farm warnings @k-okada and I receive every now and then. It should be applied to various other code bases here as well.

@knorth55
Copy link
Contributor

knorth55 commented Mar 6, 2023

We are running our PR2 on noetic now, and the issue caused by the change that LD_LIBRARY_PATH is disable by setcap.
https://stackoverflow.com/questions/9843178/linux-capabilities-setcap-seems-to-disable-ld-library-path

I also found the solution in this PR from here.
shadow-robot/ros_ethercat#76
But, I remember that the change does not solve the issue. (probably, I almost forget...)

So I added /etc/ld.so.conf.d/ros-noetic.conf as below, and it works now.

/opt/ros/noetic/lib

But I did the change last may, so I don't remember the detail...
When I have time, I will check it on our PR2 + Noetic.

@knorth55
Copy link
Contributor

knorth55 commented Mar 6, 2023

@v4hn

Indigo's support cmake version is 2.8.11, and many PR2 are running in indigo.
I think it is better to use cmake 2.8.

(I know the warning mail from build.ros.org is annoying... :(
I have the same issue because audio_common uses cmake 2.8.)

https://www.ros.org/reps/rep-0003.html#indigo-igloo-may-2014

@v4hn
Copy link
Member Author

v4hn commented Mar 6, 2023

Indigo's support cmake version is 2.8.11, and many PR2 are running in indigo. I think it is better to use cmake 2.8.

I would definitely argue that if somebody tries to use the kinetic-devel branch with ROS indigo, they are doing something very wrong. indigo was last released from the hydro-devel branch.

Don't you agree?

@v4hn
Copy link
Member Author

v4hn commented Mar 6, 2023

I also found the solution in this PR from here.
shadow-robot/ros_ethercat#76
But, I remember that the change does not solve the issue. (probably, I almost forget...)

I believe it did solve the problem for us, but it has been a while as well and I can't check the system right now (not in the lab for two weeks). Please verify again on your system and if it really fails I will drop the respective commit here.

So I added /etc/ld.so.conf.d/ros-noetic.conf as below, and it works now.

That works as well, though I'm not sure whether it still works when all/some involved packages are built in a local workspace (as the case for us).

@knorth55
Copy link
Contributor

knorth55 commented Mar 6, 2023

Don't you agree?

I agree, but indigo-devel is 62 commits behind from the kinetic-devel.
So we need back-porting PR.

Also I know that @k-okada 's policy is to maitain one branch, which works for every repo.
So I think it is better to ask @k-okada 's opinion.

@knorth55
Copy link
Contributor

knorth55 commented Mar 6, 2023

Please verify again on your system and if it really fails I will drop the respective commit here.

OK, I will check it!

- cmake drops support for <2.8.12 in the future
- CMP0048 is only available from 3.0, but complains if it is unset
  so explicitly set the policy if we compile on newer cmake (very
  likely).
@knorth55
Copy link
Contributor

knorth55 commented Mar 6, 2023

@v4hn
I checked in our PR2, and this PR works!
we don't need to change /etc/ld.so.conf.d/.
Probably, I was doing wrong change.

@v4hn
Copy link
Member Author

v4hn commented Mar 6, 2023

I agree, but indigo-devel is 62 commits behind from the kinetic-devel. So we need back-porting PR.

I agree, but this is free labor and I definitely don't feel responsible to keep current repositories running with 10-year-old distributions at the cost of annoyances for the maintainers.

Also I know that @k-okada 's policy is to maitain one branch, which works for every repo.
So I think it is better to ask @k-okada 's opinion.

Indeed, he set up travis for this repository with indigo as well. (Leaving aside that the travis configs are broken since github actions were introduced.)
I reworked the patch for now with more overhead.
We can work around the VERSION warning with

if(POLICY CMP0048)
  cmake_policy(SET CMP0048 NEW)
endif()

That still leaves the "cmake will not stay compatible with cmake <2.8.12 in the future" warning, that modern cmake versions report. ROS Indigo only required cmake 2.8.11 to support Ubuntu saucy (13.10). The only ROS indigo installations we might still consider build on Ubuntu trusty which bundles 2.8.12, so I bumped the version to 2.8.12 everywhere.

If you see @k-okada personally, please do talk to him about indigo vs (post-)noetic support.
I haven't seen him active on github issues in quite a while, though I expected he would join in with some discussions on ROS-O. He is active in the jsk repositories though, so I'm wasn't worried :) .

I checked in our PR2, and this PR works!
we don't need to change /etc/ld.so.conf.d/.
Probably, I was doing wrong change.

Good to hear 👍

@knorth55
Copy link
Contributor

knorth55 commented Mar 6, 2023

this is free labor and I definitely don't feel responsible to keep current repositories running with 10-year-old distributions at the cost of annoyances for the maintainers.

I agree.

I reworked the patch for now with more overhead.

@v4hn thank you !

If you see @k-okada personally, please do talk to him about indigo vs (post-)noetic support.
I haven't seen him active on github issues in quite a while, though I expected he would join in with some discussions on ROS-O. He is active in the jsk repositories though, so I'm wasn't worried :) .

I will talk to him.
He may be interested in ROS-O as jsk-ros-pkg/jsk_common#1770 (comment) .
We are also interested in ROS-O, because transition to ROS2 costs a lot.
If there is a doc to build deb for ROS-O + Ubuntu 22.04, I want to read the doc and set up the build and deb server.

@v4hn v4hn merged commit 80369f9 into PR2:kinetic-devel Mar 24, 2023
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