| Message ID | 20171213004519.29340-5-mcgrof@kernel.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Tue, Dec 12, 2017 at 04:45:14PM -0800, Luis R. Rodriguez wrote: > Modern gdbm-devel packages bundle together gdbm.h and ndbm.h. > The old m4 macro had detection support for some old gdbm libraries > but not for new ones. > > We fix compilation of src/dbtest.c by making the autoconf helper > check for this new arrangement: > > If both gdbm.h and gdbm.h are found define set both gdbm_ndbm_=true, ^^^^^^ ndbm.h? > and have_db=true, and define HAVE_GDBM_H. The src/dbtest.c already > had a HAVE_GDBM_H but there was never a respective autoconf settter for > it. We can just re-use this and fix it for new arrangement. > > Signed-off-by: Luis R. Rodriguez <mcgrof@kernel.org> This looks fine to me. The only system I have by hand that have both <gdbm.h> and <ndbm.h> but not any <gdbm/[gn]dbm.h> is openSUSE Tumbleweed. Without this patch, dbtest was not built on openSUSE, and was built successfully with this patch applied. And dbtest is still built on RHEL6/7 and Fedora. BTW, I'll queue patch 3 and this patch for next fstests release, while other patches seem not necessary, I agreed with Dave that groups are not for excluding tests, the required tools and environments should be detected by tests and _notrun if not met. (The README change looks fine, but it doesn't apply due to the "fsgqa-381" change, so I drop it too for now.) Thanks, Eryu -- To unsubscribe from this list: send the line "unsubscribe fstests" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Thu, Dec 14, 2017 at 01:51:02PM +0800, Eryu Guan wrote: > On Tue, Dec 12, 2017 at 04:45:14PM -0800, Luis R. Rodriguez wrote: > > Modern gdbm-devel packages bundle together gdbm.h and ndbm.h. > > The old m4 macro had detection support for some old gdbm libraries > > but not for new ones. > > > > We fix compilation of src/dbtest.c by making the autoconf helper > > check for this new arrangement: > > > > If both gdbm.h and gdbm.h are found define set both gdbm_ndbm_=true, > ^^^^^^ ndbm.h? > > and have_db=true, and define HAVE_GDBM_H. The src/dbtest.c already > > had a HAVE_GDBM_H but there was never a respective autoconf settter for > > it. We can just re-use this and fix it for new arrangement. > > > > Signed-off-by: Luis R. Rodriguez <mcgrof@kernel.org> > > This looks fine to me. > > The only system I have by hand that have both <gdbm.h> and <ndbm.h> but > not any <gdbm/[gn]dbm.h> is openSUSE Tumbleweed. Indeed, openSUSE and SLE releases. > Without this patch, > dbtest was not built on openSUSE, and was built successfully with this > patch applied. Yeap. > And dbtest is still built on RHEL6/7 and Fedora. Feel free to modify the commit log accordingly then. Curious, what packages does Fedora/ RHEL6/7 use for the requirement here? We just have one: $ rpm -ql gdbm-devel-1.12-1.282.x86_64 /usr/bin/gdbm_dump /usr/bin/gdbm_load /usr/bin/gdbmtool /usr/include/dbm.h /usr/include/gdbm.h /usr/include/ndbm.h /usr/lib64/libgdbm.a /usr/lib64/libgdbm.so /usr/lib64/libgdbm_compat.a /usr/lib64/libgdbm_compat.so /usr/lib64/libndbm.a /usr/lib64/libndbm.so /usr/share/info/gdbm.info.gz /usr/share/man/man1/gdbm_dump.1.gz /usr/share/man/man1/gdbm_load.1.gz /usr/share/man/man1/gdbmtool.1.gz /usr/share/man/man3/gdbm.3.gz > BTW, I'll queue patch 3 and this patch for next fstests release, while > other patches seem not necessary, I think patch 2 is fine too. > I agreed with Dave that groups are not > for excluding tests, the required tools and environments should be > detected by tests and _notrun if not met. Yeah makes sense now. I think we should also document when adding a group makes sense as well. > (The README change looks fine, > but it doesn't apply due to the "fsgqa-381" change, so I drop it too for > now.) Feel free to modify it, its not a big deal. Luis -- To unsubscribe from this list: send the line "unsubscribe fstests" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Thu, Dec 14, 2017 at 06:55:03PM +0100, Luis R. Rodriguez wrote: > On Thu, Dec 14, 2017 at 01:51:02PM +0800, Eryu Guan wrote: > > On Tue, Dec 12, 2017 at 04:45:14PM -0800, Luis R. Rodriguez wrote: > > > Modern gdbm-devel packages bundle together gdbm.h and ndbm.h. > > > The old m4 macro had detection support for some old gdbm libraries > > > but not for new ones. > > > > > > We fix compilation of src/dbtest.c by making the autoconf helper > > > check for this new arrangement: > > > > > > If both gdbm.h and gdbm.h are found define set both gdbm_ndbm_=true, > > ^^^^^^ ndbm.h? > > > and have_db=true, and define HAVE_GDBM_H. The src/dbtest.c already > > > had a HAVE_GDBM_H but there was never a respective autoconf settter for > > > it. We can just re-use this and fix it for new arrangement. > > > > > > Signed-off-by: Luis R. Rodriguez <mcgrof@kernel.org> > > > > This looks fine to me. > > > > The only system I have by hand that have both <gdbm.h> and <ndbm.h> but > > not any <gdbm/[gn]dbm.h> is openSUSE Tumbleweed. > > Indeed, openSUSE and SLE releases. > > > Without this patch, > > dbtest was not built on openSUSE, and was built successfully with this > > patch applied. > > Yeap. > > > And dbtest is still built on RHEL6/7 and Fedora. > > Feel free to modify the commit log accordingly then. Curious, what packages > does Fedora/ RHEL6/7 use for the requirement here? > > We just have one: > > $ rpm -ql gdbm-devel-1.12-1.282.x86_64 > /usr/bin/gdbm_dump > /usr/bin/gdbm_load > /usr/bin/gdbmtool > /usr/include/dbm.h > /usr/include/gdbm.h > /usr/include/ndbm.h > /usr/lib64/libgdbm.a > /usr/lib64/libgdbm.so > /usr/lib64/libgdbm_compat.a > /usr/lib64/libgdbm_compat.so > /usr/lib64/libndbm.a > /usr/lib64/libndbm.so > /usr/share/info/gdbm.info.gz > /usr/share/man/man1/gdbm_dump.1.gz > /usr/share/man/man1/gdbm_load.1.gz > /usr/share/man/man1/gdbmtool.1.gz > /usr/share/man/man3/gdbm.3.gz gdbm-devel too, but it has gdbm/[gn]dbm.h pointing to ../[gn]dbm.h, so there's no such problem and dbtest is building normally. # rpm -ql gdbm-devel /usr/include/dbm.h /usr/include/gdbm /usr/include/gdbm.h /usr/include/gdbm/dbm.h /usr/include/gdbm/gdbm.h /usr/include/gdbm/ndbm.h /usr/include/ndbm.h /usr/lib64/libgdbm.so /usr/lib64/libgdbm_compat.so /usr/share/info/gdbm.info.gz /usr/share/man/man3/gdbm.3.gz > > > BTW, I'll queue patch 3 and this patch for next fstests release, while > > other patches seem not necessary, > > I think patch 2 is fine too. > > > I agreed with Dave that groups are not > > for excluding tests, the required tools and environments should be > > detected by tests and _notrun if not met. > > Yeah makes sense now. I think we should also document when adding > a group makes sense as well. > > > (The README change looks fine, > > but it doesn't apply due to the "fsgqa-381" change, so I drop it too for > > now.) > > Feel free to modify it, its not a big deal. OK, I'll modify on commit, thanks! Thanks, Eryu -- To unsubscribe from this list: send the line "unsubscribe fstests" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 12/14/17 12:51 AM, Eryu Guan wrote: > On Tue, Dec 12, 2017 at 04:45:14PM -0800, Luis R. Rodriguez wrote: >> Modern gdbm-devel packages bundle together gdbm.h and ndbm.h. >> The old m4 macro had detection support for some old gdbm libraries >> but not for new ones. >> >> We fix compilation of src/dbtest.c by making the autoconf helper >> check for this new arrangement: >> >> If both gdbm.h and gdbm.h are found define set both gdbm_ndbm_=true, > ^^^^^^ ndbm.h? >> and have_db=true, and define HAVE_GDBM_H. The src/dbtest.c already >> had a HAVE_GDBM_H but there was never a respective autoconf settter for >> it. We can just re-use this and fix it for new arrangement. >> >> Signed-off-by: Luis R. Rodriguez <mcgrof@kernel.org> > > This looks fine to me. > > The only system I have by hand that have both <gdbm.h> and <ndbm.h> but > not any <gdbm/[gn]dbm.h> is openSUSE Tumbleweed. Without this patch, > dbtest was not built on openSUSE, and was built successfully with this > patch applied. And dbtest is still built on RHEL6/7 and Fedora. > > BTW, I'll queue patch 3 and this patch for next fstests release, while > other patches seem not necessary, I agreed with Dave that groups are not > for excluding tests, the required tools and environments should be > detected by tests and _notrun if not met. (The README change looks fine, > but it doesn't apply due to the "fsgqa-381" change, so I drop it too for > now.) Hi guys - This change breaks on older releases like SLES 11 where both <ndbm.h> and <gdbm.h> define datum, so we get build failures. The failure is new, but not because it used to pass and now doesn't. It's apparently never built on SLES releases since we ship /usr/include/ndbm.h and then we notrun the test that uses. Now that we're looking for gdbm.h and find it, we attempt to build src/dbtest and fail. This fix isn't the right solution. The problem is that we have a couple layers of old cruft that needs to be cleaned out. 1) As Luis notes, nothing sets HAVE_GDBM_H. The thing is that there is no version of gdbm.h that exports the NDBM interface. Further, looking at the git history, nothing has ever set HAVE_GDBM_H. It was dead code when it was committed initially as best I can tell. 2) openSUSE Tumbleweed doesn't need <gdbm.h> at all. It needs <ndbm.h> and this fix works because Luis added it to the HAVE_GDBM_H stanza. 3) AC_PACKAGE_WANT_NDBM used to check for <ndbm.h> but it was a check for IRIX and the caller was removed ages ago. It wouldn't matter if it were called anyway since libndbm is an IRIX library. Linux, IIRC, has never shipped a libndbm. I'll post a few patches following this to clean it up and get it working on SLES11. -Jeff
diff --git a/m4/package_gdbmdev.m4 b/m4/package_gdbmdev.m4 index 734a192baf4d..e96343168478 100644 --- a/m4/package_gdbmdev.m4 +++ b/m4/package_gdbmdev.m4 @@ -28,6 +28,14 @@ AC_DEFUN([AC_PACKAGE_WANT_GDBM], AC_CHECK_HEADER(gdbm/ndbm.h, [ gdbm_ndbm_=true; have_db=true ], [ gdbm_ndbm_=false; have_db=false ]) if test $gdbm_ndbm_ = true; then AC_DEFINE(HAVE_GDBM_NDBM_H_, [1], [Define to 1 if you have the <gdbm/ndbm.h> header file.]) + else + AC_CHECK_HEADER(gdbm.h, [ gdbm_ndbm_=true; have_db=true ], [ gdbm_ndbm_=false; have_db=false ]) + AC_CHECK_HEADER(ndbm.h, [ ndbm_=true ], [ ndbm_=false ]) + if test $gdbm_ndbm_ = true; then + if test $ndbm_ = true; then + AC_DEFINE(HAVE_GDBM_H, [1], [Define to 1 if you have both <gdbm.h> and <ndbm.h> header files.]) + fi + fi fi fi diff --git a/src/dbtest.c b/src/dbtest.c index ec8db0b93a4e..f45db4ac2b19 100644 --- a/src/dbtest.c +++ b/src/dbtest.c @@ -24,6 +24,7 @@ #include <gdbm-ndbm.h> #elif HAVE_GDBM_H #include <gdbm.h> +#include <ndbm.h> #elif HAVE_NDBM_H #include <ndbm.h> #else
Modern gdbm-devel packages bundle together gdbm.h and ndbm.h. The old m4 macro had detection support for some old gdbm libraries but not for new ones. We fix compilation of src/dbtest.c by making the autoconf helper check for this new arrangement: If both gdbm.h and gdbm.h are found define set both gdbm_ndbm_=true, and have_db=true, and define HAVE_GDBM_H. The src/dbtest.c already had a HAVE_GDBM_H but there was never a respective autoconf settter for it. We can just re-use this and fix it for new arrangement. Signed-off-by: Luis R. Rodriguez <mcgrof@kernel.org> --- m4/package_gdbmdev.m4 | 8 ++++++++ src/dbtest.c | 1 + 2 files changed, 9 insertions(+)