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

Brandon Casey [off-list ref] writes:

Mike Ralphson wrote:

quoted

2008/5/7 Brandon Casey [off-list ref]:

quoted

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.

2008/5/7 Junio C Hamano [off-list ref]:

Brandon Casey [off-list ref] writes:

 > Mike Ralphson wrote:
 >> 2008/5/7 Brandon Casey [off-list ref]:
 >>> 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

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 <redacted>
---
 compat/fopen.c |   13 ++++++++++++-
 1 files changed, 12 insertions(+), 1 deletions(-)

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 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

2008/5/7 Brandon Casey [off-list ref]:

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 <redacted>

Tested-by: Mike Ralphson <redacted>

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

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

2008/5/8 Johannes Sixt [off-list ref]:

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

On Thu, 8 May 2008 08:27:48 +0100, "Mike Ralphson"
[off-list ref] wrote:

2008/5/7 Brandon Casey [off-list ref]:

quoted

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 <redacted>

Tested-by: Mike Ralphson <redacted>

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/