threads / patch / 13411

patchMakefile: update the default build options for AIX

Subject: [PATCH] Makefile: update the default build options for AIX

## tl;dr

20 messages between May 7, 2008 and May 16, 2008. Diffs are folded; open one to read it.

replies: 19people: 6as markdown or json

Mike Ralphson· May 7, 2008, 08:35 UTC · lore

NO_MKDTEMP is required to build, FREAD_READS_DIRECTORIES and the definition of _LARGE_FILES fix test suite failures and INTERNAL_QSORT is required for adequate performance.

Tested on AIX v5.3 Maintenance Level 06
Signed-off-by: Mike Ralphson <mike@abacus.co.uk>
---
 Makefile |    4 ++++
 1 files changed, 4 insertions(+), 0 deletions(-)
Show changes to Makefile +4 −0
diff --git a/Makefile b/Makefile
index 7c70b00..4296656 100644
--- a/Makefile
+++ b/Makefile
@@ -632,8 +632,12 @@ endif
 ifeq ($(uname_S),AIX)
 	NO_STRCASESTR=YesPlease
 	NO_MEMMEM = YesPlease
+	NO_MKDTEMP = YesPlease
 	NO_STRLCPY = YesPlease
+	FREAD_READS_DIRECTORIES = UnfortunatelyYes
+	INTERNAL_QSORT = UnfortunatelyYes
 	NEEDS_LIBICONV=YesPlease
+	BASIC_CFLAGS += -D_LARGE_FILES
 endif
 ifeq ($(uname_S),GNU)
 	# GNU/Hurd

-- 
1.5.5.1.dirty
Johannes Sixt· May 7, 2008, 11:57 UTC · re: Mike Ralphson · lore

Re: [PATCH] Makefile: update the default build options for AIX

Mike Ralphson schrieb:
Show 28 quoted lines
> NO_MKDTEMP is required to build, FREAD_READS_DIRECTORIES and the definition
> of _LARGE_FILES fix test suite failures and INTERNAL_QSORT is required for
> adequate performance.
> 
> Tested on AIX v5.3 Maintenance Level 06
> 
> Signed-off-by: Mike Ralphson <mike@abacus.co.uk>
> ---
>  Makefile |    4 ++++
>  1 files changed, 4 insertions(+), 0 deletions(-)
> 
> diff --git a/Makefile b/Makefile
> index 7c70b00..4296656 100644
> --- a/Makefile
> +++ b/Makefile
> @@ -632,8 +632,12 @@ endif
>  ifeq ($(uname_S),AIX)
>  	NO_STRCASESTR=YesPlease
>  	NO_MEMMEM = YesPlease
> +	NO_MKDTEMP = YesPlease
>  	NO_STRLCPY = YesPlease
> +	FREAD_READS_DIRECTORIES = UnfortunatelyYes
> +	INTERNAL_QSORT = UnfortunatelyYes
>  	NEEDS_LIBICONV=YesPlease
> +	BASIC_CFLAGS += -D_LARGE_FILES
>  endif
>  ifeq ($(uname_S),GNU)
>  	# GNU/Hurd
I'm trying this patch on AIX 4.3.3 (sigh!) with gcc3. I get this:
git-compat-util.h:209:1: warning: "fopen" redefined
In file included from git-compat-util.h:51,
                 from builtin.h:4,
                 from git.c:1:
/usr/local/lib/gcc-lib/powerpc-ibm-aix4.3.2.0/3.2.1/include/stdio.h:110:1:
warning: this is the location of the previous definition

Line 110 in ...include/stdio.h is inside a #ifdef _LARGE_FILES section and says:

#define fopen fopen64

Did you also get this warning? Is _LARGE_FILES support solved in a different way on 5.3?

-- Hannes
Mike Ralphson· May 7, 2008, 12:51 UTC · re: Johannes Sixt · lore

Re: [PATCH] Makefile: update the default build options for AIX

