threads / patch / 34499

patchFix compilation on OS X.

Subject: [PATCH] Fix compilation on OS X.

## tl;dr

10 messages between Jul 20, 2013 and Jul 21, 2013. Diffs are folded; open one to read it.

replies: 9people: 4as markdown or json

Benoit Sigoure· Jul 20, 2013, 07:49 UTC · lore
On OS X libc headers don't define `environ', and since ec535cc2 removed
the redundant declaration this code no longer builds on OS X.
---
 compat/unsetenv.c | 5 +++++
 1 file changed, 5 insertions(+)
Show changes to compat/unsetenv.c +5 −0
diff --git a/compat/unsetenv.c b/compat/unsetenv.c
index 4ea1856..addf3dc 100644
--- a/compat/unsetenv.c
+++ b/compat/unsetenv.c
@@ -1,5 +1,10 @@
 #include "../git-compat-util.h"
 
+#ifdef __APPLE__
+// On OS X libc headers don't define this symbol.
+extern char **environ;
+#endif
+
 void gitunsetenv (const char *name)
 {
      int src, dst;

-- 
1.8.2.1.539.g4196a96
Ramkumar Ramachandra· Jul 20, 2013, 07:55 UTC · re: Benoit Sigoure · lore

Re: [PATCH] Fix compilation on OS X.

Benoit Sigoure wrote:
Show 12 quoted lines
> diff --git a/compat/unsetenv.c b/compat/unsetenv.c
> index 4ea1856..addf3dc 100644
> --- a/compat/unsetenv.c
> +++ b/compat/unsetenv.c
> @@ -1,5 +1,10 @@
>  #include "../git-compat-util.h"
>
> +#ifdef __APPLE__
> +// On OS X libc headers don't define this symbol.
> +extern char **environ;
> +#endif
> +

Shouldn't this go into git-compat-util.h, since there may be other files depending on this variable?

tsuna· Jul 20, 2013, 07:56 UTC · re: Ramkumar Ramachandra · lore

Re: [PATCH] Fix compilation on OS X.

On Sat, Jul 20, 2013 at 12:55 AM, Ramkumar Ramachandra <artagnon@gmail.com> wrote:

Show 16 quoted lines
> Benoit Sigoure wrote:
>> diff --git a/compat/unsetenv.c b/compat/unsetenv.c
>> index 4ea1856..addf3dc 100644
>> --- a/compat/unsetenv.c
>> +++ b/compat/unsetenv.c
>> @@ -1,5 +1,10 @@
>>  #include "../git-compat-util.h"
>>
>> +#ifdef __APPLE__
>> +// On OS X libc headers don't define this symbol.
>> +extern char **environ;
>> +#endif
>> +
>
> Shouldn't this go into git-compat-util.h, since there may be other
> files depending on this variable?

I thought about that but there are no other files that use `environ' so I opted for putting it here instead.

-- 
Benoit "tsuna" Sigoure
Torsten Bögershausen· Jul 20, 2013, 12:06 UTC · re: Benoit Sigoure · lore

Re: [PATCH] Fix compilation on OS X.

On 2013-07-20 09.49, Benoit Sigoure wrote:
Show 5 quoted lines
> +#ifdef __APPLE__
> +// On OS X libc headers don't define this symbol.
> +extern char **environ;
> +#endif
> +
A more generic approach could be:

In the file "config.mak.uname": Define a variable in the Darwin section like this NO_EXT_ENVIRON = UnfortunatelyYes

In "Makefile", pick it up, and convert it into a compiler option:
ifdef NO_EXT_ENVIRON
	BASIC_CFLAGS += -DNO_EXT_ENVIRON
endif

And in "git-compat-util.h", add these lines "at a good place": #ifdef NO_EXT_ENVIRON extern char **environ; #endif

This will allow other OS to use the NO_EXT_ENVIRON when needed,.

Thanks for working on this. /Torsten

Benoit Sigoure· Jul 20, 2013, 18:41 UTC · re: Torsten Bögershausen · lore
On OS X libc headers don't define `environ', and since ec535cc2 removed
the redundant declaration this code no longer builds on OS X.
---
 Makefile          | 5 +++++
 config.mak.uname  | 1 +
 git-compat-util.h | 4 ++++
 3 files changed, 10 insertions(+)
Show changes to 3 files +10 −0

Makefile, config.mak.uname, git-compat-util.h

diff --git a/Makefile b/Makefile
index 0600eb4..774db18 100644
--- a/Makefile
+++ b/Makefile
@@ -98,6 +98,8 @@ all::
 #
 # Define NO_UNSETENV if you don't have unsetenv in the C library.
 #
+# Define NO_EXT_ENVIRON if your C library doesn't define `environ'.
+#
 # Define NO_MKDTEMP if you don't have mkdtemp in the C library.
 #
 # Define MKDIR_WO_TRAILING_SLASH if your mkdir() can't deal with trailing slash.
@@ -1307,6 +1309,9 @@ ifdef NO_UNSETENV
 	COMPAT_CFLAGS += -DNO_UNSETENV
 	COMPAT_OBJS += compat/unsetenv.o
 endif
+ifdef NO_EXT_ENVIRON
+	COMPAT_CFLAGS += -DNO_EXT_ENVIRON
+endif
 ifdef NO_SYS_SELECT_H
 	BASIC_CFLAGS += -DNO_SYS_SELECT_H
 endif
diff --git a/config.mak.uname b/config.mak.uname
index 7ac541e..ebcfbfd 100644
--- a/config.mak.uname
+++ b/config.mak.uname
@@ -93,6 +93,7 @@ ifeq ($(uname_S),Darwin)
 		NO_STRLCPY = YesPlease
 	endif
 	NO_MEMMEM = YesPlease
+        NO_EXT_ENVIRON = UnfortunatelyYes
 	USE_ST_TIMESPEC = YesPlease
 	HAVE_DEV_TTY = YesPlease
 	NEEDS_CLIPPED_WRITE = YesPlease
diff --git a/git-compat-util.h b/git-compat-util.h
index ff193f4..3bac4e9 100644
--- a/git-compat-util.h
+++ b/git-compat-util.h
@@ -408,6 +408,10 @@ extern ssize_t git_pread(int fd, void *buf, size_t count, off_t offset);
  */
 extern ssize_t read_in_full(int fd, void *buf, size_t count);
 
+#ifdef NO_EXT_ENVIRON
+extern char **environ;
+#endif
+
 #ifdef NO_SETENV
 #define setenv gitsetenv
 extern int gitsetenv(const char *, const char *, int);

-- 
1.8.2.1.539.g4196a96
Junio C Hamano· Jul 21, 2013, 05:53 UTC · re: Torsten Bögershausen · lore

Re: [PATCH] Fix compilation on OS X.

Torsten Bögershausen <tboegi@web.de> writes:
Show 10 quoted lines
> On 2013-07-20 09.49, Benoit Sigoure wrote:
>> +#ifdef __APPLE__
>> +// On OS X libc headers don't define this symbol.
>> +extern char **environ;
>> +#endif
>> +
> A more generic approach could be:
>
> In the file "config.mak.uname": Define a variable in the Darwin section like this
> NO_EXT_ENVIRON = UnfortunatelyYes

Actually, it is _wrong_ for us to rely on system header files to define this symbol for us. Declaring "extern char **environ" is responsibility of the user programs (like us).

When _GNU_SOURCE is defined glibc header (I think it is unistd.h) seem to define it for us.

Perhaps the correct fix is to revert ec535cc2 for everybody, and if MinGW needs such a workaround, do it inside #ifndef MINGW?

tsuna· Jul 21, 2013, 06:10 UTC · re: Junio C Hamano · lore

Re: [PATCH] Fix compilation on OS X.

On Sat, Jul 20, 2013 at 10:53 PM, Junio C Hamano <gitster@pobox.com> wrote:
> Actually, it is _wrong_ for us to rely on system header files to
> define this symbol for us.  Declaring "extern char **environ" is
> responsibility of the user programs (like us).

Actually, that's right. The C99 standard doesn't mention anything about `environ' (only 7.20.4.5 defines `getenv') and POSIX explicitly states "the [environ] variable, which must be declared by the user if it is to be used directly" (http://pubs.opengroup.org/onlinepubs/9699919799/functions/environ.html)

Show 5 quoted lines
> When _GNU_SOURCE is defined glibc header (I think it is unistd.h)
> seem to define it for us.
>
> Perhaps the correct fix is to revert ec535cc2 for everybody, and if
> MinGW needs such a workaround, do it inside #ifndef MINGW?
That sounds right.
-- 
Benoit "tsuna" Sigoure
Benoit Sigoure· Jul 21, 2013, 06:17 UTC · re: Junio C Hamano · lore

[PATCH] Revert "compat/unsetenv.c: Fix a sparse warning"

This reverts commit ec535cc27e6c4f5e0b1d157e04f5511f166ecd9d.

POSIX explicitly states "the [environ] variable, which must be declared by the user if it is to be used directly". Not declaring it causes compilation to fail on OS X.

Instead don't declare the variable on MinGW, as it causes
a spurious warning there.
---
 compat/unsetenv.c | 3 +++
 1 file changed, 3 insertions(+)
Show changes to compat/unsetenv.c +3 −0
diff --git a/compat/unsetenv.c b/compat/unsetenv.c
index 4ea1856..bf5fd70 100644
--- a/compat/unsetenv.c
+++ b/compat/unsetenv.c
@@ -2,6 +2,9 @@
 
 void gitunsetenv (const char *name)
 {
+#if !defined(__MINGW32__)
+     extern char **environ;
+#endif
      int src, dst;
      size_t nmln;
 

-- 
1.8.2.1.539.g4196a96
Benoit Sigoure· Jul 21, 2013, 19:54 UTC · re: Benoit Sigoure · lore

[PATCH] Revert "compat/unsetenv.c: Fix a sparse warning"

This reverts commit ec535cc27e6c4f5e0b1d157e04f5511f166ecd9d.

POSIX explicitly states "the [environ] variable, which must be declared by the user if it is to be used directly". Not declaring it causes compilation to fail on OS X.

Instead don't declare the variable on MinGW, as it causes a spurious warning there.

Signed-off-by: Benoit Sigoure <tsunanet@gmail.com>
---
Resending as I forgot to Sign-off the previous patch.
 compat/unsetenv.c | 3 +++
 1 file changed, 3 insertions(+)
Show changes to compat/unsetenv.c +3 −0
diff --git a/compat/unsetenv.c b/compat/unsetenv.c
index 4ea1856..bf5fd70 100644
--- a/compat/unsetenv.c
+++ b/compat/unsetenv.c
@@ -2,6 +2,9 @@
 
 void gitunsetenv (const char *name)
 {
+#if !defined(__MINGW32__)
+     extern char **environ;
+#endif
      int src, dst;
      size_t nmln;
 

-- 
1.8.2.1.539.g4196a96
Junio C Hamano· Jul 21, 2013, 22:09 UTC · re: Benoit Sigoure · lore

Re: [PATCH] Revert "compat/unsetenv.c: Fix a sparse warning"

Benoit Sigoure <tsunanet@gmail.com> writes:
Show 10 quoted lines
> This reverts commit ec535cc27e6c4f5e0b1d157e04f5511f166ecd9d.
>
> POSIX explicitly states "the [environ] variable, which
> must be declared by the user if it is to be used directly".
> Not declaring it causes compilation to fail on OS X.
>
> Instead don't declare the variable on MinGW, as it causes
> a spurious warning there.
>
> Signed-off-by: Benoit Sigoure <tsunanet@gmail.com>
Thanks, will queue.
Show 20 quoted lines
> ---
>
> Resending as I forgot to Sign-off the previous patch.
>
>  compat/unsetenv.c | 3 +++
>  1 file changed, 3 insertions(+)
>
> diff --git a/compat/unsetenv.c b/compat/unsetenv.c
> index 4ea1856..bf5fd70 100644
> --- a/compat/unsetenv.c
> +++ b/compat/unsetenv.c
> @@ -2,6 +2,9 @@
>  
>  void gitunsetenv (const char *name)
>  {
> +#if !defined(__MINGW32__)
> +     extern char **environ;
> +#endif
>       int src, dst;
>       size_t nmln;

← back to recent threads