[hurd,commited] hurd: do not check Mach and Hurd headers

  as they are not standard.

	* scripts/check-installed-headers.sh: Ignore Hurd and Mach headers.
---
 ChangeLog                          | 4 ++++
 scripts/check-installed-headers.sh | 7 +++++++
 2 files changed, 11 insertions(+)
  

On Sat, 3 Mar 2018, Samuel Thibault wrote:

> as they are not standard.

That's not a sufficient reason for this change.  What this script is 
checking is nothing to do with whether the headers are standard; it's that 
each header can be included in isolation with any supported feature test 
macro defined (actually, just a few macros are tested) and in any C/C++ 
standards mode.

That property should apply to Hurd-specific headers installed by glibc 
just as it applies to non-Hurd-specific headers (this script doesn't test 
headers not installed by glibc).  That is, any case of such a header 
failing this test is presumptively a bug that should be fixed.  (If a 
header is in fact purely internal, not for direct use by users or by other 
installed headers, stopping it being installed is the correct fix - this 
test is specifically for installed headers.  If a header is installed only 
for use by other installed headers and is not meant to be included 
directly by user programs, it should move into bits/; bits/ headers are 
already excluded from this test.)
  

Hello,

Joseph Myers, on sam. 03 mars 2018 22:08:49 +0000, wrote:
> On Sat, 3 Mar 2018, Samuel Thibault wrote:
> 
> > as they are not standard.
> 
> That's not a sufficient reason for this change.  What this script is 
> checking is nothing to do with whether the headers are standard; it's that 
> each header can be included in isolation with any supported feature test 
> macro defined (actually, just a few macros are tested) and in any C/C++ 
> standards mode.

Well hurd & mach headers are not meant to be used without _GNU_SOURCE=1,
for a start...  Usual applications wouldn't use them anyway, only things
like e.g. libparted, Xorg, etc. which need to interact closely with
system things use them, and thus have to enable the GNU extensions.  You
can think of them like linux/ headers.

Samuel
  

On Sat, 3 Mar 2018, Samuel Thibault wrote:

> Well hurd & mach headers are not meant to be used without _GNU_SOURCE=1,
> for a start...  Usual applications wouldn't use them anyway, only things

If a header requires some type T, I'd expect it to include bits/types/T.h, 
thereby ensuring that type T is defined regardless of feature test macros, 
instead of including some other header that might or might not define type 
T depending on feature test macros.

Likewise for any other dependencies on features from other headers.  (And 
of course these headers shouldn't themselves contain feature test macro 
conditionals.)

> like e.g. libparted, Xorg, etc. which need to interact closely with
> system things use them, and thus have to enable the GNU extensions.  You
> can think of them like linux/ headers.