2008/5/7 Johannes Sixt <j.sixt@viscovery.net>:
Show 17 quoted lines
>
>  I'm trying this patch on AIX 4.3.3 (sigh!) with gcc3. I get this:
>
>  git-compat-util.h:209:1: warning: "fopen" redefined
>  In file included from git-compat-util.h:51,
>                  from builtin.h:4,
>                  from git.c:1:
>  /usr/local/lib/gcc-lib/powerpc-ibm-aix4.3.2.0/3.2.1/include/stdio.h:110:1:
>  warning: this is the location of the previous definition
>
>  Line 110 in ...include/stdio.h is inside a #ifdef _LARGE_FILES section and
>  says:
>
>  #define fopen fopen64
>
>  Did you also get this warning? Is _LARGE_FILES support solved in a
>  different way on 5.3?

The warning (I get rather a lot of them) is caused by the compat/fopen.c included when FREAD_READS_DIRECTORIES is defined. I tried moving the #undef fopen to git-compat-util.h but that resulted in a broken build and me reaching the end of my limited ability with c.

In file included from cache.h:4,
                 from daemon.c:1:
git-compat-util.h:209:1: warning: "fopen" redefined
In file included from git-compat-util.h:51,
                 from cache.h:4,
                 from daemon.c:1:
/opt/freeware/lib/gcc-lib/powerpc-ibm-aix5.3.0.0/3.3.2/include/stdio.h:110:1:
warning: this is the location of the previous definition
The warnings are harmless, though untidy.

I don't believe it's anything to do with _LARGE_FILES. Could you try building first with one commented out, then the other? I don't think I have access to a 4.3.3 box any more.

Mike
Mike Ralphson· May 7, 2008, 13:05 UTC · re: Mike Ralphson · lore

Re: [PATCH] Makefile: update the default build options for AIX

2008/5/7 Mike Ralphson <mike.ralphson@gmail.com>:
Show 8 quoted lines
> 2008/5/7 Johannes Sixt <j.sixt@viscovery.net>:
>  >
>  >  Did you also get this warning? Is _LARGE_FILES support solved in a
>  >  different way on 5.3?
>
>  I don't believe it's anything to do with _LARGE_FILES. Could you try
>  building first with one commented out, then the other? I don't think I
>  have access to a 4.3.3 box any more.

I'm full of it (and I didn't try my own suggestion). It does appear to be related to defining _LARGE_FILES.

I'm afraid I can't see how the current #undef is working, let alone suggest how to fix it when fopen is already redefined. 8-(

Mike
Johannes Sixt· May 7, 2008, 13:14 UTC · re: Mike Ralphson · lore

Re: [PATCH] Makefile: update the default build options for AIX

Mike Ralphson schrieb:
Show 32 quoted lines
> 2008/5/7 Johannes Sixt <j.sixt@viscovery.net>:
>>  I'm trying this patch on AIX 4.3.3 (sigh!) with gcc3. I get this:
>>
>>  git-compat-util.h:209:1: warning: "fopen" redefined
>>  In file included from git-compat-util.h:51,
>>                  from builtin.h:4,
>>                  from git.c:1:
>>  /usr/local/lib/gcc-lib/powerpc-ibm-aix4.3.2.0/3.2.1/include/stdio.h:110:1:
>>  warning: this is the location of the previous definition
>>
>>  Line 110 in ...include/stdio.h is inside a #ifdef _LARGE_FILES section and
>>  says:
>>
>>  #define fopen fopen64
>>
>>  Did you also get this warning? Is _LARGE_FILES support solved in a
>>  different way on 5.3?
> 
> The warning (I get rather a lot of them) is caused by the
> compat/fopen.c included when FREAD_READS_DIRECTORIES is defined. I
> tried moving the #undef fopen to git-compat-util.h but that resulted
> in a broken build and me reaching the end of my limited ability with
> c.
> 
> In file included from cache.h:4,
>                  from daemon.c:1:
> git-compat-util.h:209:1: warning: "fopen" redefined
> In file included from git-compat-util.h:51,
>                  from cache.h:4,
>                  from daemon.c:1:
> /opt/freeware/lib/gcc-lib/powerpc-ibm-aix5.3.0.0/3.3.2/include/stdio.h:110:1:
> warning: this is the location of the previous definition
So you we in the same boat.
> The warnings are harmless, though untidy.
> I don't believe it's anything to do with _LARGE_FILES. Could you try
> building first with one commented out, then the other? I don't think I
> have access to a 4.3.3 box any more.
Untidy, yes; harmless: not necessarily. It has a lot to do with _LARGE_FILES.

