{"thread":{"id":"13411","subject":"[PATCH] Makefile: update the default build options for AIX","startedAt":"2008-05-07T08:35:55Z","lastAt":"2008-05-16T13:37:11Z","messageCount":20,"participants":["Mike Ralphson","Johannes Sixt","Brandon Casey","Junio C Hamano","H.Merijn Brand"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"76258","messageId":"1210149355875-git-send-email-mike@abacus.co.uk","threadId":"13411","inReplyTo":null,"subject":"[PATCH] Makefile: update the default build options for AIX","fromName":"Mike Ralphson","fromEmail":"mike@abacus.co.uk","sentAt":"2008-05-07T08:35:55Z","receivedAt":"2008-05-07T08:35:55Z","isPatch":true,"sender":{"key":"mike@abacus.co.uk","avatar":"https://gravatar.com/avatar/717865f1e9a9197ec082b9f6a12e048210a9da06878e47f165b470c43a9e9175?d=mp&s=160"},"body":"NO_MKDTEMP is required to build, FREAD_READS_DIRECTORIES and the definition\nof _LARGE_FILES fix test suite failures and INTERNAL_QSORT is required for\nadequate performance.\n\nTested on AIX v5.3 Maintenance Level 06\n\nSigned-off-by: Mike Ralphson \n---\n Makefile | 4 ++++\n 1 files changed, 4 insertions(+), 0 deletions(-)\n\ndiff --git a/Makefile b/Makefile\nindex 7c70b00..4296656 100644\n--- a/Makefile\n+++ b/Makefile\n@@ -632,8 +632,12 @@ endif\n ifeq ($(uname_S),AIX)\n \tNO_STRCASESTR=YesPlease\n \tNO_MEMMEM = YesPlease\n+\tNO_MKDTEMP = YesPlease\n \tNO_STRLCPY = YesPlease\n+\tFREAD_READS_DIRECTORIES = UnfortunatelyYes\n+\tINTERNAL_QSORT = UnfortunatelyYes\n \tNEEDS_LIBICONV=YesPlease\n+\tBASIC_CFLAGS += -D_LARGE_FILES\n endif\n ifeq ($(uname_S),GNU)\n \t# GNU/Hurd\n-- \n1.5.5.1.dirty\n"},{"id":"76271","messageId":"4821992F.4060201@viscovery.net","threadId":"13411","inReplyTo":"1210149355875-git-send-email-mike@abacus.co.uk","subject":"Re: [PATCH] Makefile: update the default build options for AIX","fromName":"Johannes Sixt","fromEmail":"j.sixt@viscovery.net","sentAt":"2008-05-07T11:57:35Z","receivedAt":"2008-05-07T11:57:35Z","isPatch":true,"sender":{"key":"j6t@kdbg.org","avatar":"https://avatars.githubusercontent.com/u/14810926?v=4"},"body":"Mike Ralphson schrieb:\n> NO_MKDTEMP is required to build, FREAD_READS_DIRECTORIES and the definition\n> of _LARGE_FILES fix test suite failures and INTERNAL_QSORT is required for\n> adequate performance.\n> \n> Tested on AIX v5.3 Maintenance Level 06\n> \n> Signed-off-by: Mike Ralphson \n> ---\n> Makefile | 4 ++++\n> 1 files changed, 4 insertions(+), 0 deletions(-)\n> \n> diff --git a/Makefile b/Makefile\n> index 7c70b00..4296656 100644\n> --- a/Makefile\n> +++ b/Makefile\n> @@ -632,8 +632,12 @@ endif\n> ifeq ($(uname_S),AIX)\n> \tNO_STRCASESTR=YesPlease\n> \tNO_MEMMEM = YesPlease\n> +\tNO_MKDTEMP = YesPlease\n> \tNO_STRLCPY = YesPlease\n> +\tFREAD_READS_DIRECTORIES = UnfortunatelyYes\n> +\tINTERNAL_QSORT = UnfortunatelyYes\n> \tNEEDS_LIBICONV=YesPlease\n> +\tBASIC_CFLAGS += -D_LARGE_FILES\n> endif\n> ifeq ($(uname_S),GNU)\n> \t# GNU/Hurd\n\nI'm trying this patch on AIX 4.3.3 (sigh!) with gcc3. I get this:\n\ngit-compat-util.h:209:1: warning: \"fopen\" redefined\nIn file included from git-compat-util.h:51,\n from builtin.h:4,\n from git.c:1:\n/usr/local/lib/gcc-lib/powerpc-ibm-aix4.3.2.0/3.2.1/include/stdio.h:110:1:\nwarning: this is the location of the previous definition\n\nLine 110 in ...include/stdio.h is inside a #ifdef _LARGE_FILES section and\nsays:\n\n#define fopen fopen64\n\nDid you also get this warning? Is _LARGE_FILES support solved in a\ndifferent way on 5.3?\n\n-- Hannes\n"},{"id":"76275","messageId":"e2b179460805070551x7a0072e0w4d406ef4112849ce@mail.gmail.com","threadId":"13411","inReplyTo":"4821992F.4060201@viscovery.net","subject":"Re: [PATCH] Makefile: update the default build options for AIX","fromName":"Mike Ralphson","fromEmail":"mike.ralphson@gmail.com","sentAt":"2008-05-07T12:51:46Z","receivedAt":"2008-05-07T12:51:46Z","isPatch":true,"sender":{"key":"mike.ralphson@gmail.com","avatar":"https://avatars.githubusercontent.com/u/21603?v=4"},"body":"2008/5/7 Johannes Sixt :\n>\n> I'm trying this patch on AIX 4.3.3 (sigh!) with gcc3. I get this:\n>\n> git-compat-util.h:209:1: warning: \"fopen\" redefined\n> In file included from git-compat-util.h:51,\n> from builtin.h:4,\n> from git.c:1:\n> /usr/local/lib/gcc-lib/powerpc-ibm-aix4.3.2.0/3.2.1/include/stdio.h:110:1:\n> warning: this is the location of the previous definition\n>\n> Line 110 in ...include/stdio.h is inside a #ifdef _LARGE_FILES section and\n> says:\n>\n> #define fopen fopen64\n>\n> Did you also get this warning? Is _LARGE_FILES support solved in a\n> different way on 5.3?\n\nThe warning (I get rather a lot of them) is caused by the\ncompat/fopen.c included when FREAD_READS_DIRECTORIES is defined. I\ntried moving the #undef fopen to git-compat-util.h but that resulted\nin a broken build and me reaching the end of my limited ability with\nc.\n\nIn file included from cache.h:4,\n from daemon.c:1:\ngit-compat-util.h:209:1: warning: \"fopen\" redefined\nIn file included from git-compat-util.h:51,\n from cache.h:4,\n from daemon.c:1:\n/opt/freeware/lib/gcc-lib/powerpc-ibm-aix5.3.0.0/3.3.2/include/stdio.h:110:1:\nwarning: this is the location of the previous definition\n\nThe warnings are harmless, though untidy.\n\nI don't believe it's anything to do with _LARGE_FILES. Could you try\nbuilding first with one commented out, then the other? I don't think I\nhave access to a 4.3.3 box any more.\n\nMike\n"},{"id":"76277","messageId":"e2b179460805070605t71bed59eq8dc64e204623fd18@mail.gmail.com","threadId":"13411","inReplyTo":"e2b179460805070551x7a0072e0w4d406ef4112849ce@mail.gmail.com","subject":"Re: [PATCH] Makefile: update the default build options for AIX","fromName":"Mike Ralphson","fromEmail":"mike.ralphson@gmail.com","sentAt":"2008-05-07T13:05:25Z","receivedAt":"2008-05-07T13:05:25Z","isPatch":true,"sender":{"key":"mike.ralphson@gmail.com","avatar":"https://avatars.githubusercontent.com/u/21603?v=4"},"body":"2008/5/7 Mike Ralphson :\n> 2008/5/7 Johannes Sixt :\n> >\n> > Did you also get this warning? Is _LARGE_FILES support solved in a\n> > different way on 5.3?\n>\n> I don't believe it's anything to do with _LARGE_FILES. Could you try\n> building first with one commented out, then the other? I don't think I\n> have access to a 4.3.3 box any more.\n\nI'm full of it (and I didn't try my own suggestion). It does appear to\nbe related to defining _LARGE_FILES.\n\nI'm afraid I can't see how the current #undef is working, let alone\nsuggest how to fix it when fopen is already redefined. 8-(\n\nMike\n"},{"id":"76279","messageId":"4821AB32.8090700@viscovery.net","threadId":"13411","inReplyTo":"e2b179460805070551x7a0072e0w4d406ef4112849ce@mail.gmail.com","subject":"Re: [PATCH] Makefile: update the default build options for AIX","fromName":"Johannes Sixt","fromEmail":"j.sixt@viscovery.net","sentAt":"2008-05-07T13:14:26Z","receivedAt":"2008-05-07T13:14:26Z","isPatch":true,"sender":{"key":"j6t@kdbg.org","avatar":"https://avatars.githubusercontent.com/u/14810926?v=4"},"body":"Mike Ralphson schrieb:\n> 2008/5/7 Johannes Sixt :\n>> I'm trying this patch on AIX 4.3.3 (sigh!) with gcc3. I get this:\n>>\n>> git-compat-util.h:209:1: warning: \"fopen\" redefined\n>> In file included from git-compat-util.h:51,\n>> from builtin.h:4,\n>> from git.c:1:\n>> /usr/local/lib/gcc-lib/powerpc-ibm-aix4.3.2.0/3.2.1/include/stdio.h:110:1:\n>> warning: this is the location of the previous definition\n>>\n>> Line 110 in ...include/stdio.h is inside a #ifdef _LARGE_FILES section and\n>> says:\n>>\n>> #define fopen fopen64\n>>\n>> Did you also get this warning? Is _LARGE_FILES support solved in a\n>> different way on 5.3?\n> \n> The warning (I get rather a lot of them) is caused by the\n> compat/fopen.c included when FREAD_READS_DIRECTORIES is defined. I\n> tried moving the #undef fopen to git-compat-util.h but that resulted\n> in a broken build and me reaching the end of my limited ability with\n> c.\n> \n> In file included from cache.h:4,\n> from daemon.c:1:\n> git-compat-util.h:209:1: warning: \"fopen\" redefined\n> In file included from git-compat-util.h:51,\n> from cache.h:4,\n> from daemon.c:1:\n> /opt/freeware/lib/gcc-lib/powerpc-ibm-aix5.3.0.0/3.3.2/include/stdio.h:110:1:\n> warning: this is the location of the previous definition\n\nSo you we in the same boat.\n\n> The warnings are harmless, though untidy.\n> I don't believe it's anything to do with _LARGE_FILES. Could you try\n> building first with one commented out, then the other? I don't think I\n> have access to a 4.3.3 box any more.\n\nUntidy, yes; harmless: not necessarily. It has a lot to do with _LARGE_FILES.\n\nThe #define fopen in git-compat-util.h essentially defeats the effect of\n_LARGE_FILES as far as fopen() calls are concerned: If\nFREAD_READS_DIRECTORIES is not defined, fopen() would be redirected to\nfopen64(), but when it is defined, it is redirected to git_fopen(), which\nin turn uses fopen() instead of fopen64() (due to the #undef in\ncompat/fopen.c).\n\nThis might be dangerous if some other function of the f*64() family uses\nthe FILE* that the fopen() call returned. I don't know if there is such a\nusage pattern somewhere in git.\n\nWhy did you need _LARGE_FILES in the first place?\n\n-- Hannes\n"},{"id":"76281","messageId":"e2b179460805070720o4028a841t6eabe9c1668a7a9f@mail.gmail.com","threadId":"13411","inReplyTo":"4821AB32.8090700@viscovery.net","subject":"Re: [PATCH] Makefile: update the default build options for AIX","fromName":"Mike Ralphson","fromEmail":"mike.ralphson@gmail.com","sentAt":"2008-05-07T14:20:45Z","receivedAt":"2008-05-07T14:20:45Z","isPatch":true,"sender":{"key":"mike.ralphson@gmail.com","avatar":"https://avatars.githubusercontent.com/u/21603?v=4"},"body":"2008/5/7 Johannes Sixt :\n> Mike Ralphson schrieb:\n>\n> > 2008/5/7 Johannes Sixt :\n> So you we in the same boat.\n>\n> > The warnings are harmless, though untidy.\n> > I don't believe it's anything to do with _LARGE_FILES. Could you try\n> > building first with one commented out, then the other? I don't think I\n> > have access to a 4.3.3 box any more.\n>\n> Untidy, yes; harmless: not necessarily. It has a lot to do with _LARGE_FILES.\n>\n> The #define fopen in git-compat-util.h essentially defeats the effect of\n> _LARGE_FILES as far as fopen() calls are concerned: If\n> FREAD_READS_DIRECTORIES is not defined, fopen() would be redirected to\n> fopen64(), but when it is defined, it is redirected to git_fopen(), which\n> in turn uses fopen() instead of fopen64() (due to the #undef in\n> compat/fopen.c).\n>\n> This might be dangerous if some other function of the f*64() family uses\n> the FILE* that the fopen() call returned. I don't know if there is such a\n> usage pattern somewhere in git.\n>\n> Why did you need _LARGE_FILES in the first place?\n\nWelcome aboard!\n\nI was seeing test failures in t5302-pack-index.sh\n\nSpecifically tests 9 and 20\n# 9: index v2: force some 64-bit offsets with pack-objects\n# 20: create a stealth corruption in a delta base reference\n\nThese tests seem to be skipped these days if off_t isn't large enough,\nI preferred the passing tests to the skipped ones. Maybe it isn't an\nissue in the real world.\n\nMike\n"},{"id":"76285","messageId":"4821BECA.8020509@nrlssc.navy.mil","threadId":"13411","inReplyTo":"4821AB32.8090700@viscovery.net","subject":"Re: [PATCH] Makefile: update the default build options for AIX","fromName":"Brandon Casey","fromEmail":"casey@nrlssc.navy.mil","sentAt":"2008-05-07T14:38:02Z","receivedAt":"2008-05-07T14:38:02Z","isPatch":true,"sender":{"key":"drafnel@gmail.com","avatar":"https://avatars.githubusercontent.com/u/921167?v=4"},"body":"Johannes Sixt wrote:\n> The #define fopen in git-compat-util.h essentially defeats the effect of\n> _LARGE_FILES as far as fopen() calls are concerned: If\n> FREAD_READS_DIRECTORIES is not defined, fopen() would be redirected to\n> fopen64(), but when it is defined, it is redirected to git_fopen(), which\n> in turn uses fopen() instead of fopen64() (due to the #undef in\n> compat/fopen.c).\n> \n\nHow about something like this?\n\ndiff --git a/compat/fopen.c b/compat/fopen.c\nindex ccb9e89..70b0d4d 100644\n--- a/compat/fopen.c\n+++ b/compat/fopen.c\n@@ -1,5 +1,5 @@\n+#undef FREAD_READS_DIRECTORIES\n #include \"../git-compat-util.h\"\n-#undef fopen\n FILE *git_fopen(const char *path, const char *mode)\n {\n FILE *fp;\n\n\n-brandon\n"},{"id":"76289","messageId":"e2b179460805070815u6cc627feo6137084fe7c5a635@mail.gmail.com","threadId":"13411","inReplyTo":"4821BECA.8020509@nrlssc.navy.mil","subject":"Re: [PATCH] Makefile: update the default build options for AIX","fromName":"Mike Ralphson","fromEmail":"mike.ralphson@gmail.com","sentAt":"2008-05-07T15:15:58Z","receivedAt":"2008-05-07T15:15:58Z","isPatch":true,"sender":{"key":"mike.ralphson@gmail.com","avatar":"https://avatars.githubusercontent.com/u/21603?v=4"},"body":"2008/5/7 Brandon Casey :\n> Johannes Sixt wrote:\n> > The #define fopen in git-compat-util.h essentially defeats the effect of\n> > _LARGE_FILES as far as fopen() calls are concerned: If\n> > FREAD_READS_DIRECTORIES is not defined, fopen() would be redirected to\n> > fopen64(), but when it is defined, it is redirected to git_fopen(), which\n> > in turn uses fopen() instead of fopen64() (due to the #undef in\n> > compat/fopen.c).\n> >\n>\n> How about something like this?\n>\n> diff --git a/compat/fopen.c b/compat/fopen.c\n> index ccb9e89..70b0d4d 100644\n> --- a/compat/fopen.c\n> +++ b/compat/fopen.c\n> @@ -1,5 +1,5 @@\n> +#undef FREAD_READS_DIRECTORIES\n> #include \"../git-compat-util.h\"\n> -#undef fopen\n> FILE *git_fopen(const char *path, const char *mode)\n> {\n> FILE *fp;\n>\n>\n> -brandon\n>\n\nTa. I still get all the warnings with that, was that what you were\ntrying to solve? The 64 bit specific tests in t5302 do still pass.\n\nMike\n"},{"id":"76293","messageId":"4821CD1E.80603@viscovery.net","threadId":"13411","inReplyTo":"e2b179460805070815u6cc627feo6137084fe7c5a635@mail.gmail.com","subject":"Re: [PATCH] Makefile: update the default build options for AIX","fromName":"Johannes Sixt","fromEmail":"j.sixt@viscovery.net","sentAt":"2008-05-07T15:39:10Z","receivedAt":"2008-05-07T15:39:10Z","isPatch":true,"sender":{"key":"j6t@kdbg.org","avatar":"https://avatars.githubusercontent.com/u/14810926?v=4"},"body":"Mike Ralphson schrieb:\n> 2008/5/7 Brandon Casey :\n>> How about something like this?\n>>\n>> diff --git a/compat/fopen.c b/compat/fopen.c\n>> index ccb9e89..70b0d4d 100644\n>> --- a/compat/fopen.c\n>> +++ b/compat/fopen.c\n>> @@ -1,5 +1,5 @@\n>> +#undef FREAD_READS_DIRECTORIES\n>> #include \"../git-compat-util.h\"\n>> -#undef fopen\n>> FILE *git_fopen(const char *path, const char *mode)\n>> {\n>> FILE *fp;\n>>\n>>\n>> -brandon\n>>\n> \n> Ta. I still get all the warnings with that, was that what you were\n> trying to solve? The 64 bit specific tests in t5302 do still pass.\n\nYou should get one less warning (the one from compat/fopen.c), but this\ntime they *are* harmless. ;)\n\n-- Hannes\n"},{"id":"76295","messageId":"4821CD5C.5010506@nrlssc.navy.mil","threadId":"13411","inReplyTo":"e2b179460805070815u6cc627feo6137084fe7c5a635@mail.gmail.com","subject":"Re: [PATCH] Makefile: update the default build options for AIX","fromName":"Brandon Casey","fromEmail":"casey@nrlssc.navy.mil","sentAt":"2008-05-07T15:40:12Z","receivedAt":"2008-05-07T15:40:12Z","isPatch":true,"sender":{"key":"drafnel@gmail.com","avatar":"https://avatars.githubusercontent.com/u/921167?v=4"},"body":"Mike Ralphson wrote:\n> 2008/5/7 Brandon Casey :\n>> Johannes Sixt wrote:\n>> > The #define fopen in git-compat-util.h essentially defeats the effect of\n>> > _LARGE_FILES as far as fopen() calls are concerned: If\n>> > FREAD_READS_DIRECTORIES is not defined, fopen() would be redirected to\n>> > fopen64(), but when it is defined, it is redirected to git_fopen(), which\n>> > in turn uses fopen() instead of fopen64() (due to the #undef in\n>> > compat/fopen.c).\n>> >\n>>\n>> How about something like this?\n>>\n>> diff --git a/compat/fopen.c b/compat/fopen.c\n>> index ccb9e89..70b0d4d 100644\n>> --- a/compat/fopen.c\n>> +++ b/compat/fopen.c\n>> @@ -1,5 +1,5 @@\n>> +#undef FREAD_READS_DIRECTORIES\n>> #include \"../git-compat-util.h\"\n>> -#undef fopen\n>> FILE *git_fopen(const char *path, const char *mode)\n>> {\n>> FILE *fp;\n>>\n>>\n>> -brandon\n>>\n> \n> Ta. I still get all the warnings with that, was that what you were\n> trying to solve? The 64 bit specific tests in t5302 do still pass.\n\nAh, yes. You would still get the warnings for every other file that\nincludes git-compat-util.h, except compat/fopen.c. I didn't think\nabout all of those. :) In this case those are indeed harmless. And now\nthe git provided git_fopen() will use the compiler selected fopen()\nwhich should avoid any of the gotchas that Hannes brought up.\n\n-brandon\n"},{"id":"76299","messageId":"7vfxsudrt0.fsf@gitster.siamese.dyndns.org","threadId":"13411","inReplyTo":"4821CD5C.5010506@nrlssc.navy.mil","subject":"Re: [PATCH] Makefile: update the default build options for AIX","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2008-05-07T16:04:11Z","receivedAt":"2008-05-07T16:04:11Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Brandon Casey writes:\n\n> Mike Ralphson wrote:\n>> 2008/5/7 Brandon Casey :\n>>> Johannes Sixt wrote:\n>>> > The #define fopen in git-compat-util.h essentially defeats the effect of\n>>> > _LARGE_FILES as far as fopen() calls are concerned: If\n>>> > FREAD_READS_DIRECTORIES is not defined, fopen() would be redirected to\n>>> > fopen64(), but when it is defined, it is redirected to git_fopen(), which\n>>> > in turn uses fopen() instead of fopen64() (due to the #undef in\n>>> > compat/fopen.c).\n>>> >\n>>>\n>>> How about something like this?\n>>>\n>>> diff --git a/compat/fopen.c b/compat/fopen.c\n>>> index ccb9e89..70b0d4d 100644\n>>> --- a/compat/fopen.c\n>>> +++ b/compat/fopen.c\n>>> @@ -1,5 +1,5 @@\n>>> +#undef FREAD_READS_DIRECTORIES\n>>> #include \"../git-compat-util.h\"\n>>> -#undef fopen\n>>> FILE *git_fopen(const char *path, const char *mode)\n>>> {\n>>> FILE *fp;\n>>>\n>>>\n>>> -brandon\n>>>\n>> \n>> Ta. I still get all the warnings with that, was that what you were\n>> trying to solve? The 64 bit specific tests in t5302 do still pass.\n>\n> Ah, yes. You would still get the warnings for every other file that\n> includes git-compat-util.h, except compat/fopen.c. I didn't think\n> about all of those. :) In this case those are indeed harmless. And now\n> the git provided git_fopen() will use the compiler selected fopen()\n> which should avoid any of the gotchas that Hannes brought up.\n\nIn any case, that #undef then #include dance needs a big comment on why it\nhas to be so.\n"},{"id":"76303","messageId":"e2b179460805070920i2ff5798dpacb5c55d851d5ede@mail.gmail.com","threadId":"13411","inReplyTo":"7vfxsudrt0.fsf@gitster.siamese.dyndns.org","subject":"Re: [PATCH] Makefile: update the default build options for AIX","fromName":"Mike Ralphson","fromEmail":"mike.ralphson@gmail.com","sentAt":"2008-05-07T16:20:14Z","receivedAt":"2008-05-07T16:20:14Z","isPatch":true,"sender":{"key":"mike.ralphson@gmail.com","avatar":"https://avatars.githubusercontent.com/u/21603?v=4"},"body":"2008/5/7 Junio C Hamano :\n>\n> Brandon Casey writes:\n>\n> > Mike Ralphson wrote:\n> >> 2008/5/7 Brandon Casey :\n> >>> Johannes Sixt wrote:\n> >>> > The #define fopen in git-compat-util.h essentially defeats the effect of\n> >>> > _LARGE_FILES as far as fopen() calls are concerned: If\n> >>> > FREAD_READS_DIRECTORIES is not defined, fopen() would be redirected to\n> >>> > fopen64(), but when it is defined, it is redirected to git_fopen(), which\n> >>> > in turn uses fopen() instead of fopen64() (due to the #undef in\n> >>> > compat/fopen.c).\n> >>> >\n> >>>\n> >>> How about something like this?\n> >>>\n> >>> diff --git a/compat/fopen.c b/compat/fopen.c\n> >>> index ccb9e89..70b0d4d 100644\n> >>> --- a/compat/fopen.c\n> >>> +++ b/compat/fopen.c\n> >>> @@ -1,5 +1,5 @@\n> >>> +#undef FREAD_READS_DIRECTORIES\n> >>> #include \"../git-compat-util.h\"\n> >>> -#undef fopen\n> >>> FILE *git_fopen(const char *path, const char *mode)\n> >>> {\n> >>> FILE *fp;\n> >>>\n> >>>\n> >>> -brandon\n> >>>\n> >>\n> >> Ta. I still get all the warnings with that, was that what you were\n> >> trying to solve? The 64 bit specific tests in t5302 do still pass.\n> >\n> > Ah, yes. You would still get the warnings for every other file that\n> > includes git-compat-util.h, except compat/fopen.c. I didn't think\n> > about all of those. :) In this case those are indeed harmless. And now\n> > the git provided git_fopen() will use the compiler selected fopen()\n> > which should avoid any of the gotchas that Hannes brought up.\n>\n> In any case, that #undef then #include dance needs a big comment on why it\n> has to be so.\n>\n\nIndeed. Please add ascii-art diagrams and don't use long words. I may\nthen have a chance of understanding how this works, and how I should\nhave spotted 5 potentially non-harmless warnings among 400 noise ones,\nwhen all I did was get the testsuite from non-passing to passing! 8-)\n\nIn reality, thanks to all for pitching in.\n\nMike\n"},{"id":"76308","messageId":"4821E81A.4030600@nrlssc.navy.mil","threadId":"13411","inReplyTo":"7vfxsudrt0.fsf@gitster.siamese.dyndns.org","subject":"[PATCH] compat/fopen.c: avoid clobbering the system defined fopen macro","fromName":"Brandon Casey","fromEmail":"casey@nrlssc.navy.mil","sentAt":"2008-05-07T17:34:18Z","receivedAt":"2008-05-07T17:34:18Z","isPatch":true,"sender":{"key":"drafnel@gmail.com","avatar":"https://avatars.githubusercontent.com/u/921167?v=4"},"body":"Some systems define fopen as a macro based on compiler settings.\nThe previous technique for reverting to the system fopen function\nby merely undefining fopen is inadequate in this case. Instead,\navoid defining fopen entirely when compiling this source file.\n\nSigned-off-by: Brandon Casey \n---\n compat/fopen.c | 13 ++++++++++++-\n 1 files changed, 12 insertions(+), 1 deletions(-)\n\ndiff --git a/compat/fopen.c b/compat/fopen.c\nindex ccb9e89..b5ca142 100644\n--- a/compat/fopen.c\n+++ b/compat/fopen.c\n@@ -1,5 +1,16 @@\n+/*\n+ * The order of the following two lines is important.\n+ *\n+ * FREAD_READS_DIRECTORIES is undefined before including git-compat-util.h\n+ * to avoid the redefinition of fopen within git-compat-util.h. This is\n+ * necessary since fopen is a macro on some platforms which may be set\n+ * based on compiler options. For example, on AIX fopen is set to fopen64\n+ * when _LARGE_FILES is defined. The previous technique of merely undefining\n+ * fopen after including git-compat-util.h is inadequate in this case.\n+ */\n+#undef FREAD_READS_DIRECTORIES\n #include \"../git-compat-util.h\"\n-#undef fopen\n+\n FILE *git_fopen(const char *path, const char *mode)\n {\n \tFILE *fp;\n-- \n1.5.5\n"},{"id":"76309","messageId":"4821E892.4080104@nrlssc.navy.mil","threadId":"13411","inReplyTo":"e2b179460805070920i2ff5798dpacb5c55d851d5ede@mail.gmail.com","subject":"Re: [PATCH] Makefile: update the default build options for AIX","fromName":"Brandon Casey","fromEmail":"casey@nrlssc.navy.mil","sentAt":"2008-05-07T17:36:18Z","receivedAt":"2008-05-07T17:36:18Z","isPatch":true,"sender":{"key":"drafnel@gmail.com","avatar":"https://avatars.githubusercontent.com/u/921167?v=4"},"body":"Mike Ralphson wrote:\n> Indeed. Please add ascii-art diagrams and don't use long words. I may\n> then have a chance of understanding how this works,\n\nI think this is simpler than you are making it out to be.\n\nAll the git source files currently #include git-compat-util.h. When a\nplatform is missing a function, we implement that function in the compat/\nsubdirectory and add an entry for it in git-compat-util.h.\n\nIn this case we found a problem that could be worked around by replacing every\ncall to fopen with an internal function. So we did the standard thing of\ncreating a new function in the compat/ subdirectory named git_fopen() and added\nmacro statements within git-compat-util.h to redefine fopen to be git_fopen.\nBut, git_fopen needs to call the _real_ fopen and it _also_ includes git-compat-util.h.\nSo, after including git-compat-util.h, we undefined the fopen macro to undo the\nassignment that we had just performed. This doesn't work if the system is also setting\nan fopen macro. So the fix is to avoid clobbering the system setting at all when\ncompiling compat/fopen.c\n\n-brandon\n"},{"id":"76352","messageId":"e2b179460805080027pf9ff518xf4fcbb248ecac4bf@mail.gmail.com","threadId":"13411","inReplyTo":"4821E81A.4030600@nrlssc.navy.mil","subject":"Re: [PATCH] compat/fopen.c: avoid clobbering the system defined fopen macro","fromName":"Mike Ralphson","fromEmail":"mike.ralphson@gmail.com","sentAt":"2008-05-08T07:27:48Z","receivedAt":"2008-05-08T07:27:48Z","isPatch":true,"sender":{"key":"mike.ralphson@gmail.com","avatar":"https://avatars.githubusercontent.com/u/21603?v=4"},"body":"2008/5/7 Brandon Casey :\n>\n> Some systems define fopen as a macro based on compiler settings.\n> The previous technique for reverting to the system fopen function\n> by merely undefining fopen is inadequate in this case. Instead,\n> avoid defining fopen entirely when compiling this source file.\n>\n> Signed-off-by: Brandon Casey \n>\n\nTested-by: Mike Ralphson \n\nBoth with and without -D_LARGE_FILES. Many thanks.\n\nH.Merijn, is this change also ok for your HP-UX?\n\nI guess there may still be a case for not defining _LARGE_FILES by\ndefault on AIX as all the warnings may be off-putting or mask other\nissues. Maybe instead having a comment for those who need large\npack-file support? Will submit amended Makefile patch if there's\ninterest.\n\nMike\n"},{"id":"76354","messageId":"4822AD19.6000609@viscovery.net","threadId":"13411","inReplyTo":"e2b179460805080027pf9ff518xf4fcbb248ecac4bf@mail.gmail.com","subject":"Re: [PATCH] compat/fopen.c: avoid clobbering the system defined fopen macro","fromName":"Johannes Sixt","fromEmail":"j.sixt@viscovery.net","sentAt":"2008-05-08T07:34:49Z","receivedAt":"2008-05-08T07:34:49Z","isPatch":true,"sender":{"key":"j6t@kdbg.org","avatar":"https://avatars.githubusercontent.com/u/14810926?v=4"},"body":"Mike Ralphson schrieb:\n> I guess there may still be a case for not defining _LARGE_FILES by\n> default on AIX as all the warnings may be off-putting or mask other\n> issues. Maybe instead having a comment for those who need large\n> pack-file support? Will submit amended Makefile patch if there's\n> interest.\n\nSince with this patch we are treating fopen specially anyway, we could go\none step further and do this, too:\n---\ndiff --git a/git-compat-util.h b/git-compat-util.h\nindex b2708f3..dad4d48 100644\n--- a/git-compat-util.h\n+++ b/git-compat-util.h\n@@ -230,6 +230,9 @@ void *gitmemmem(const void *haystack,\n #endif\n\n #ifdef FREAD_READS_DIRECTORIES\n+#ifdef fopen\n+#undef fopen\n+#endif\n #define fopen(a,b) git_fopen(a,b)\n extern FILE *git_fopen(const char*, const char*);\n #endif\n"},{"id":"76359","messageId":"e2b179460805080059s76b07f30wedded8b1f5b17dfa@mail.gmail.com","threadId":"13411","inReplyTo":"4822AD19.6000609@viscovery.net","subject":"Re: [PATCH] compat/fopen.c: avoid clobbering the system defined fopen macro","fromName":"Mike Ralphson","fromEmail":"mike.ralphson@gmail.com","sentAt":"2008-05-08T07:59:15Z","receivedAt":"2008-05-08T07:59:15Z","isPatch":true,"sender":{"key":"mike.ralphson@gmail.com","avatar":"https://avatars.githubusercontent.com/u/21603?v=4"},"body":"2008/5/8 Johannes Sixt :\n> Mike Ralphson schrieb:\n>> I guess there may still be a case for not defining _LARGE_FILES by\n>> default on AIX as all the warnings may be off-putting or mask other\n>> issues. Maybe instead having a comment for those who need large\n>> pack-file support? Will submit amended Makefile patch if there's\n>> interest.\n>\n> Since with this patch we are treating fopen specially anyway, we could go\n> one step further and do this, too:\n> ---\n> diff --git a/git-compat-util.h b/git-compat-util.h\n> index b2708f3..dad4d48 100644\n> --- a/git-compat-util.h\n> +++ b/git-compat-util.h\n> @@ -230,6 +230,9 @@ void *gitmemmem(const void *haystack,\n> #endif\n>\n> #ifdef FREAD_READS_DIRECTORIES\n> +#ifdef fopen\n> +#undef fopen\n> +#endif\n> #define fopen(a,b) git_fopen(a,b)\n> extern FILE *git_fopen(const char*, const char*);\n> #endif\n>\n\nLoving your work! Squashes all the related warnings, re-tested etc.\nTechnically, is the #ifdef / #endif actually required? Or is\n#undef'ing an undefined macro not portable? I agree it aids clarity\nfor no cost.\n\nMike\n"},{"id":"76366","messageId":"20080508123732.33d6ef00@pc09.procura.nl","threadId":"13411","inReplyTo":"e2b179460805080027pf9ff518xf4fcbb248ecac4bf@mail.gmail.com","subject":"Re: [PATCH] compat/fopen.c: avoid clobbering the system defined fopen macro","fromName":"H.Merijn Brand","fromEmail":"h.m.brand@xs4all.nl","sentAt":"2008-05-08T10:37:32Z","receivedAt":"2008-05-08T10:37:32Z","isPatch":true,"sender":{"key":"h.m.brand@xs4all.nl","avatar":"https://gravatar.com/avatar/5b8f83ee35c427a646cbea3b104346e00ab3663b99bbf435cddeb75cd4b3857b?d=mp&s=160"},"body":"On Thu, 8 May 2008 08:27:48 +0100, \"Mike Ralphson\"\n wrote:\n\n> 2008/5/7 Brandon Casey :\n> >\n> > Some systems define fopen as a macro based on compiler settings.\n> > The previous technique for reverting to the system fopen function\n> > by merely undefining fopen is inadequate in this case. Instead,\n> > avoid defining fopen entirely when compiling this source file.\n> >\n> > Signed-off-by: Brandon Casey \n> >\n> \n> Tested-by: Mike Ralphson \n> \n> Both with and without -D_LARGE_FILES. Many thanks.\n> \n> H.Merijn, is this change also ok for your HP-UX?\n\nI'm not really actively following the ML anymore, as it is kinda busy :)\nI was able to compile/test/install 1.5.5.1 on HP-UX 11.00/32 and on\nHP-UX 11.23-ilp64 with a reasonable small set of additional changes.\n\nMain thing I found to make most tests that used to fail now pass is to\nuse bash, instead of HP's native POSIX shell.\n\nhttp://www.xs4all.nl/~procura/git-1.5.5.1-11.00.diff\nhttp://www.xs4all.nl/~procura/git-1.5.5.1-11.23.diff\n\nWe - as a company - now actively use git on HP-UX 11.00 and Linux\n\n> I guess there may still be a case for not defining _LARGE_FILES by\n> default on AIX as all the warnings may be off-putting or mask other\n> issues.\n\nI also have AIX, but I hate it, and don't really care about it. It's\njust that we have some poor customers whose IT people forced this OS\nupon them.\n\n> Maybe instead having a comment for those who need large pack-file\n> support? Will submit amended Makefile patch if there's interest.\n\n-- \nH.Merijn Brand Amsterdam Perl Mongers (http://amsterdam.pm.org/)\nusing & porting perl 5.6.2, 5.8.x, 5.10.x on HP-UX 10.20, 11.00, 11.11,\n& 11.23, SuSE 10.1 & 10.2, AIX 5.2, and Cygwin. http://qa.perl.org\nhttp://mirrors.develooper.com/hpux/ http://www.test-smoke.org\n http://www.goldmark.org/jeff/stupid-disclaimers/\n"},{"id":"77106","messageId":"e2b179460805160319n420c309eg9b9bbb1e3adb299@mail.gmail.com","threadId":"13411","inReplyTo":"4821992F.4060201@viscovery.net","subject":"Re: [PATCH] Makefile: update the default build options for AIX","fromName":"Mike Ralphson","fromEmail":"mike.ralphson@gmail.com","sentAt":"2008-05-16T10:19:06Z","receivedAt":"2008-05-16T10:19:06Z","isPatch":true,"sender":{"key":"mike.ralphson@gmail.com","avatar":"https://avatars.githubusercontent.com/u/21603?v=4"},"body":"2008/5/7 Johannes Sixt :\n> Mike Ralphson schrieb:\n>> NO_MKDTEMP is required to build, FREAD_READS_DIRECTORIES and the definition\n>> of _LARGE_FILES fix test suite failures and INTERNAL_QSORT is required for\n>> adequate performance.\n>\n> I'm trying this patch on AIX 4.3.3 (sigh!) with gcc3...\n\nNow the interaction between FREAD_READS_DIRECTORIES and _LARGE_FILES\nhas been sorted out, and the\nwt-status.h warning fix is also in, did you manage to finish testing\nthis? The INTERNAL_QSORT gave me a 2 orders of magnitude speed up on\ngit status / commit etc.\n\nI should have mentioned that I build with SHELL_PATH = /bin/bash and\nensure that /usr/linux/bin (or /opt/freeware/bin) from the AIX toolbox\nis prepended to the PATH to run the test-suite. I didn't want to fold\nthese into the patch as the paths are somewhat environment specific.\n\nMike\n"},{"id":"77127","messageId":"482D8E07.3050300@viscovery.net","threadId":"13411","inReplyTo":"e2b179460805160319n420c309eg9b9bbb1e3adb299@mail.gmail.com","subject":"Re: [PATCH] Makefile: update the default build options for AIX","fromName":"Johannes Sixt","fromEmail":"j.sixt@viscovery.net","sentAt":"2008-05-16T13:37:11Z","receivedAt":"2008-05-16T13:37:11Z","isPatch":true,"sender":{"key":"j6t@kdbg.org","avatar":"https://avatars.githubusercontent.com/u/14810926?v=4"},"body":"Mike Ralphson schrieb:\n> 2008/5/7 Johannes Sixt :\n>> Mike Ralphson schrieb:\n>>> NO_MKDTEMP is required to build, FREAD_READS_DIRECTORIES and the definition\n>>> of _LARGE_FILES fix test suite failures and INTERNAL_QSORT is required for\n>>> adequate performance.\n>> I'm trying this patch on AIX 4.3.3 (sigh!) with gcc3...\n> \n> Now the interaction between FREAD_READS_DIRECTORIES and _LARGE_FILES\n> has been sorted out, and the\n> wt-status.h warning fix is also in, did you manage to finish testing\n> this? The INTERNAL_QSORT gave me a 2 orders of magnitude speed up on\n> git status / commit etc.\n> \n> I should have mentioned that I build with SHELL_PATH = /bin/bash and\n> ensure that /usr/linux/bin (or /opt/freeware/bin) from the AIX toolbox\n> is prepended to the PATH to run the test-suite. I didn't want to fold\n> these into the patch as the paths are somewhat environment specific.\n\nI compiled git on AIX 4.3.3. Even though the testsuite does not pass (due\nto a too old perl, non-working libiconv, and a sed that does not print the\nlast line if there's no LF at the end), there's no failure that I would\nattribute to your changes. Therefore, I'd say:\n\nTested-by: Johannes Sixt \n\n-- Hannes\n"}]}