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

remove dependency on linux-headers-generic #100

Merged
merged 1 commit into from Nov 7, 2019
Merged

remove dependency on linux-headers-generic #100

merged 1 commit into from Nov 7, 2019

Conversation

christian-rauch
Copy link
Contributor

@christian-rauch christian-rauch commented Sep 18, 2019

The rosdep key linux-headers-generic will always install the "standard" kernel headers. On Ubuntu LTS, where kernels are updated with point releases, this will depend on the old kernel header files, while a newer kernel image might be booted.

Since the libpcan build script depends on the headers of the currently booted kernel anyway, installing the "old" headers does not have an effect.

rosdep key 'linux-headers-generic' will always install the "standard" kernel headers. On Ubuntu LTS, where kernels are updated with point
releases, this will depend on the old kernel header files, while a newer kernel image might be booted.
@christian-rauch
Copy link
Contributor Author

@fmessmer Can you have a look?

@fmessmer
Copy link
Member

seems reasonable

also, open-close to get the expectec travis jobs to start - needed after #101

@fmessmer fmessmer closed this Oct 31, 2019
@fmessmer fmessmer reopened this Oct 31, 2019
@fmessmer
Copy link
Member

ref ros/rosdistro#21921 (comment)

it will take a bit longer until I find the time to get this done...

@christian-rauch
Copy link
Contributor Author

@fmessmer I am not sure if I get this. The issue you linked is related to the rosdep key linux-headers-generic that cannot be resolved to the most recent Debian kernel header package. This is somehow the same issue for Ubuntu, where the rosdep key will refer to a fixed package, but not the most recent kernel that might be booted.

Shouldn't this go away once the rosdep key is removed by this PR?

@fmessmer
Copy link
Member

yes...the referenced issue was rather an additional argument for merging this pr 😉

@fmessmer
Copy link
Member

fmessmer commented Nov 5, 2019

@clalancette
do you think merging this would help with ros/rosdistro#21921 (comment)

@clalancette
Copy link

@fmessmer @christian-rauch So I took a closer look at this PR.

The question to answer here is whether building libpcan really requires anything out of the linux-headers-generic package. The way to look at that is to look at the source code that comprises libpcan, and see if there are any specific headers that would be required.

If we look at CMakeLists.txt, we see that make is invoked only in the lib subdirectory. Looking further in there, we see that the lib subdirectory also adds the driver subdirectory as a location for includes. So those are the two directories we need to look in to find whether anything requires the linux headers:

grep -rI "include" lib/* > /tmp/includes
grep -I "include" driver/*.h >> /tmp/includes
cat /tmp/includes | cut -d':' -f2 | sort -u

The list I got from the above looks like:

#include <errno.h>
#include <fcntl.h>
#include "libpcan.c"
#include "libpcanfd.h"
#include <libpcan.h>
#include <linux/ioctl.h>
#include <linux/types.h>
#include <pcanfd.h>
#include "pcan.h"
#include <rtdm/rtdm.h>
#include "src/libprivate.h"
#include <stdarg.h>
#include <stdio.h>
#include <stdlib.h>
#include <string.h>
#include <sys/ioctl.h>
#include <unistd.h>

As far as I can tell, those are all standard headers (even the linux/* ones are from the linux-libc-dev package). Thus, there is no reason for this package to depend on linux-headers-generic. I'll approve this.

@christian-rauch
Copy link
Contributor Author

libpcan does not directly depend on the Linux headers, but the peak-linux-driver does:

SOURCE_DIR=build/peak-linux-driver-8.3
KERNEL_VER=/usr/src/linux-headers-"`uname -r`"
# apply patch for current Linux kernel versions
#patch -p0 < patch.diff
cd $SOURCE_DIR && make install KERNEL_LOCATION=$KERNEL_VER DBG=NO_DEBUG MOD=MODVERSIONS PAR=PARPORT_SUBSYSTEM USB=USB_SUPPORT PCI=PCI_SUPPORT DNG=DONGLE_SUPPORT ISA=NO PCC=NO NET=NO RT=NO

But the version of the kernel headers is determined by the currently loaded kernel (uname -r). That is why I argue that the rosdep key cannot assure that this header version is installed. But Linux headers are still required to build the driver.

Or am I missing something here?

@christian-rauch
Copy link
Contributor Author

Addendum:
The CMakeLists.txt only compiles the userspace library libpcan.so. The kernel module is compiled via install_pcan.sh. The Makefile will determine the running kernel automatically, i.e. make can be called without providing KERNEL_LOCATION.
However, the sources for the kernel module seem to be outdated. The script fails with: error: implicit declaration of function ‘do_gettimeofday’; did you mean ‘do_settimeofday64’? and running make KERNEL_LOCATION=$KERNEL_VER DBG=NO_DEBUG MOD=MODVERSIONS PAR=PARPORT_SUBSYSTEM USB=USB_SUPPORT PCI=PCI_SUPPORT DNG=DONGLE_SUPPORT ISA=NO PCC=NO NET=NO RT=NO manually from the root gives me yet another error: fatal error: rtdm/rtdm.h: No such file or directory

@fmessmer What is the status of this script? Is it still required and if so, what would be the correct way to build&install the kernel module?

@fmessmer
Copy link
Member

fmessmer commented Nov 7, 2019

to be honest, I don't have all the insights about what's going on...and I'm only doing releases every now and then...I'm not officially maintaining this repo anymore...

however, it seems to me that merging this is a good idea...thus, merging... 😉

@fmessmer fmessmer merged commit ffa64da into ipa320:indigo_dev Nov 7, 2019
@christian-rauch christian-rauch deleted the rm_dep_header branch November 7, 2019 11:15
@clalancette
Copy link

The kernel module is compiled via install_pcan.sh. The Makefile will determine the running kernel automatically, i.e. make can be called without providing KERNEL_LOCATION.

While that is true, it looks like install_pcan.sh is never called from the CMakeLists.txt, and thus is never needed in the package. It looks like some kind of "helper" script for those who are building from source, which is why the package.xml doesn't really need to reflect a dependency for it. In any case, I'm satisfied with this merge, thanks!

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.

None yet

3 participants