Re: CVS commit: src

[Izumi Tsutsui <tsutsui%ceres.dti.ne.jp@localhost>]

[should I move this to tech-toolchain?] 

christos%zoulas.com@localhost wrote:

> | > | > I think that vasnprintf and asnprintf are not used by anything in
> | > | > heimdal and can safely be removed. Combined with the winsize fix,
> | > | > this fixes the cygwin problems with minimal changes. I am trying a
> | > | > build now.
> | > | 
> | > | Note roken.h includes <resolv.h> and <arpa/nameser.h> that don't
> | > | exist on Cygwin so we had to handle it in src/tools/compat/configure.
> | > | (toolchain/29032)
> | > 
> | > Which is fine; I'd rather have one place to keep roken.h and deal with
> | > portability in compat, rather than 2.
> | 
> | But src/include/heimdal/roken.h is a generated file for NetBSD
> | with unusual method.
> | 
> | We might be able to generate roken.h for tools host from
> | src/crypto/dist/heimdal/lib/roken/roken.h.in using
> | src/crypto/dist/heimdal/lib/roken/roken.awk as defined
> | in src/crypto/dist/heimdal/lib/roken/Makefile.am:
> | ---
> | roken.h: make-roken$(EXEEXT)
> |     @./make-roken$(EXEEXT) > tmp.h ;\
> |     if [ -f roken.h ] && cmp -s tmp.h roken.h ; then rm -f tmp.h ; \
> |     else rm -f roken.h; mv tmp.h roken.h; fi
> | 
> | make-roken.c: roken.h.in roken.awk
> |     $(AWK) -f $(srcdir)/roken.awk $(srcdir)/roken.h.in > make-roken.c
> | ---
> | but it requires all macros like HAVE_FOO referred in roken.h.in
> | and we have to add checks for them into src/tools/compat/configure.ac,
> | as defined in src/include/heimdal/config.h configured for NetBSD.
> | 
> | Is it really worth than adding a manually edited dumb roken.h for tools?
> 
> Well the manually edited roken.h will need to have HAVE_FOO for each feature
> in order to work across different platforms. What is currently broken in
> the one we have? As far as resolv.h and arpa/nameser.h we need them elsewhere
> too, so we have to fix them anyway.

 * src/crypto/dist/heimdal/lib/roken/roken.h.in has HAVE_FOO for each feature
   in order to work across different platforms:
---
#ifdef HAVE_ARPA_NAMESER_H
#include <arpa/nameser.h>
#endif
#ifdef HAVE_RESOLV_H
#include <resolv.h>
#endif

 :

#if !defined(HAVE_ASNPRINTF) || defined(NEED_ASNPRINTF_PROTO)
int ROKEN_LIB_FUNCTION
    asnprintf (char **, size_t, const char *, ...)
     __attribute__ ((format (printf, 3, 4)));
#endif

#if !defined(HAVE_VASNPRINTF) || defined(NEED_VASNPRINTF_PROTO)
int ROKEN_LIB_FUNCTION
    vasnprintf (char **, size_t, const char *, va_list)
     __attribute__((format (printf, 3, 0)));
#endif

 :

int ROKEN_LIB_FUNCTION issuid(void);

#ifndef HAVE_STRUCT_WINSIZE
struct winsize {
        unsigned short ws_row, ws_col;
        unsigned short ws_xpixel, ws_ypixel;
};
#endif

int ROKEN_LIB_FUNCTION get_window_size(int fd, struct winsize *);

 :
---

 * But heimdal doesn't use it as is for a header file to be installed to
   a target system.

 * Makefiles in heimdal generate "make-roken.c" from roken.h.in using
   src/crypto/dist/heimdal/lib/roken/roken.awk.

 * generated make-roken.c is a dumb program to print "pre-processed" lines:
---
#ifdef HAVE_CONFIG_H
#include <config.h>
#endif
#include <stdio.h>

int main(int argc, char **argv)
{

 :

#ifdef HAVE_ARPA_NAMESER_H
puts("#include <arpa/nameser.h>");
#endif
#ifdef HAVE_RESOLV_H
puts("#include <resolv.h>");
#endif

 :

#if !defined(HAVE_ASNPRINTF) || defined(NEED_ASNPRINTF_PROTO)
puts("int ROKEN_LIB_FUNCTION");
puts("    asnprintf (char **, size_t, const char *, ...)");
puts("     __attribute__ ((format (printf, 3, 4)));");
#endif
puts("");
#if !defined(HAVE_VASNPRINTF) || defined(NEED_VASNPRINTF_PROTO)
puts("int ROKEN_LIB_FUNCTION");
puts("    vasnprintf (char **, size_t, const char *, va_list)");
puts("     __attribute__((format (printf, 3, 0)));");
#endif
puts("");

 :

puts("int ROKEN_LIB_FUNCTION issuid(void);");
puts("");
#ifndef HAVE_STRUCT_WINSIZE
puts("struct winsize {");
puts("  unsigned short ws_row, ws_col;");
puts("  unsigned short ws_xpixel, ws_ypixel;");
puts("};");
#endif
puts("");
puts("int ROKEN_LIB_FUNCTION get_window_size(int fd, struct winsize *);");
puts("");
---

 * Then generated roken.h (installed into src/include/heimdal/ in NetBSD)
   looks like:
---
#include <arpa/inet.h>
#include <netdb.h>
#include <arpa/nameser.h>
#include <resolv.h>
#include <syslog.h>

 :


int ROKEN_LIB_FUNCTION
    asnprintf (char **, size_t, const char *, ...)
     __attribute__ ((format (printf, 3, 4)));

int ROKEN_LIB_FUNCTION
    vasnprintf (char **, size_t, const char *, va_list)
     __attribute__((format (printf, 3, 0)));

 :

int ROKEN_LIB_FUNCTION issuid(void);


int ROKEN_LIB_FUNCTION get_window_size(int fd, struct winsize *);
---

quoted again:
> Well the manually edited roken.h will need to have HAVE_FOO for each feature
> in order to work across different platforms. What is currently broken in
> the one we have?

 * As mentioned above, src/include/heimdal/roken.h doesn't have HAVE_FOO
   for each features in order to work across different platforms,
   so we should not use it for tools build. That's the broken point.

 * roken.h required to build src/tools/asn1_compile and src/tools/compile_et
   on tools hosts needs to have HAVE_FOO for each feature, but only
   limited definitions in roken.h.in are required by these two programs.

 * In my patch, I copied src/crypto/dist/heimdal/lib/roken/roken.h.in
   into src/tools/asn1_compile/roken.h, manually edited it, and added
   -I${.CURDIR} to HOST_CFLAGS in Makefiles.

 * I left all HAVE_FOO_H header checks and added necessary checks into
   src/tools/compat/configure.ac because each header might be required
   by <roken-common.h> included from roken.h itself.

 * I removed all HAVE_FEATURES checks except HAVE_STRUCT_WINSIZE
   from manually edited roken.h for tools because asn1_compile and
   compile_et builds didn't require them. So I also added a check for
   struct winsize in src/tools/compat/configure.ac.

 * I also removed all function decls except get_windows_size()
   because tools programs didn't use them.

 * I didn't change src/include/heimdal/roken.h for the next import
   of heimdal in future.

I think this is an reasonable compromise.
Do you have any better suggestion?

---
Izumi Tsutsui