[openib-general] Please give 1.0 RC1 a whirl

[Bryan O'Sullivan]

On Wed, 2006-03-15 at 17:15 +0200, Moni Shoua wrote:

Thanks for doing this.  I have a few comments.

In general, your .spec.in files should follow the pattern of
the .spec.in files in the 1.0 branch.  Here are some problems I can see:

> +%define prefix /usr

This should not be present.

> +%description
> +udat is 

Please provide a proper description.

> +make -C udapl clean

There's no need to make clean.

> +%files
> +%{prefix}/lib64/libdapl.a
> +%{prefix}/lib64/libdapl.so

These are wrong.  There ought to be two packages here, a regular package
and a -devel package.  The .so file should be a symlink to a .so.N file.
Only the .so.N file belongs in the main package - the .a and the .so
should be in -devel, along with headers.  Headers should be packaged,
too.

Also, libraries go in %{_libdir}, not %{prefix}.

> +%defattr(-,root,root)

It's normal to have this immediately after the %files directive.

> -CFLAGS   += -I/usr/local/include/infiniband
> +CFLAGS   += -I/usr/include/infiniband -I/usr/local/include/infiniband

Please remove /usr/local from all paths in all build-related files.

>  ifeq ($(VERBS),openib)
>  LDFLAGS += -libverbs -libcm -libat
> -LDFLAGS += -rpath /usr/local/lib -L /usr/local/lib
> +LDFLAGS += -rpath /usr/local/lib64 -L /usr/local/lib64

Please remove -rpath from all build-related files.

> +%ifarch x86_64
> +%define dual_arch 1
> +%endif

This should not be here at all.

> +%files
> +%{prefix}/include/dat/dat.h

Please use %{_includedir}.

> +/etc/dat.conf

Please use %{_sysconfdir}.

> +%if %{dual_arch}
> +%{prefix}/lib64/libdat.a
> +%{prefix}/lib64/libdat.so
> +%endif

Same comments apply as to previous spec file, and the conditional should
go.

> +++ openib.org/src/userspace/dapl/dat/udat/Makefile	2006-03-14 16:07:50.000000000 +0200
> @@ -59,6 +59,7 @@
>  DYNAMIC = $(TARGET_PATH)/libdat.so
>  STATIC32 = $(TARGET_PATH32)/libdat.a
>  DYNAMIC32 = $(TARGET_PATH32)/libdat.so
> +CONF=$(UDAT_ROOT)/dat.conf
>  
>  ifeq "$(ARCH)" "x86_64"
>  DUAL_ARCH = true

These Makefiles must be fixed so that they do not try dual-arch builds.

>  install: all
> +	mkdir -p $(PREFIX)/usr/lib
> +	mkdir -p $(PREFIX)/usr/lib64
> +	mkdir -p $(PREFIX)/etc

This will break if someone redefines _prefix on the rpmbuild command
line.  This Makefile should be rewritten to use autotools.

>  %defattr(-,root,root)
> -%{_libdir}/librdmacm*.so.*
> +%{_libdir}/librdmacm*.so

I already have a correct .spec.in file for librdmacm in the 1.0 branch.
Please drop the changes to this file, as they are wrong and conflict
with my changes.

Thank you,

	<b