The #define fopen in git-compat-util.h essentially defeats the effect of _LARGE_FILES as far as fopen() calls are concerned: If FREAD_READS_DIRECTORIES is not defined, fopen() would be redirected to fopen64(), but when it is defined, it is redirected to git_fopen(), which in turn uses fopen() instead of fopen64() (due to the #undef in compat/fopen.c).

This might be dangerous if some other function of the f*64() family uses the FILE* that the fopen() call returned. I don't know if there is such a usage pattern somewhere in git.

Why did you need _LARGE_FILES in the first place?
-- Hannes
Mike Ralphson· May 7, 2008, 14:20 UTC · re: Johannes Sixt · lore

Re: [PATCH] Makefile: update the default build options for AIX

2008/5/7 Johannes Sixt <j.sixt@viscovery.net>:
Show 24 quoted lines
> Mike Ralphson schrieb:
>
> > 2008/5/7 Johannes Sixt <j.sixt@viscovery.net>:
>  So you we in the same boat.
>
>  > The warnings are harmless, though untidy.
>  > I don't believe it's anything to do with _LARGE_FILES. Could you try
>  > building first with one commented out, then the other? I don't think I
>  > have access to a 4.3.3 box any more.
>
>  Untidy, yes; harmless: not necessarily. It has a lot to do with _LARGE_FILES.
>
>  The #define fopen in git-compat-util.h essentially defeats the effect of
>  _LARGE_FILES as far as fopen() calls are concerned: If
>  FREAD_READS_DIRECTORIES is not defined, fopen() would be redirected to
>  fopen64(), but when it is defined, it is redirected to git_fopen(), which
>  in turn uses fopen() instead of fopen64() (due to the #undef in
>  compat/fopen.c).
>
>  This might be dangerous if some other function of the f*64() family uses
>  the FILE* that the fopen() call returned. I don't know if there is such a
>  usage pattern somewhere in git.
>
>  Why did you need _LARGE_FILES in the first place?
Welcome aboard!
I was seeing test failures in t5302-pack-index.sh

Specifically tests 9 and 20 # 9: index v2: force some 64-bit offsets with pack-objects # 20: create a stealth corruption in a delta base reference

These tests seem to be skipped these days if off_t isn't large enough, I preferred the passing tests to the skipped ones. Maybe it isn't an issue in the real world.

Mike
Brandon Casey· May 7, 2008, 14:38 UTC · re: Johannes Sixt · lore

Re: [PATCH] Makefile: update the default build options for AIX

Johannes Sixt wrote:
Show 7 quoted lines
> The #define fopen in git-compat-util.h essentially defeats the effect of
> _LARGE_FILES as far as fopen() calls are concerned: If
> FREAD_READS_DIRECTORIES is not defined, fopen() would be redirected to
> fopen64(), but when it is defined, it is redirected to git_fopen(), which
> in turn uses fopen() instead of fopen64() (due to the #undef in
> compat/fopen.c).
> 
How about something like this?
Show changes to compat/fopen.c +1 −2
diff --git a/compat/fopen.c b/compat/fopen.c
index ccb9e89..70b0d4d 100644
--- a/compat/fopen.c
+++ b/compat/fopen.c
@@ -1,5 +1,5 @@
+#undef FREAD_READS_DIRECTORIES
 #include "../git-compat-util.h"
