{"thread":{"id":"48232","subject":"FREAD_READS_DIRECTORIES test fails for the wrong reasons","startedAt":"2018-04-06T18:12:03Z","lastAt":"2018-04-06T22:07:42Z","messageCount":4,"participants":["Jonathan Primrose","Jeff King"],"isPatch":false,"patchVersion":null,"patchTotal":null},"messages":[{"id":"343976","messageId":"82c91eac-9dc4-834b-4648-3c4ba45af80d@gmail.com","threadId":"48232","inReplyTo":null,"subject":"FREAD_READS_DIRECTORIES test fails for the wrong reasons","fromName":"Jonathan Primrose","fromEmail":"jprimros@gmail.com","sentAt":"2018-04-06T18:06:50Z","receivedAt":"2018-04-06T18:12:03Z","isPatch":false,"sender":{"key":"jprimros@gmail.com","avatar":null},"body":"A while ago, the configure test for FREAD_READS_DIRECTORIES was changed\nto (attempt to) check for a NULL result from fopen. Unfortunately, the\ntest will always fail - because it won't compile. The code snippet in\nconfigure.ac translates to:\n\nreturn f);\n\nEither there's an extra ) or a missing (. A cast to int wouldn't hurt\neither.\n\nI'd supply a patch to fix this, but I'm not sure what the results of\nsuddenly fixing the test would be. It seems to work well on my\nmachine, but I don't stress git much here, and it's just one machine.\n"},{"id":"343994","messageId":"20180406193301.GA11450@sigill.intra.peff.net","threadId":"48232","inReplyTo":"82c91eac-9dc4-834b-4648-3c4ba45af80d@gmail.com","subject":"Re: FREAD_READS_DIRECTORIES test fails for the wrong reasons","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2018-04-06T19:33:01Z","receivedAt":"2018-04-06T19:33:12Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Fri, Apr 06, 2018 at 02:06:50PM -0400, Jonathan Primrose wrote:\n\n> A while ago, the configure test for FREAD_READS_DIRECTORIES was changed\n> to (attempt to) check for a NULL result from fopen. Unfortunately, the\n> test will always fail - because it won't compile. The code snippet in\n> configure.ac translates to:\n> \n> return f);\n> \n> Either there's an extra ) or a missing (. A cast to int wouldn't hurt\n> either.\n\nOops. This is due to my 3adf9fdecf (configure.ac: loosen\nFREAD_READS_DIRECTORIES test program, 2017-06-14).\n\nNeither I nor the original tester noticed, because we're on Linux, which\nneeds that set.\n\n> I'd supply a patch to fix this, but I'm not sure what the results of\n> suddenly fixing the test would be. It seems to work well on my\n> machine, but I don't stress git much here, and it's just one machine.\n\nI think it should be fixed (and agreed on the \"int\" thing; the return\nvalue should be \"f != NULL\" or similar).\n\nI don't know autoconf very well, but is there a way to invoke it that\nwill let us know if a test-program fails to compile at all? Obviously\nfor probing the system compler/includes, \"does not compile\" is an\nexpected possible outcome. But for tests like this it's really a\ntristate: yes, no, or something went horribly wrong.\n\n-Peff\n"},{"id":"344017","messageId":"8ed8491f-9410-c509-5ef7-77638aa74038@gmail.com","threadId":"48232","inReplyTo":"20180406193301.GA11450@sigill.intra.peff.net","subject":"Re: FREAD_READS_DIRECTORIES test fails for the wrong reasons","fromName":"Jonathan Primrose","fromEmail":"jprimros@gmail.com","sentAt":"2018-04-06T21:57:15Z","receivedAt":"2018-04-06T21:57:23Z","isPatch":false,"sender":{"key":"jprimros@gmail.com","avatar":null},"body":"On 04/06/2018 03:33 PM, Jeff King wrote:\n> On Fri, Apr 06, 2018 at 02:06:50PM -0400, Jonathan Primrose wrote:\n>\n>> A while ago, the configure test for FREAD_READS_DIRECTORIES was changed\n>> to (attempt to) check for a NULL result from fopen. Unfortunately, the\n>> test will always fail - because it won't compile. The code snippet in\n>> configure.ac translates to:\n>>\n>> return f);\n>>\n>> Either there's an extra ) or a missing (. A cast to int wouldn't hurt\n>> either.\n>\n> Oops. This is due to my 3adf9fdecf (configure.ac: loosen\n> FREAD_READS_DIRECTORIES test program, 2017-06-14).\n>\n> Neither I nor the original tester noticed, because we're on Linux, which\n> needs that set.\n\nI'm also running Linux, but somehow I ended up needing to check\nconfig.log and saw the error. I don't remember why I had to check.\n(I've been fixing the problem locally for the last few releases,\nfiguring someone else would notice. I also remove the 'rm configure'\nfrom the distclean target in Makefile, just in case I decide to\nrebuild later.)\n>\n>> I'd supply a patch to fix this, but I'm not sure what the results of\n>> suddenly fixing the test would be. It seems to work well on my\n>> machine, but I don't stress git much here, and it's just one machine.\n>\n> I think it should be fixed (and agreed on the \"int\" thing; the return\n> value should be \"f != NULL\" or similar).\n\nIronically, most of the time I check config.log is because of the\nmissing cast to int. A few years ago I set this machine up as a\ntri-arch system (x86, x86_64, and x32). Because of subtle errors\nthat appear when converting between int and pointer on x32, I\ntend to use -Werror=int-conversion on all my builds, and I wrap\nconfigure in a script that (among other things) checks config.log\nfor that particular error. Even though x32 isn't as promising\nanymore (thanks to a few projects rejecting support patches),\nthere are very few packages that still trigger the error.\n>\n> I don't know autoconf very well, but is there a way to invoke it that\n> will let us know if a test-program fails to compile at all? Obviously\n> for probing the system compler/includes, \"does not compile\" is an\n> expected possible outcome. But for tests like this it's really a\n> tristate: yes, no, or something went horribly wrong.\n\nI don't know if that's supported. From what I've seen in the docs I've\nfound online, it looks like the closest is a case for \"I couldn't run\nthe program because I'm cross-compiling.\" You might be able to\ndetermine that the compile and/or link failed by looking at some of\nautoconf's internal variables, but it would be fragile.\n>\n> -Peff\n>\n\nJonathan\n"},{"id":"344019","messageId":"20180406220736.GH7870@sigill.intra.peff.net","threadId":"48232","inReplyTo":"8ed8491f-9410-c509-5ef7-77638aa74038@gmail.com","subject":"Re: FREAD_READS_DIRECTORIES test fails for the wrong reasons","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2018-04-06T22:07:36Z","receivedAt":"2018-04-06T22:07:42Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Fri, Apr 06, 2018 at 05:57:15PM -0400, Jonathan Primrose wrote:\n\n> > Oops. This is due to my 3adf9fdecf (configure.ac: loosen\n> > FREAD_READS_DIRECTORIES test program, 2017-06-14).\n> > \n> > Neither I nor the original tester noticed, because we're on Linux, which\n> > needs that set.\n> \n> I'm also running Linux, but somehow I ended up needing to check\n> config.log and saw the error. I don't remember why I had to check.\n> (I've been fixing the problem locally for the last few releases,\n> figuring someone else would notice. I also remove the 'rm configure'\n> from the distclean target in Makefile, just in case I decide to\n> rebuild later.)\n\nI think you'll find that most of the developers don't actually use\nautoconf themselves, so bugs tend to go unnoticed there for a while.\n(The failure case for setting this flag accidentally is not bad, either;\nit just incurs an extra fstat).\n\n-Peff\n"}]}