[parted-devel] Parted on GNU Hurd based systems

[Jim Meyering]

"Debarshi 'Rishi' Ray" <debarshi.ray at gmail.com> wrote:

>>> +dnl Checks for #defines.
>>> +have_s390=no
>>> +AC_CHECK_FUNC([__s390__], have_s390=yes, )
>>> +AC_CHECK_FUNC([__s390x__], have_s390=yes, )
>
>> __s390__ isn't really a *function*, is it?
>> Everywhere else, it's used like a cpp symbol.
>> If so, you need a different test (and different variable name):
>
> I think it is something like __linux__, __gnu__ which is pre-defined
> by the compiler. I tried AC_CHECK_DEFINE

Right.
As I said, those are cpp-defined (C preprocessor) symbols.

> (http://autoconf-archive.cryp.to/ax_check_define.html) but it seems to
> have become obsolete. I will use AC_EGREP_CPP now.
>
>> a *proper* autoconf-style test would not check for __s390__,
>> but rather for the existence of "struct fdasd_anchor" in whatever
>> header defines it.  Then the #if above would be
>>
>>   #if HAVE_STRUCT_FDASD_ANCHOR
>
> But 'struct fdasd_anchor' is a struct defined in include/parted/fdasd.h.

Then I wonder why there was such an #if directive in the first place.
Maybe some of the types used in that struct definition are s390-specific?

>> > +AM_CONDITIONAL([HAVE_S390], [test "$have_s390" = yes])
>> > +
>> >  dnl check for "check", unit testing library/header
>> >  PKG_CHECK_MODULES([CHECK], [check >= 0.9.3], have_check=yes, have_check=no)
>> >  if test "$have_check" != "yes"; then
>> > diff --git a/include/parted/Makefile.am b/include/parted/Makefile.am
>> > index 13df6c0..79e08cb 100644
>> > --- a/include/parted/Makefile.am
>> > +++ b/include/parted/Makefile.am
>> > @@ -1,4 +1,13 @@
>> > +if HAVE_S390
>> > +S390_HEADERS = fdasd.h vtoc.h
>> > +else
>> > +S390_HEADERS =
>> > +endif
>
>> Unless you have a good reason to exclude headers here, please don't.
>> Listing them here should be ok, even when not used.
>
> My first patch did not remove any. I thought David preferred to have
> them removed for consistency. Maybe I misunderstood. David?
>
>> >  partedincludedir      =      $(includedir)/parted
>> > +# Dummy-- to prevent error: "S390dir not defined".
>> > +S390dir               = $(partedincludedir)
>
>> Why is the above necessary?
>> I don't see any other use of that symbol.
>
> Automake throws a message during bootstrapping:
> include/parted/Makefile.am:4: `S390_HEADERS' is used but `S390dir' is undefined
> include/parted/Makefile.am:2: `S390_HEADERS' is used but `S390dir' is undefined
> Is that alright?

ISTR that _HEADERS is an automake-reserved variable suffix.
When you define <FOO>_HEADERS, it expects a <FOO>dir definition, too.
Just remove the S390_HEADERS definition, as I suggested, and you'll
no longer need the S390dir one either.

BTW, what is your goal in excluding them?
If you insist on excluding them, be sure that you're
not also excluding them from the distribution tarball (the one
created by "make dist" run on an x86 Linux system) -- unless
that is the intent, in which case I'd say you should just remove
them altogether.

>> >  partedinclude_HEADERS = gnu.h                \
>> >                       linux.h         \
>> >                       constraint.h    \
>> > @@ -11,9 +20,8 @@ partedinclude_HEADERS = gnu.h               \
>> >                       natmath.h       \
>> >                       timer.h         \
>> >                       unit.h          \
>> > -                     parted.h    \
>> > -                     vtoc.h          \
>> > -                     fdasd.h
>> > +                     parted.h        \
>> > +                        $(S390_HEADERS)
>
>> Inconsistent indentation.
>> In Makefiles, always indent with TAB.
>
> Even when we are preferring space over tabs in the source files?

Yes.  Of course!
Use spaces to indent in C, C++, Perl, etc. sources.

Makefiles must have TABs in some places, but not in definitions
like that one, so you could write it like this (which I prefer,
e.g., since changing the variable name length doesn't require changing
all of the following lines):

 partedinclude_HEADERS = \
   gnu.h           \
   linux.h         \
   constraint.h    \
   natmath.h       \
   timer.h         \
   unit.h          \
   parted.h        \
   vtoc.h          \
   fdasd.h         \
   parted.h        \
   $(S390_HEADERS)

The problem with inconsistent indentation like the above is that
it makes diffs look bad.  Sometimes very bad.