-#undef fopen
 FILE *git_fopen(const char *path, const char *mode)
 {
        FILE *fp;


-brandon
Mike Ralphson· May 7, 2008, 15:15 UTC · re: Brandon Casey · lore

Re: [PATCH] Makefile: update the default build options for AIX

2008/5/7 Brandon Casey <casey@nrlssc.navy.mil>:
Show 26 quoted lines
> Johannes Sixt wrote:
>  > The #define fopen in git-compat-util.h essentially defeats the effect of
>  > _LARGE_FILES as far as fopen() calls are concerned: If
>  > FREAD_READS_DIRECTORIES is not defined, fopen() would be redirected to
>  > fopen64(), but when it is defined, it is redirected to git_fopen(), which
>  > in turn uses fopen() instead of fopen64() (due to the #undef in
>  > compat/fopen.c).
>  >
>
>  How about something like this?
>
>  diff --git a/compat/fopen.c b/compat/fopen.c
>  index ccb9e89..70b0d4d 100644
>  --- a/compat/fopen.c
>  +++ b/compat/fopen.c
>  @@ -1,5 +1,5 @@
>  +#undef FREAD_READS_DIRECTORIES
>   #include "../git-compat-util.h"
>  -#undef fopen
>   FILE *git_fopen(const char *path, const char *mode)
>   {
>         FILE *fp;
>
>
>  -brandon
>

Ta. I still get all the warnings with that, was that what you were trying to solve? The 64 bit specific tests in t5302 do still pass.

Mike
Johannes Sixt· May 7, 2008, 15:39 UTC · re: Mike Ralphson · lore

Re: [PATCH] Makefile: update the default build options for AIX

Mike Ralphson schrieb:
Show 21 quoted lines
> 2008/5/7 Brandon Casey <casey@nrlssc.navy.mil>:
>>  How about something like this?
>>
>>  diff --git a/compat/fopen.c b/compat/fopen.c
>>  index ccb9e89..70b0d4d 100644
>>  --- a/compat/fopen.c
>>  +++ b/compat/fopen.c
>>  @@ -1,5 +1,5 @@
>>  +#undef FREAD_READS_DIRECTORIES
>>   #include "../git-compat-util.h"
>>  -#undef fopen
>>   FILE *git_fopen(const char *path, const char *mode)
>>   {
>>         FILE *fp;
>>
>>
>>  -brandon
>>
> 
> Ta. I still get all the warnings with that, was that what you were
> trying to solve? The 64 bit specific tests in t5302 do still pass.

You should get one less warning (the one from compat/fopen.c), but this time they *are* harmless. ;)

-- Hannes
Brandon Casey· May 7, 2008, 15:40 UTC · re: Mike Ralphson · lore

Re: [PATCH] Makefile: update the default build options for AIX

Mike Ralphson wrote:
Show 30 quoted lines
> 2008/5/7 Brandon Casey <casey@nrlssc.navy.mil>:
>> Johannes Sixt wrote:
>>  > The #define fopen in git-compat-util.h essentially defeats the effect of
>>  > _LARGE_FILES as far as fopen() calls are concerned: If
>>  > FREAD_READS_DIRECTORIES is not defined, fopen() would be redirected to
>>  > fopen64(), but when it is defined, it is redirected to git_fopen(), which
>>  > in turn uses fopen() instead of fopen64() (due to the #undef in
>>  > compat/fopen.c).
>>  >
>>
>>  How about something like this?
>>
>>  diff --git a/compat/fopen.c b/compat/fopen.c
>>  index ccb9e89..70b0d4d 100644
>>  --- a/compat/fopen.c
>>  +++ b/compat/fopen.c
>>  @@ -1,5 +1,5 @@
>>  +#undef FREAD_READS_DIRECTORIES
>>   #include "../git-compat-util.h"
>>  -#undef fopen
>>   FILE *git_fopen(const char *path, const char *mode)
>>   {
>>         FILE *fp;
>>
>>
>>  -brandon
>>
> 
> Ta. I still get all the warnings with that, was that what you were
> trying to solve? The 64 bit specific tests in t5302 do still pass.

Ah, yes. You would still get the warnings for every other file that includes git-compat-util.h, except compat/fopen.c. I didn't think about all of those. :) In this case those are indeed harmless. And now the git provided git_fopen() will use the compiler selected fopen() which should avoid any of the gotchas that Hannes brought up.

-brandon
Junio C Hamano· May 7, 2008, 16:04 UTC · re: Brandon Casey · lore

Re: [PATCH] Makefile: update the default build options for AIX

Brandon Casey <casey@nrlssc.navy.mil> writes:
Show 37 quoted lines
> Mike Ralphson wrote:
>> 2008/5/7 Brandon Casey <casey@nrlssc.navy.mil>:
>>> Johannes Sixt wrote:
>>>  > The #define fopen in git-compat-util.h essentially defeats the effect of
>>>  > _LARGE_FILES as far as fopen() calls are concerned: If
>>>  > FREAD_READS_DIRECTORIES is not defined, fopen() would be redirected to
>>>  > fopen64(), but when it is defined, it is redirected to git_fopen(), which
>>>  > in turn uses fopen() instead of fopen64() (due to the #undef in
>>>  > compat/fopen.c).
>>>  >
>>>
>>>  How about something like this?
>>>
>>>  diff --git a/compat/fopen.c b/compat/fopen.c
>>>  index ccb9e89..70b0d4d 100644
>>>  --- a/compat/fopen.c
>>>  +++ b/compat/fopen.c
>>>  @@ -1,5 +1,5 @@
>>>  +#undef FREAD_READS_DIRECTORIES
>>>   #include "../git-compat-util.h"
>>>  -#undef fopen
>>>   FILE *git_fopen(const char *path, const char *mode)
>>>   {
>>>         FILE *fp;
>>>
>>>
>>>  -brandon
>>>
>> 
>> Ta. I still get all the warnings with that, was that what you were
>> trying to solve? The 64 bit specific tests in t5302 do still pass.
>
> Ah, yes. You would still get the warnings for every other file that
> includes git-compat-util.h, except compat/fopen.c. I didn't think
> about all of those. :) In this case those are indeed harmless. And now
> the git provided git_fopen() will use the compiler selected fopen()
> which should avoid any of the gotchas that Hannes brought up.

In any case, that #undef then #include dance needs a big comment on why it has to be so.

Mike Ralphson· May 7, 2008, 16:20 UTC · re: Junio C Hamano · lore

Re: [PATCH] Makefile: update the default build options for AIX

2008/5/7 Junio C Hamano <gitster@pobox.com>:
Show 44 quoted lines
>
> Brandon Casey <casey@nrlssc.navy.mil> writes:
>
>  > Mike Ralphson wrote:
>  >> 2008/5/7 Brandon Casey <casey@nrlssc.navy.mil>:
>  >>> Johannes Sixt wrote:
>  >>>  > The #define fopen in git-compat-util.h essentially defeats the effect of
>  >>>  > _LARGE_FILES as far as fopen() calls are concerned: If
>  >>>  > FREAD_READS_DIRECTORIES is not defined, fopen() would be redirected to
>  >>>  > fopen64(), but when it is defined, it is redirected to git_fopen(), which
>  >>>  > in turn uses fopen() instead of fopen64() (due to the #undef in
>  >>>  > compat/fopen.c).
>  >>>  >
>  >>>
>  >>>  How about something like this?
>  >>>
>  >>>  diff --git a/compat/fopen.c b/compat/fopen.c
>  >>>  index ccb9e89..70b0d4d 100644
>  >>>  --- a/compat/fopen.c
>  >>>  +++ b/compat/fopen.c
>  >>>  @@ -1,5 +1,5 @@
>  >>>  +#undef FREAD_READS_DIRECTORIES
>  >>>   #include "../git-compat-util.h"
>  >>>  -#undef fopen
>  >>>   FILE *git_fopen(const char *path, const char *mode)
>  >>>   {
>  >>>         FILE *fp;
>  >>>
>  >>>
>  >>>  -brandon
>  >>>
>  >>
>  >> Ta. I still get all the warnings with that, was that what you were
>  >> trying to solve? The 64 bit specific tests in t5302 do still pass.
>  >
>  > Ah, yes. You would still get the warnings for every other file that
>  > includes git-compat-util.h, except compat/fopen.c. I didn't think
>  > about all of those. :) In this case those are indeed harmless. And now
>  > the git provided git_fopen() will use the compiler selected fopen()
>  > which should avoid any of the gotchas that Hannes brought up.
>
>  In any case, that #undef then #include dance needs a big comment on why it
>  has to be so.
>

Indeed. Please add ascii-art diagrams and don't use long words. I may then have a chance of understanding how this works, and how I should have spotted 5 potentially non-harmless warnings among 400 noise ones, when all I did was get the testsuite from non-passing to passing! 8-)

In reality, thanks to all for pitching in.
Mike
Brandon Casey· May 7, 2008, 17:36 UTC · re: Mike Ralphson · lore

Re: [PATCH] Makefile: update the default build options for AIX

Mike Ralphson wrote:
> Indeed. Please add ascii-art diagrams and don't use long words. I may
> then have a chance of understanding how this works,
I think this is simpler than you are making it out to be.

All the git source files currently #include git-compat-util.h. When a platform is missing a function, we implement that function in the compat/ subdirectory and add an entry for it in git-compat-util.h.

In this case we found a problem that could be worked around by replacing every call to fopen with an internal function. So we did the standard thing of creating a new function in the compat/ subdirectory named git_fopen() and added macro statements within git-compat-util.h to redefine fopen to be git_fopen. But, git_fopen needs to call the _real_ fopen and it _also_ includes git-compat-util.h. So, after including git-compat-util.h, we undefined the fopen macro to undo the assignment that we had just performed. This doesn't work if the system is also setting an fopen macro. So the fix is to avoid clobbering the system setting at all when compiling compat/fopen.c

-brandon
Brandon Casey· May 7, 2008, 17:34 UTC · re: Junio C Hamano · lore

[PATCH] compat/fopen.c: avoid clobbering the system defined fopen macro

Some systems define fopen as a macro based on compiler settings. The previous technique for reverting to the system fopen function by merely undefining fopen is inadequate in this case. Instead, avoid defining fopen entirely when compiling this source file.

Signed-off-by: Brandon Casey <casey@nrlssc.navy.mil>
---
 compat/fopen.c |   13 ++++++++++++-
 1 files changed, 12 insertions(+), 1 deletions(-)
Show changes to compat/fopen.c +12 −1
diff --git a/compat/fopen.c b/compat/fopen.c
index ccb9e89..b5ca142 100644
--- a/compat/fopen.c
+++ b/compat/fopen.c
@@ -1,5 +1,16 @@
+/*
+ *  The order of the following two lines is important.
+ *
+ *  FREAD_READS_DIRECTORIES is undefined before including git-compat-util.h
+ *  to avoid the redefinition of fopen within git-compat-util.h. This is
+ *  necessary since fopen is a macro on some platforms which may be set
+ *  based on compiler options. For example, on AIX fopen is set to fopen64
+ *  when _LARGE_FILES is defined. The previous technique of merely undefining
+ *  fopen after including git-compat-util.h is inadequate in this case.
+ */
+#undef FREAD_READS_DIRECTORIES
 #include "../git-compat-util.h"
-#undef fopen
+
 FILE *git_fopen(const char *path, const char *mode)
 {
 	FILE *fp;

-- 
1.5.5
Mike Ralphson· May 8, 2008, 07:27 UTC · re: Brandon Casey · lore

Re: [PATCH] compat/fopen.c: avoid clobbering the system defined fopen macro

2008/5/7 Brandon Casey <casey@nrlssc.navy.mil>:
Show 8 quoted lines
>
> Some systems define fopen as a macro based on compiler settings.
> The previous technique for reverting to the system fopen function
> by merely undefining fopen is inadequate in this case. Instead,
> avoid defining fopen entirely when compiling this source file.
>
> Signed-off-by: Brandon Casey <casey@nrlssc.navy.mil>
>
Tested-by: Mike Ralphson <mike@abacus.co.uk>
Both with and without -D_LARGE_FILES. Many thanks.
H.Merijn, is this change also ok for your HP-UX?

I guess there may still be a case for not defining _LARGE_FILES by default on AIX as all the warnings may be off-putting or mask other issues. Maybe instead having a comment for those who need large pack-file support? Will submit amended Makefile patch if there's interest.

Mike
Johannes Sixt· May 8, 2008, 07:34 UTC · re: Mike Ralphson · lore

Re: [PATCH] compat/fopen.c: avoid clobbering the system defined fopen macro

Mike Ralphson schrieb:
Show 5 quoted lines
> I guess there may still be a case for not defining _LARGE_FILES by
> default on AIX as all the warnings may be off-putting or mask other
> issues. Maybe instead having a comment for those who need large
> pack-file support? Will submit amended Makefile patch if there's
> interest.

Since with this patch we are treating fopen specially anyway, we could go one step further and do this, too: ---

Show changes to git-compat-util.h +3 −0
diff --git a/git-compat-util.h b/git-compat-util.h
index b2708f3..dad4d48 100644
--- a/git-compat-util.h
+++ b/git-compat-util.h
@@ -230,6 +230,9 @@ void *gitmemmem(const void *haystack,
 #endif

 #ifdef FREAD_READS_DIRECTORIES
+#ifdef fopen
+#undef fopen
+#endif
 #define fopen(a,b) git_fopen(a,b)
 extern FILE *git_fopen(const char*, const char*);
 #endif
Mike Ralphson· May 8, 2008, 07:59 UTC · re: Johannes Sixt · lore

Re: [PATCH] compat/fopen.c: avoid clobbering the system defined fopen macro

2008/5/8 Johannes Sixt <j.sixt@viscovery.net>:
Show 25 quoted lines
> Mike Ralphson schrieb:
>> I guess there may still be a case for not defining _LARGE_FILES by
>> default on AIX as all the warnings may be off-putting or mask other
>> issues. Maybe instead having a comment for those who need large
>> pack-file support? Will submit amended Makefile patch if there's
>> interest.
>
> Since with this patch we are treating fopen specially anyway, we could go
> one step further and do this, too:
> ---
> diff --git a/git-compat-util.h b/git-compat-util.h
> index b2708f3..dad4d48 100644
> --- a/git-compat-util.h
> +++ b/git-compat-util.h
> @@ -230,6 +230,9 @@ void *gitmemmem(const void *haystack,
>  #endif
>
>  #ifdef FREAD_READS_DIRECTORIES
> +#ifdef fopen
> +#undef fopen
> +#endif
>  #define fopen(a,b) git_fopen(a,b)
>  extern FILE *git_fopen(const char*, const char*);
>  #endif
>

Loving your work! Squashes all the related warnings, re-tested etc. Technically, is the #ifdef / #endif actually required? Or is #undef'ing an undefined macro not portable? I agree it aids clarity for no cost.

Mike
H.Merijn Brand· May 8, 2008, 10:37 UTC · re: Mike Ralphson · lore

Re: [PATCH] compat/fopen.c: avoid clobbering the system defined fopen macro

On Thu, 8 May 2008 08:27:48 +0100, "Mike Ralphson" <mike.ralphson@gmail.com> wrote:

Show 15 quoted lines
> 2008/5/7 Brandon Casey <casey@nrlssc.navy.mil>:
> >
> > Some systems define fopen as a macro based on compiler settings.
> > The previous technique for reverting to the system fopen function
> > by merely undefining fopen is inadequate in this case. Instead,
> > avoid defining fopen entirely when compiling this source file.
> >
> > Signed-off-by: Brandon Casey <casey@nrlssc.navy.mil>
> >
> 
> Tested-by: Mike Ralphson <mike@abacus.co.uk>
> 
> Both with and without -D_LARGE_FILES. Many thanks.
> 
> H.Merijn, is this change also ok for your HP-UX?

I'm not really actively following the ML anymore, as it is kinda busy :) I was able to compile/test/install 1.5.5.1 on HP-UX 11.00/32 and on HP-UX 11.23-ilp64 with a reasonable small set of additional changes.

Main thing I found to make most tests that used to fail now pass is to use bash, instead of HP's native POSIX shell.

http://www.xs4all.nl/~procura/git-1.5.5.1-11.00.diff http://www.xs4all.nl/~procura/git-1.5.5.1-11.23.diff

We - as a company - now actively use git on HP-UX 11.00 and Linux
> I guess there may still be a case for not defining _LARGE_FILES by
> default on AIX as all the warnings may be off-putting or mask other
> issues.

I also have AIX, but I hate it, and don't really care about it. It's just that we have some poor customers whose IT people forced this OS upon them.

> Maybe instead having a comment for those who need large pack-file
> support? Will submit amended Makefile patch if there's interest.
-- 
H.Merijn Brand         Amsterdam Perl Mongers (http://amsterdam.pm.org/)
using & porting perl 5.6.2, 5.8.x, 5.10.x  on HP-UX 10.20, 11.00, 11.11,
& 11.23, SuSE 10.1 & 10.2, AIX 5.2, and Cygwin.       http://qa.perl.org
http://mirrors.develooper.com/hpux/            http://www.test-smoke.org
                        http://www.goldmark.org/jeff/stupid-disclaimers/
Mike Ralphson· May 16, 2008, 10:19 UTC · re: Johannes Sixt · lore

Re: [PATCH] Makefile: update the default build options for AIX

2008/5/7 Johannes Sixt <j.sixt@viscovery.net>:
Show 6 quoted lines
> Mike Ralphson schrieb:
>> NO_MKDTEMP is required to build, FREAD_READS_DIRECTORIES and the definition
>> of _LARGE_FILES fix test suite failures and INTERNAL_QSORT is required for
>> adequate performance.
>
> I'm trying this patch on AIX 4.3.3 (sigh!) with gcc3...

Now the interaction between FREAD_READS_DIRECTORIES and _LARGE_FILES has been sorted out, and the wt-status.h warning fix is also in, did you manage to finish testing this? The INTERNAL_QSORT gave me a 2 orders of magnitude speed up on git status / commit etc.

I should have mentioned that I build with SHELL_PATH = /bin/bash and ensure that /usr/linux/bin (or /opt/freeware/bin) from the AIX toolbox is prepended to the PATH to run the test-suite. I didn't want to fold these into the patch as the paths are somewhat environment specific.

Mike
Johannes Sixt· May 16, 2008, 13:37 UTC · re: Mike Ralphson · lore

Re: [PATCH] Makefile: update the default build options for AIX

Mike Ralphson schrieb:
Show 17 quoted lines
> 2008/5/7 Johannes Sixt <j.sixt@viscovery.net>:
>> Mike Ralphson schrieb:
>>> NO_MKDTEMP is required to build, FREAD_READS_DIRECTORIES and the definition
>>> of _LARGE_FILES fix test suite failures and INTERNAL_QSORT is required for
>>> adequate performance.
>> I'm trying this patch on AIX 4.3.3 (sigh!) with gcc3...
> 
> Now the interaction between FREAD_READS_DIRECTORIES and _LARGE_FILES
> has been sorted out, and the
> wt-status.h warning fix is also in, did you manage to finish testing
> this? The INTERNAL_QSORT gave me a 2 orders of magnitude speed up on
> git status / commit etc.
> 
> I should have mentioned that I build with SHELL_PATH = /bin/bash and
> ensure that /usr/linux/bin (or /opt/freeware/bin) from the AIX toolbox
> is prepended to the PATH to run the test-suite. I didn't want to fold
> these into the patch as the paths are somewhat environment specific.

I compiled git on AIX 4.3.3. Even though the testsuite does not pass (due to a too old perl, non-working libiconv, and a sed that does not print the last line if there's no LF at the end), there's no failure that I would attribute to your changes. Therefore, I'd say:

Tested-by: Johannes Sixt <johannes.sixt@telecom.at>
-- Hannes

← back to recent threads