threads / patch / 12488

patchConfigure test for FREAD_READS_DIRECTORIES

Subject: [PATCH] Configure test for FREAD_READS_DIRECTORIES

## tl;dr

7 messages between Mar 4, 2008 and Mar 5, 2008. Diffs are folded; open one to read it.

replies: 6people: 3as markdown or json

Michal Rokos· Mar 4, 2008, 09:48 UTC · lore

Hello,

this patch adds missing tests for FREAD_READS_DIRECTORIES.

Signed-off-by: Michal Rokos <michal.rokos@nextsoft.cz>

Show changes to 3 files +23 −0

Makefile, config.mak.in, configure.ac

diff --git a/Makefile b/Makefile
index ca5aad9..344ab49 100644
--- a/Makefile
+++ b/Makefile
@@ -526,6 +526,7 @@ ifeq ($(uname_S),HP-UX)
 	NO_UNSETENV = YesPlease
 	NO_HSTRERROR = YesPlease
 	NO_SYS_SELECT_H = YesPlease
+	FREAD_READS_DIRECTORIES = UnfortunatelyYes
 endif
 ifneq (,$(findstring arm,$(uname_M)))
 	ARM_SHA1 = YesPlease
diff --git a/config.mak.in b/config.mak.in
index ee6c33d..516c468 100644
--- a/config.mak.in
+++ b/config.mak.in
@@ -46,3 +46,4 @@ NO_MKDTEMP=@NO_MKDTEMP@
 NO_ICONV=@NO_ICONV@
 OLD_ICONV=@OLD_ICONV@
 NO_DEFLATE_BOUND=@NO_DEFLATE_BOUND@
+FREAD_READS_DIRECTORIES=@FREAD_READS_DIRECTORIES@
diff --git a/configure.ac b/configure.ac
index 85d7ef5..0ac28f6 100644
--- a/configure.ac
+++ b/configure.ac
@@ -326,6 +326,27 @@ else
 	NO_C99_FORMAT=
 fi
 AC_SUBST(NO_C99_FORMAT)
