-
Notifications
You must be signed in to change notification settings - Fork 1
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
Baseimage upgrade to support multi-arch builds #62
Conversation
I am a little bit surprised about this because I thought that we need more work on the Yocto side. |
Yes. But there is still a small issue from what I tested locally. When the container starts, it calls for the "/etc/init.d/udev start". And it made the host system to lose connection to some usb devices such as mouse. When mouse unplugged and plugged again, it works ok. We need to check if it creates a similar problem with the devices, in such a way that it interrupts the rplidar. |
6b8acb4
to
6343d61
Compare
@@ -49,7 +49,7 @@ fi | |||
if [ "${USE_AUTO_FOCUS}" = "1" ]; then | |||
ROS_FLAGS="${ROS_FLAGS} use_auto_focus:=true" | |||
fi | |||
|
|||
/etc/init.d/udev start |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
if an entrypoint starts udev inside of a container (which is highly unusual) the why needs to be commented
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I was trying to research the "why"
All I find is this single udev rule file: https://github.com/tiiuae/depthai_ctrl/blob/fdfeb3af76e8407904719115c945619ba9937a13/80-movidius.rules
All it does is assign 0666
permissions to the device file. Which shouldn't be needed when this is run under --privileged
anyway, and anyway for the file permission issue there is probably some more lightweight way to solve it than udev.
And when you run the container, that rule file doesn't seem to be included:
$ ls -al /etc/udev/rules.d
total 156
drwxr-xr-x 2 root root 4096 Jul 4 11:24 .
drwxr-xr-x 3 root root 4096 Jul 4 11:24 ..
-rw-r--r-- 1 root root 144815 Feb 21 2021 40-libgphoto2.rules
-rw-r--r-- 1 root root 0 Feb 9 2021 80-net-name-slot.rules
-rw-r--r-- 1 root root 885 Feb 9 2021 local.rules
Can you help me find out, which problem does the udev solve? I.e. what doesn't work if the udev start
line is removed.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
COPY --from=builder /main_ws/ros-*-depthai-ctrl_*_amd64.deb /depthai.deb | ||
# The multi-arch build has the unnecessary line "/etc/init.d/udev start" for udev rules. | ||
# Not needed in ubuntu OS baseimage | ||
RUN sed -i '/\/etc\/init.d\/udev start/d' /entrypoint.sh |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The entrypoint file contains "udev start" but then the sed command deletes that line?
Can't the "udev start" be removed from the container then?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Dockerfile.ros
is an legacy format of building. We don't use that for multiarch, it is left there to have the reference and have ability to run legacy format. That's why it is removing it, because it doesn't require the udev start command.
However, the Dockerfile does not remove the sed line as it needs it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh sorry I didn't realize this was not used.
I get the idea of leaving it as a reference.
I'm worried about maintenance: by leaving it here we are either:
a) maintaining something that will not be used.
b) (more likely) leaving this here to rot and then people getting confused about mismatches in Dockerfile
vs Dockerfile.ros
- "why is this change/concept in file A but not in B?"
c) risking people looking at the wrong file (happened to my stupid ass now :) )
if this file is for reference purposes, can't we at top of Dockerfile
leave a comment like this:
# this file before multi-arch work looked like this:
# https://github.com/tiiuae/depthai_ctrl/blob/d51b9765e7f017bd062f872d8bc53ed7eb5a1b78/Dockerfile.ros
b458e80
to
5948ceb
Compare
5948ceb
to
4fc010f
Compare
This PR brings the functionality to build component in amd64, arm64, and riscv64 architectures.
For further information about the baseimage related changes, see fog-ros-baseimage/docs
/Multiarchitecture_builds.md
Merge of this PR will be done by the fog-ros-baseimage maintainers.