Well, all linux/*.h and asm/*h headers also ought to be includable in 
isolation, with any feature test macros defined.  But that's the Linux 
kernel's problem, not ours, since it's the Linux kernel that provides 
those headers.  Whereas this test is purely for headers installed by 
glibc.  And every header installed by glibc should either work in 
isolation, or if it's not meant to work in some context should have a 
#error explaining the issue - like the #errors in bits/*.h headers saying 
not to include them directly, or the #error in sys/elf.h for x86_64 
GNU/Linux, for example.

I think it's dubious to have a #error requiring _GNU_SOURCE to be used, 
because it should always be possible to write a header in a way that 
doesn't have such a requirement.  But in any case where, after analysis, 
such a #error is found to make sense (and a comment goes on the #error 
explaining why it makes sense), that specific header might have testing 
disabled in check-installed-headers.sh *only* when _GNU_SOURCE is not 
passed - not for other feature test macros, not using wildcards like 
hurd/*.h.
  

Hello,

Joseph Myers, on sam. 03 mars 2018 22:53:23 +0000, wrote:
> On Sat, 3 Mar 2018, Samuel Thibault wrote:
> > like e.g. libparted, Xorg, etc. which need to interact closely with
> > system things use them, and thus have to enable the GNU extensions.  You
> > can think of them like linux/ headers.
> 
> Well, all linux/*.h and asm/*h headers also ought to be includable in 
> isolation, with any feature test macros defined.  But that's the Linux 
> kernel's problem, not ours, since it's the Linux kernel that provides 
> those headers.  Whereas this test is purely for headers installed by 
> glibc.

I only mentioned Linux to point out which kind of interface we are
talking about: not something for normal applications.  Here, glibc is
distributing pieces of the Hurd which the Linux kernel distributes.

> And every header installed by glibc should either work in 
> isolation, or if it's not meant to work in some context should have a 
> #error explaining the issue - like the #errors in bits/*.h headers saying 
> not to include them directly, or the #error in sys/elf.h for x86_64 
> GNU/Linux, for example.

Ok.

> But in any case where, after analysis, such a #error is found to
> make sense (and a comment goes on the #error explaining why it
> makes sense), that specific header might have testing disabled in
> check-installed-headers.sh *only* when _GNU_SOURCE is not passed - not
> for other feature test macros, not using wildcards like hurd/*.h.

I have sent a patch adding support for this, and whitelisting mach
headers, could you have a look?

Samuel
  

Joseph Myers, on sam. 03 mars 2018 22:53:23 +0000, wrote:
> I think it's dubious to have a #error requiring _GNU_SOURCE to be used, 
> because it should always be possible to write a header in a way that 
> doesn't have such a requirement.  But in any case where, after analysis, 
> such a #error is found to make sense

Notably, most Hurd interfaces use the error_t type, which is GNU-only.
I assume this is enough to make it a #error case?

Samuel
  

On Sun, 4 Mar 2018, Samuel Thibault wrote:

> Joseph Myers, on sam. 03 mars 2018 22:53:23 +0000, wrote:
> > I think it's dubious to have a #error requiring _GNU_SOURCE to be used, 
> > because it should always be possible to write a header in a way that 
> > doesn't have such a requirement.  But in any case where, after analysis, 
> > such a #error is found to make sense
> 
> Notably, most Hurd interfaces use the error_t type, which is GNU-only.
> I assume this is enough to make it a #error case?

No.  Create bits/types/error_t.h (which would have a generic version and a 
Hurd version), which would define error_t (subject to __error_t_defined as 
a multiple-include guard).  Then make errno.h include 
<bits/types/error_t.h> if __USE_GNU, while all the Hurd headers using that 
type would include <bits/types/error_t.h> unconditionally.  
(sysdeps/mach/hurd/bits/errno.h would no longer need to define error_t and 
wouldn't need to include <bits/types/error_t.h> either because of errno.h 
doing so - I'm assuming the current reason for defining error_t there is 
simply to get a Hurd-specific definition instead of the default from 
errno.h, which would be dealt with by having a Hurd-specific 
bits/types/error_t.h.)

Having such a bits/types/*.h header is the normal way in glibc of dealing 
with types that different headers need under different conditions.
  

On Sun, 4 Mar 2018, Samuel Thibault wrote:

> > But in any case where, after analysis, such a #error is found to
> > make sense (and a comment goes on the #error explaining why it
> > makes sense), that specific header might have testing disabled in
> > check-installed-headers.sh *only* when _GNU_SOURCE is not passed - not
> > for other feature test macros, not using wildcards like hurd/*.h.
> 
> I have sent a patch adding support for this, and whitelisting mach
> headers, could you have a look?

It's not yet clear any such special cases in the script are needed.  But 
if they are, they would only be for specific headers that use #error 
without __USE_GNU, not any wildcard such as hurd/*.h, and we should review 
whether the #error usage is necessary or whether there's a better approach 
in each particular case.
  

Joseph Myers, on dim. 04 mars 2018 01:33:18 +0000, wrote:
> On Sun, 4 Mar 2018, Samuel Thibault wrote:
> > > But in any case where, after analysis, such a #error is found to
> > > make sense (and a comment goes on the #error explaining why it
> > > makes sense), that specific header might have testing disabled in
> > > check-installed-headers.sh *only* when _GNU_SOURCE is not passed - not
> > > for other feature test macros, not using wildcards like hurd/*.h.
> > 
> > I have sent a patch adding support for this, and whitelisting mach
> > headers, could you have a look?
> 
> It's not yet clear any such special cases in the script are needed.

See the patch: mach/mach/mig_support.h explains why it needs
_GNU_SOURCE.

> But 
> if they are, they would only be for specific headers that use #error 
> without __USE_GNU, not any wildcard such as hurd/*.h,

Sure, I _*NEVER*_ meant to do anything like that, as my patch shows.

Samuel
  

On Sun, 4 Mar 2018, Samuel Thibault wrote:

> Joseph Myers, on dim. 04 mars 2018 01:33:18 +0000, wrote:
> > On Sun, 4 Mar 2018, Samuel Thibault wrote:
> > > > But in any case where, after analysis, such a #error is found to
> > > > make sense (and a comment goes on the #error explaining why it
> > > > makes sense), that specific header might have testing disabled in
> > > > check-installed-headers.sh *only* when _GNU_SOURCE is not passed - not
> > > > for other feature test macros, not using wildcards like hurd/*.h.
> > > 
> > > I have sent a patch adding support for this, and whitelisting mach
> > > headers, could you have a look?
> > 
> > It's not yet clear any such special cases in the script are needed.
> 
> See the patch: mach/mach/mig_support.h explains why it needs
> _GNU_SOURCE.

Well, I disagree with the explanation there.  The use of __stpncpy is in 
an _LIBC-conditional block, and _LIBC can reasonably be taken to imply 
_GNU_SOURCE, so if you remove the #error I see no reason for this header 
to break without _GNU_SOURCE.  It just requires a few Mach-specific types, 
and all the headers declaring such types should do so unconditionally.

(Such _LIBC-conditional code should where possible move out of installed 
headers and into non-installed headers used only for building glibc, but 
that's a separate longstanding issue.  _LIBC conditionals are more 
appropriate for e.g. code shared with gnulib.)

> > But 
> > if they are, they would only be for specific headers that use #error 
> > without __USE_GNU, not any wildcard such as hurd/*.h,
> 
> Sure, I _*NEVER*_ meant to do anything like that, as my patch shows.

My reading of <https://sourceware.org/ml/libc-alpha/2018-03/msg00074.html> 
is that it still has:

+        # Hurd headers are not standard anyway
+        (hurd.h | hurd/*.h | faultexc_server.h)
+            continue;;

and as noted, I don't think hurd/*.h should be special-cased like that (or 
that "not standard" is a sufficient explanation for a comment here).
  

Joseph Myers, on dim. 04 mars 2018 01:41:43 +0000, wrote:
> My reading of <https://sourceware.org/ml/libc-alpha/2018-03/msg00074.html> 
> is that it still has:
> 
> +        # Hurd headers are not standard anyway
> +        (hurd.h | hurd/*.h | faultexc_server.h)
> +            continue;;

The reading of the patch really is all the - on mach headers.  Small
step to get something done instead of having to do _*ALL*_ the work at
one time and never manage to find motivation to finish it all.

> and as noted, I don't think hurd/*.h should be special-cased like that (or 
> that "not standard" is a sufficient explanation for a comment here).

Sure, that was never the intent of that patch.

Samuel
  

Joseph Myers, on dim. 04 mars 2018 01:41:43 +0000, wrote:
> Well, I disagree with the explanation there.  The use of __stpncpy is in 
> an _LIBC-conditional block,

Indeed, that got in in between.

/me more and more tired of fixed all kinds of code that were written by
other people more than two decades ago.

Samuel
  

On Sun, 4 Mar 2018, Samuel Thibault wrote:

> Joseph Myers, on dim. 04 mars 2018 01:41:43 +0000, wrote:
> > My reading of <https://sourceware.org/ml/libc-alpha/2018-03/msg00074.html> 
> > is that it still has:
> > 
> > +        # Hurd headers are not standard anyway
> > +        (hurd.h | hurd/*.h | faultexc_server.h)
> > +            continue;;
> 
> The reading of the patch really is all the - on mach headers.  Small
> step to get something done instead of having to do _*ALL*_ the work at
> one time and never manage to find motivation to finish it all.

I don't actually see the problem with having some of these tests failing - 
no Hurd special cases in this script - unless and until we decide that a 
particular Hurd special case in this script is appropriate.  That was how 
we handled many architecture-specific fixes for header problems shown by 
these tests for GNU/Linux (some of these tests failed on some 
architectures for some time); I think it's how we should handle them for 
Hurd.  When individual headers are fixed, the number of failures may go 
down.
  

Joseph Myers, on sam. 03 mars 2018 22:53:23 +0000, wrote:
> it should always be possible to write a header in a way that 
> doesn't have such a requirement.

I'm almost there, the only remaining issue is that hurd/hurd/signal.h
uses struct sigaction.  I guess I should move existing definitions to a
struct_sigaction.h header?

Samuel
  

On Sat, Mar 3, 2018 at 9:55 PM, Samuel Thibault
<samuel.thibault@ens-lyon.org> wrote:
> Joseph Myers, on sam. 03 mars 2018 22:53:23 +0000, wrote:
>> it should always be possible to write a header in a way that
>> doesn't have such a requirement.
>
> I'm almost there, the only remaining issue is that hurd/hurd/signal.h
> uses struct sigaction.  I guess I should move existing definitions to a
> struct_sigaction.h header?

In this case, you just need a Hurd version of the existing
bits/sigaction.h.  (It's bits/sigaction.h rather than
bits/types/struct_sigaction.h because it also defines the SA_*
constants.)

zw
  

Zack Weinberg, on dim. 04 mars 2018 11:08:00 -0500, wrote:
> On Sat, Mar 3, 2018 at 9:55 PM, Samuel Thibault
> <samuel.thibault@ens-lyon.org> wrote:
> > Joseph Myers, on sam. 03 mars 2018 22:53:23 +0000, wrote:
> >> it should always be possible to write a header in a way that
> >> doesn't have such a requirement.
> >
> > I'm almost there, the only remaining issue is that hurd/hurd/signal.h
> > uses struct sigaction.  I guess I should move existing definitions to a
> > struct_sigaction.h header?
> 
> In this case, you just need a Hurd version of the existing
> bits/sigaction.h.

I don't understand why a different version.  Can't I just make
hurd/hurd/signal.h include <bits/sigaction.h>? (well, there is a
technical reason: it does not have multi-inclusion guard, but that can
be fixed)

Samuel
  

On Sun, Mar 4, 2018 at 1:09 PM, Samuel Thibault
<samuel.thibault@ens-lyon.org> wrote:
> Zack Weinberg, on dim. 04 mars 2018 11:08:00 -0500, wrote:
>> On Sat, Mar 3, 2018 at 9:55 PM, Samuel Thibault
>> <samuel.thibault@ens-lyon.org> wrote:
>> > Joseph Myers, on sam. 03 mars 2018 22:53:23 +0000, wrote:
>> >> it should always be possible to write a header in a way that
>> >> doesn't have such a requirement.
>> >
>> > I'm almost there, the only remaining issue is that hurd/hurd/signal.h
>> > uses struct sigaction.  I guess I should move existing definitions to a
>> > struct_sigaction.h header?
>>
>> In this case, you just need a Hurd version of the existing
>> bits/sigaction.h.
>
> I don't understand why a different version.  Can't I just make
> hurd/hurd/signal.h include <bits/sigaction.h>? (well, there is a
> technical reason: it does not have multi-inclusion guard, but that can
> be fixed)

Oh, yes, if the existing generic bits/sigaction.h works for Hurd then
that's fine.  I thought there had to be something wrong with it,
because hurd/signal.h already includes signal.h which includes
bits/sigaction.h ... but now I realize that the problem is signal.h
doesn't _always_ include bits/sigaction.h.

zw