[openib-general] RE: [PATCH2] uDAPL/uDAT autotools - Package for udat, udaplcma, udaplscm

[Arlin Davis]

James Lentini wrote:

>On Thu, 16 Mar 2006, Arlin Davis wrote:
>  
>
>This looks excellent Arlin. I think it is essentailly ready to go. I 
>have a few minor questions/commnets:
>
>I'll add an empty directory called "config" to the root of the dapl 
>tree.
>  
>

perfect.

>  
>
>>Index: AUTHORS
>>===================================================================
>>--- AUTHORS	(revision 0)
>>+++ AUTHORS	(revision 0)
>>@@ -0,0 +1,2 @@
>>+James Lentini	<jlentini at netapp.com>
>>+Arlin Davis	<arlin.r.davis at intel.com>
>>    
>>
>
>I'll add several more names to this. 
>  
>

Yes, this was just a start. Please check the COPYING file also for 
completeness.

>  
>
>>+dnl Checks for typedefs, structures, and compiler characteristics.
>>+AC_C_CONST
>>+AC_CHECK_SIZEOF(long)
>>    
>>
>
>If the code does not include config.h, do these have any effect? 
>  
>

no, we can remove.

>  
>
>>+
>>+dnl Checks for libraries
>>+if test "$disable_libcheck" != "yes"
>>+then
>>+AC_CHECK_LIB(ibverbs, ibv_get_device_list, [],
>>+    AC_MSG_ERROR([ibv_get_device_list() not found.  libdapl requires libibverbs.]))
>>+fi
>>    
>>
>
>Should we throw in a check for librdmacm?
>  
>

While there is dependency for libdaplcma there is no dependency for 
libdaplscm (socket CM).  Not sure
how to deal with different dependencies across multiple libraries in the 
package. Anyone have suggestions?

>+AC_HEADER_STDC
>  
>
>
>Again, if the code doesn't include config.h, does this have any 
>effect?
>  
>

no, we can remove.

>>+		dat_srq_resize;
>>+		dat_srq_set_lw;
>>    
>>
>
>We can make this local, right?
>
>  
>
>>+		dats_get_ia_handle;
>>    
>>
>
>  
>

I would say yes but the current version of Intel MPI is designed to support
both 1.1 and 1.2 udat/udapl providers on the fly via some tricks and 
actually does
a dlsym lookup on this function to determine a 1.2 uDAT version.

>I think the right thing to do is to add something to the configure.in 
>script. Here's what I propose (patch to your patch). I'm not 
>autotools expert, so let me know if you see anything wrong with this:
>
>
>+AC_CACHE_CHECK(whether this is an RHEL system, ac_cv_rhel,
>+    if test -f /etc/redhat-release && 
>+       test -n "`grep -v Fedora /etc/redhat-release`"; then
>+        ac_cv_rhel=yes
>+    else
>+        ac_cv_rhel=no
>+    fi)
>+
>+AM_CONDITIONAL(OS_RHEL, test "$ac_cv_rhel" = "yes")
>+
>
> 
>-# TODO...Need check to set properly
>-OSVENDOR="REDHAT_EL4"
>+if OS_RHEL
>+OSFLAGS=-DREDHAT_EL4
>+else
>+OSFLAGS=
>+endif
> 
> if DEBUG
> DBGFLAGS = -ggdb -DDAPL_DBG
>@@ -19,17 +22,17 @@ datlib_LTLIBRARIES = dat/udat/libdat.la
> dapllibcma_LTLIBRARIES = dapl/udapl/libdaplcma.la
> dapllibscm_LTLIBRARIES = dapl/udapl/libdaplscm.la
> 
>-dat_udat_libdat_la_CFLAGS = -Wall $(DBGFLAGS) -D_GNU_SOURCE -D$(OSVENDOR) 
>  
>

This looks good. Just change the Makefile.am _CFLAGS lines to get rid of 
the extra -D

I will package up a version 3 patch that will include these changes and 
a new dat.conf that works with the RPM.

-arlin