Message ID | 1407956022-62199-1-git-send-email-Matthew.Weber@rockwellcollins.com |
---|---|
State | Superseded |
Headers | show |
Dear Matt Weber, On Wed, 13 Aug 2014 13:53:42 -0500, Matt Weber wrote: > The fio package at runtime (dlopen)links in the libaio library > when the ioengine is selected as libaio. I.e. this isn't caught at > build time since the link wouldn't occur. > > Signed-off-by: Matt Weber <Matthew.Weber@rockwellcollins.com> Thanks for the explanation. However, this patch still isn't completely correct. The first issue is that you have added a changelog information ("added description") in the commit title, which is preserved forever in the Git history. The changelog information should go... > --- ... here, so that it doesn't get preserved forever in the Git history. Also, the (bugfix) part of your commit title is kind of strange. I think a proper commit title would be: fio: add missing libaio dependency Also, you say that libaio is needed when the ioengine is selected as libaio. Are there other ioengine that could potentially be used, and therefore make the libaio dependency optional? > package/fio/Config.in | 1 + > package/fio/fio.mk | 1 + > 2 files changed, 2 insertions(+) > > diff --git a/package/fio/Config.in b/package/fio/Config.in > index 8cbbf6c..b907fb9 100644 > --- a/package/fio/Config.in > +++ b/package/fio/Config.in > @@ -3,6 +3,7 @@ config BR2_PACKAGE_FIO > depends on BR2_USE_MMU # fork() > depends on BR2_LARGEFILE > depends on BR2_TOOLCHAIN_HAS_THREADS > + select BR2_PACKAGE_LIBAIO #ioengine libaio This should be: # Runtime dependency: libaio is dlopen()ed by fio select BR2_PACKAGE_LIBAIO Also, the libaio package is only available for a limited set of architectures (see package/libaio/Config.in), and those dependencies must be propagated to fio if libaio is indeed a mandatory dependency of fio. > # fio uses posix_madvise(), which is not part of any official > # release of uClibc, but is part of uClibc Git, and backported > # in Buildroot patch set of uClibc 0.9.33. Therefore, we > diff --git a/package/fio/fio.mk b/package/fio/fio.mk > index f9a690e..e0c79e8 100644 > --- a/package/fio/fio.mk > +++ b/package/fio/fio.mk > @@ -8,6 +8,7 @@ FIO_VERSION = fio-2.1.4 > FIO_SITE = git://git.kernel.dk/fio.git > FIO_LICENSE = GPLv2 + special obligations > FIO_LICENSE_FILES = LICENSE > +FIO_DEPENDENCIES = libaio If libaio is only a runtime dependency of fio (and not a build time dependency), then this line is not needed. Thanks, Thomas
Thomas, On Wed, Aug 13, 2014 at 4:59 PM, Thomas Petazzoni <thomas.petazzoni@free-electrons.com> wrote: > Dear Matt Weber, > > On Wed, 13 Aug 2014 13:53:42 -0500, Matt Weber wrote: >> The fio package at runtime (dlopen)links in the libaio library >> when the ioengine is selected as libaio. I.e. this isn't caught at >> build time since the link wouldn't occur. >> >> Signed-off-by: Matt Weber <Matthew.Weber@rockwellcollins.com> > > Thanks for the explanation. However, this patch still isn't completely > correct. The first issue is that you have added a changelog information > ("added description") in the commit title, which is preserved forever > in the Git history. The changelog information should go... > >> --- > > ... here, so that it doesn't get preserved forever in the Git history. > > Also, the (bugfix) part of your commit title is kind of strange. I > think a proper commit title would be: > > fio: add missing libaio dependency Agree, I didn't scrub my commit before generating the path. Sorry about that I'll cleanup and submit v2. > > Also, you say that libaio is needed when the ioengine is selected as > libaio. Are there other ioengine that could potentially be used, and > therefore make the libaio dependency optional? The code only has this one dlopen case. Everything else is linked at compile time. > >> package/fio/Config.in | 1 + >> package/fio/fio.mk | 1 + >> 2 files changed, 2 insertions(+) >> >> diff --git a/package/fio/Config.in b/package/fio/Config.in >> index 8cbbf6c..b907fb9 100644 >> --- a/package/fio/Config.in >> +++ b/package/fio/Config.in >> @@ -3,6 +3,7 @@ config BR2_PACKAGE_FIO >> depends on BR2_USE_MMU # fork() >> depends on BR2_LARGEFILE >> depends on BR2_TOOLCHAIN_HAS_THREADS >> + select BR2_PACKAGE_LIBAIO #ioengine libaio > > This should be: > > # Runtime dependency: libaio is dlopen()ed by fio > select BR2_PACKAGE_LIBAIO Agree, will update. > > Also, the libaio package is only available for a limited set of > architectures (see package/libaio/Config.in), and those dependencies > must be propagated to fio if libaio is indeed a mandatory dependency of > fio. The code doesn't (currently) seem to make the libaio optional. So I agree at this point it would make the most sense to restrict fio to the same build targets as libaio and enforce this dependency. > >> # fio uses posix_madvise(), which is not part of any official >> # release of uClibc, but is part of uClibc Git, and backported >> # in Buildroot patch set of uClibc 0.9.33. Therefore, we <snip> >> FIO_LICENSE_FILES = LICENSE >> +FIO_DEPENDENCIES = libaio > > If libaio is only a runtime dependency of fio (and not a build time > dependency), then this line is not needed. True, we would have see build failures if this was required. Thank you for the feedback,
diff --git a/package/fio/Config.in b/package/fio/Config.in index 8cbbf6c..b907fb9 100644 --- a/package/fio/Config.in +++ b/package/fio/Config.in @@ -3,6 +3,7 @@ config BR2_PACKAGE_FIO depends on BR2_USE_MMU # fork() depends on BR2_LARGEFILE depends on BR2_TOOLCHAIN_HAS_THREADS + select BR2_PACKAGE_LIBAIO #ioengine libaio # fio uses posix_madvise(), which is not part of any official # release of uClibc, but is part of uClibc Git, and backported # in Buildroot patch set of uClibc 0.9.33. Therefore, we diff --git a/package/fio/fio.mk b/package/fio/fio.mk index f9a690e..e0c79e8 100644 --- a/package/fio/fio.mk +++ b/package/fio/fio.mk @@ -8,6 +8,7 @@ FIO_VERSION = fio-2.1.4 FIO_SITE = git://git.kernel.dk/fio.git FIO_LICENSE = GPLv2 + special obligations FIO_LICENSE_FILES = LICENSE +FIO_DEPENDENCIES = libaio define FIO_CONFIGURE_CMDS (cd $(@D); ./configure --cc="$(TARGET_CC)" --extra-cflags="$(TARGET_CFLAGS)")
The fio package at runtime (dlopen)links in the libaio library when the ioengine is selected as libaio. I.e. this isn't caught at build time since the link wouldn't occur. Signed-off-by: Matt Weber <Matthew.Weber@rockwellcollins.com> --- package/fio/Config.in | 1 + package/fio/fio.mk | 1 + 2 files changed, 2 insertions(+)