threads / discuss / 48232

FREAD_READS_DIRECTORIES test fails for the wrong reasons

Subject: FREAD_READS_DIRECTORIES test fails for the wrong reasons

## tl;dr

4 messages between Apr 6, 2018 and Apr 6, 2018.

replies: 3people: 2as markdown or json

Jonathan Primrose· Apr 6, 2018, 18:06 UTC · lore

A while ago, the configure test for FREAD_READS_DIRECTORIES was changed to (attempt to) check for a NULL result from fopen. Unfortunately, the test will always fail - because it won't compile. The code snippet in configure.ac translates to:

return f);

Either there's an extra ) or a missing (. A cast to int wouldn't hurt either.

I'd supply a patch to fix this, but I'm not sure what the results of suddenly fixing the test would be. It seems to work well on my machine, but I don't stress git much here, and it's just one machine.

Jeff King· Apr 6, 2018, 19:33 UTC · re: Jonathan Primrose · lore

Re: FREAD_READS_DIRECTORIES test fails for the wrong reasons

On Fri, Apr 06, 2018 at 02:06:50PM -0400, Jonathan Primrose wrote:
Show 9 quoted lines
> A while ago, the configure test for FREAD_READS_DIRECTORIES was changed
> to (attempt to) check for a NULL result from fopen. Unfortunately, the
> test will always fail - because it won't compile. The code snippet in
> configure.ac translates to:
> 
> return f);
> 
> Either there's an extra ) or a missing (. A cast to int wouldn't hurt
> either.

Oops. This is due to my 3adf9fdecf (configure.ac: loosen FREAD_READS_DIRECTORIES test program, 2017-06-14).

Neither I nor the original tester noticed, because we're on Linux, which needs that set.

> I'd supply a patch to fix this, but I'm not sure what the results of
> suddenly fixing the test would be. It seems to work well on my
> machine, but I don't stress git much here, and it's just one machine.

I think it should be fixed (and agreed on the "int" thing; the return value should be "f != NULL" or similar).

I don't know autoconf very well, but is there a way to invoke it that will let us know if a test-program fails to compile at all? Obviously for probing the system compler/includes, "does not compile" is an expected possible outcome. But for tests like this it's really a tristate: yes, no, or something went horribly wrong.

-Peff
Jonathan Primrose· Apr 6, 2018, 21:57 UTC · re: Jeff King · lore

Re: FREAD_READS_DIRECTORIES test fails for the wrong reasons

On 04/06/2018 03:33 PM, Jeff King wrote:
Show 17 quoted lines
> On Fri, Apr 06, 2018 at 02:06:50PM -0400, Jonathan Primrose wrote:
>
>> A while ago, the configure test for FREAD_READS_DIRECTORIES was changed
>> to (attempt to) check for a NULL result from fopen. Unfortunately, the
>> test will always fail - because it won't compile. The code snippet in
>> configure.ac translates to:
>>
>> return f);
>>
>> Either there's an extra ) or a missing (. A cast to int wouldn't hurt
>> either.
>
> Oops. This is due to my 3adf9fdecf (configure.ac: loosen
> FREAD_READS_DIRECTORIES test program, 2017-06-14).
>
> Neither I nor the original tester noticed, because we're on Linux, which
> needs that set.

I'm also running Linux, but somehow I ended up needing to check config.log and saw the error. I don't remember why I had to check. (I've been fixing the problem locally for the last few releases, figuring someone else would notice. I also remove the 'rm configure' from the distclean target in Makefile, just in case I decide to rebuild later.)

Show 7 quoted lines
>
>> I'd supply a patch to fix this, but I'm not sure what the results of
>> suddenly fixing the test would be. It seems to work well on my
>> machine, but I don't stress git much here, and it's just one machine.
>
> I think it should be fixed (and agreed on the "int" thing; the return
> value should be "f != NULL" or similar).

Ironically, most of the time I check config.log is because of the missing cast to int. A few years ago I set this machine up as a tri-arch system (x86, x86_64, and x32). Because of subtle errors that appear when converting between int and pointer on x32, I tend to use -Werror=int-conversion on all my builds, and I wrap configure in a script that (among other things) checks config.log for that particular error. Even though x32 isn't as promising anymore (thanks to a few projects rejecting support patches), there are very few packages that still trigger the error.

Show 6 quoted lines
>
> I don't know autoconf very well, but is there a way to invoke it that
> will let us know if a test-program fails to compile at all? Obviously
> for probing the system compler/includes, "does not compile" is an
> expected possible outcome. But for tests like this it's really a
> tristate: yes, no, or something went horribly wrong.

I don't know if that's supported. From what I've seen in the docs I've found online, it looks like the closest is a case for "I couldn't run the program because I'm cross-compiling." You might be able to determine that the compile and/or link failed by looking at some of autoconf's internal variables, but it would be fragile.

>
> -Peff
>
Jonathan
Jeff King· Apr 6, 2018, 22:07 UTC · re: Jonathan Primrose · lore

Re: FREAD_READS_DIRECTORIES test fails for the wrong reasons

On Fri, Apr 06, 2018 at 05:57:15PM -0400, Jonathan Primrose wrote:
Show 12 quoted lines
> > Oops. This is due to my 3adf9fdecf (configure.ac: loosen
> > FREAD_READS_DIRECTORIES test program, 2017-06-14).
> > 
> > Neither I nor the original tester noticed, because we're on Linux, which
> > needs that set.
> 
> I'm also running Linux, but somehow I ended up needing to check
> config.log and saw the error. I don't remember why I had to check.
> (I've been fixing the problem locally for the last few releases,
> figuring someone else would notice. I also remove the 'rm configure'
> from the distclean target in Makefile, just in case I decide to
> rebuild later.)

I think you'll find that most of the developers don't actually use autoconf themselves, so bugs tend to go unnoticed there for a while. (The failure case for setting this flag accidentally is not bad, either; it just incurs an extra fstat).

-Peff

← back to recent threads