+#
+# Define FREAD_READS_DIRECTORIES if your are on a system which succeeds
+# when attempting to read from an fopen'ed directory.
+AC_CACHE_CHECK([whether system succeeds to read fopen'ed directory],
+ [ac_cv_fread_reads_directories],
+[
+AC_RUN_IFELSE(
+	[AC_LANG_PROGRAM([AC_INCLUDES_DEFAULT],
+		[[char c;
+		FILE *f = fopen("/etc", "r");
+		if (! f) return 0;
+		if (f && fread(&c, 1, 1, f) > 0) return 1]])],
+	[ac_cv_fread_reads_directories=no],
+	[ac_cv_fread_reads_directories=yes])
+])
+if test $ac_cv_fread_reads_directories = yes; then
+	FREAD_READS_DIRECTORIES=UnfortunatelyYes
+else
+	FREAD_READS_DIRECTORIES=
+fi
+AC_SUBST(FREAD_READS_DIRECTORIES)
 
 
 ## Checks for library functions.

Junio C Hamano· Mar 4, 2008, 11:03 UTC · re: Michal Rokos · lore

Re: [PATCH] Configure test for FREAD_READS_DIRECTORIES

Michal Rokos <michal.rokos@nextsoft.cz> writes:

> Hello,

"Hello," not wanted in the commit log message.

> this patch adds missing tests for FREAD_READS_DIRECTORIES.
>
> Signed-off-by: Michal Rokos <michal.rokos@nextsoft.cz>

Show 10 quoted lines

> +#
> +# Define FREAD_READS_DIRECTORIES if your are on a system which succeeds
> +# when attempting to read from an fopen'ed directory.
> +AC_CACHE_CHECK([whether system succeeds to read fopen'ed directory],
> + [ac_cv_fread_reads_directories],
> +[
> +AC_RUN_IFELSE(
> +	[AC_LANG_PROGRAM([AC_INCLUDES_DEFAULT],
> +		[[char c;
> +		FILE *f = fopen("/etc", "r");

Why "/etc" and not "." I have to wonder...

On how many different platforms was this configure check tested on?

Michal Rokos· Mar 4, 2008, 11:17 UTC · re: Junio C Hamano · lore

Re: [PATCH] Configure test for FREAD_READS_DIRECTORIES

Hello,

On Tuesday 04 March 2008 12:03:12 Junio C Hamano wrote:

Show 12 quoted lines

> > +#
> > +# Define FREAD_READS_DIRECTORIES if your are on a system which succeeds
> > +# when attempting to read from an fopen'ed directory.
> > +AC_CACHE_CHECK([whether system succeeds to read fopen'ed directory],
> > + [ac_cv_fread_reads_directories],
> > +[
> > +AC_RUN_IFELSE(
> > +	[AC_LANG_PROGRAM([AC_INCLUDES_DEFAULT],
> > +		[[char c;
> > +		FILE *f = fopen("/etc", "r");
>
> Why "/etc" and not "." I have to wonder...

I think "." is brilliant - ie no reason for "/etc" apart that I'm dumb.

> On how many different platforms was this configure check tested on?

Test works on Linux (no FREAD_READS_DIRECTORIES) and HPUXes (FREAD_READS_DIRECTORIES): HP-UX B.11.23 ia64 (Itanium) and HP-UX B.11.11 9000/800 (PaRisc)

Do you want me to resend with "."?

MR

-- 
Michal Rokos

NextSoft s.r.o.
Vyskočilova 1/1410
140 21 Praha 4
phone:  +420 267 224 311
fax:    +420 267 224 307
mobile: +420 736 646 591
e-mail: michal.rokos@nextsoft.cz

Junio C Hamano· Mar 4, 2008, 11:32 UTC · re: Michal Rokos · lore

Re: [PATCH] Configure test for FREAD_READS_DIRECTORIES

Michal Rokos <michal.rokos@nextsoft.cz> writes:

Show 7 quoted lines

>> On how many different platforms was this configure check tested on?
>
> Test works on Linux (no FREAD_READS_DIRECTORIES) and HPUXes 
> (FREAD_READS_DIRECTORIES): HP-UX B.11.23 ia64 (Itanium) and HP-UX B.11.11 
> 9000/800 (PaRisc)
>
> Do you want me to resend with "."?

Probably resending with "." and asking the list audiences for help in testing would help you gather success reports on different platforms.

Michal Rokos· Mar 4, 2008, 11:48 UTC · re: Junio C Hamano · lore

Re: [PATCH] Configure test for FREAD_READS_DIRECTORIES

Hello,

On Tuesday 04 March 2008 12:32:57 Junio C Hamano wrote:

Show 11 quoted lines

> Michal Rokos <michal.rokos@nextsoft.cz> writes:
> >> On how many different platforms was this configure check tested on?
> >
> > Test works on Linux (no FREAD_READS_DIRECTORIES) and HPUXes
> > (FREAD_READS_DIRECTORIES): HP-UX B.11.23 ia64 (Itanium) and HP-UX B.11.11
> > 9000/800 (PaRisc)
> >
> > Do you want me to resend with "."?
>
> Probably resending with "." and asking the list audiences for help in
> testing would help you gather success reports on different platforms.

Will do... Did that.

Do you think that there's some reason not-to merge it? I mean if fopen(".") throws an error, FREAD_READS_DIRECTORIES will NOT be defined - as is it now.

I don't know how many people cares about configure script since there are missing bits in it again and again. I believe it could receive good amount of testing only when it's merged in.

I'm trying to make GIT working on HPUX - next patch in my queue is about broken vsnprintf() that returns -1 on maxsize overrun. Do you think that it's more likely that patch will be accepted when I omit "broken vsnprintf()" detection code from configure.ac?

MR

-- 
Michal Rokos

NextSoft s.r.o.
Vyskočilova 1/1410
140 21 Praha 4
phone:  +420 267 224 311
fax:    +420 267 224 307
mobile: +420 736 646 591
e-mail: michal.rokos@nextsoft.cz

Jakub Narebski· Mar 4, 2008, 12:17 UTC · re: Michal Rokos · lore

Re: [PATCH] Configure test for FREAD_READS_DIRECTORIES

Michal Rokos <michal.rokos@nextsoft.cz> writes:

> I don't know how many people care about configure script since
> there are missing bits in it again and again. I believe it could
> receive good amount of testing only when it's merged in.

Because configure script is optional, people do tend to forget to add test to it, when adding new compile configuration option. Configuration is mainly done by guessing based on uname.

Unfortunately we don't have maintainer for configure script, who would catch new make configuration options, and add appropriate tests to ./configure.

> I'm trying to make GIT working on HPUX - next patch in my queue is
> about broken vsnprintf() that returns -1 on maxsize overrun. Do you
> think that it's more likely that patch will be accepted when I omit
> "broken vsnprintf()" detection code from configure.ac?

I think it would be better to split patch into two: one adding build option, or setting it for given operating system or operating system version, and one adding test to ./configure script. It is much simplier to test first patch; the patch to configure needs more review, as it should work correctly on all operating systems.

-- 
Jakub Narebski
Poland
ShadeHawk on #git

Junio C Hamano· Mar 5, 2008, 17:57 UTC · re: Michal Rokos · lore

Re: [PATCH] Configure test for FREAD_READS_DIRECTORIES

Michal Rokos <michal.rokos@nextsoft.cz> writes:

> Will do... Did that.
>
> Do you think that there's some reason not-to merge it?

Yes, if you meant "apply as-is" by "merge it". No, if you meant "apply after an initial round of sanity checks, even if it is not perfect".

I was hoping that with this approach, in a week after you sent out your call-for-help-in-testing, you could send a version for inclusion with a commit log message that says "tested on X (by Foo), Y (by Bar),...", with the patch text that is exactly the same as what people tested. The point is not to make that list of platforms exhaustive, but at least make it a bit more than "works for me".

And I think that plan has worked well.