* FREAD_READS_DIRECTORIES test fails for the wrong reasons @ 2018-04-06 18:06 Jonathan Primrose 2018-04-06 19:33 ` Jeff King 0 siblings, 1 reply; 4+ messages in thread From: Jonathan Primrose @ 2018-04-06 18:06 UTC (permalink / raw) To: git 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. ^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: FREAD_READS_DIRECTORIES test fails for the wrong reasons 2018-04-06 18:06 FREAD_READS_DIRECTORIES test fails for the wrong reasons Jonathan Primrose @ 2018-04-06 19:33 ` Jeff King 2018-04-06 21:57 ` Jonathan Primrose 0 siblings, 1 reply; 4+ messages in thread From: Jeff King @ 2018-04-06 19:33 UTC (permalink / raw) To: Jonathan Primrose; +Cc: git 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'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 ^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: FREAD_READS_DIRECTORIES test fails for the wrong reasons 2018-04-06 19:33 ` Jeff King @ 2018-04-06 21:57 ` Jonathan Primrose 2018-04-06 22:07 ` Jeff King 0 siblings, 1 reply; 4+ messages in thread From: Jonathan Primrose @ 2018-04-06 21:57 UTC (permalink / raw) To: Jeff King; +Cc: git On 04/06/2018 03:33 PM, Jeff King wrote: > 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.) > >> 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. > > 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 ^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: FREAD_READS_DIRECTORIES test fails for the wrong reasons 2018-04-06 21:57 ` Jonathan Primrose @ 2018-04-06 22:07 ` Jeff King 0 siblings, 0 replies; 4+ messages in thread From: Jeff King @ 2018-04-06 22:07 UTC (permalink / raw) To: Jonathan Primrose; +Cc: git On Fri, Apr 06, 2018 at 05:57:15PM -0400, Jonathan Primrose wrote: > > 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 ^ permalink raw reply [flat|nested] 4+ messages in thread
end of thread, other threads:[~2018-04-06 22:07 UTC | newest] Thread overview: 4+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2018-04-06 18:06 FREAD_READS_DIRECTORIES test fails for the wrong reasons Jonathan Primrose 2018-04-06 19:33 ` Jeff King 2018-04-06 21:57 ` Jonathan Primrose 2018-04-06 22:07 ` Jeff King
Code repositories for project(s) associated with this public inbox https://80x24.org/mirrors/git.git This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).