search

gearman / gearmand

http://gearman.org/
Other
740 stars 137 forks source link

Fix error: ‘environ’ was declared ‘extern’ and later ‘static’ [-fpermissive] #289

Closed thibaultduponchelle closed 4 years ago

thibaultduponchelle commented 4 years ago

Hello,

I'm not sure about this fix.

In the CPANTesters I noticed this error : 

libtest/cmdline.cc:65:15: error: 'environ' was declared 'extern' and later 'static' [-fpermissive]
   65 | static char **environ= NULL;
      |               ^~~~~~~
In file included from /usr/include/fortify/unistd.h:22,
                 from ./libtest/common.h:77,
                 from libtest/cmdline.cc:39:
/usr/include/unistd.h:183:15: note: previous declaration of 'environ'
  183 | extern char **environ;
      |               ^~~~~~~

From http://www.cpantesters.org/cpan/report/003e64ec-ca05-11ea-9066-e374b0ba08e8 (it is on an alpine Linux)

From the git blame it was added for OSX. I have tested with and without #ifdef on my machine but I have not tested on OSX...

Best regards.

Thibault

esabol commented 4 years ago

What compiler is this? Wondering why we don't see this on any other platform....

esabol commented 4 years ago

I think this #ifndef is supposed to be for macOS? If so, I don't think this is the right thing to do. I found the following code in another project (czmq):

#if defined (__UNIX__)
#   if defined (__UTYPE_OSX)
#include <crt_externs.h>
#define environ (*_NSGetEnviron ()) 
#   else
extern char **environ;              // should be declared as a part of unistd.h, but fail in some targets in Travis
                                    // declare it explicitly
#   endif
#endif
thibaultduponchelle commented 4 years ago

It's a matter of libc.

On alpine, it is musl instead gblic and in unistd.h we have:

#ifdef _GNU_SOURCE     
extern char **environ;
...
...
#endif

musl seems to rely on _GNU_SOURCE.

On glibc environment, __USE_GNU is implied by _GNU_SOURCE, see this in /usr/include/features.h in glibc environment:

#ifdef  _GNU_SOURCE
# define __USE_GNU      1
#endif

On my glibc machine, the unistd.h contains:

/* NULL-terminated array of "NAME=VALUE" environment variables.  */
extern char **__environ;
#ifdef __USE_GNU
extern char **environ;
#endif

You can reproduce the failing compilation like this:

docker run -it alpine /bin/sh

Then inside running container:

cd /home
wget https://github.com/gearman/gearmand/releases/download/1.1.19.1/gearmand-1.1.19.1.tar.gz
tar xvzf gearmand-1.1.19.1.tar.gz
cd gearmand-1.1.19.1
apk add musl-dev gcc g++ autoconf automake m4 git make util-linux-dev libuuid libevent-dev gperf boost-dev
./configure
make

I pushed a new fix that compiles both on alpine with musl and ubuntu with glibc.

It uses _GNU_SOURCE for the ifdef because __USE_GNU seems to be "internal"

Thibault

esabol commented 4 years ago

I pushed a new fix that compiles both on alpine with musl and ubuntu with glibc.

It uses _GNU_SOURCE for the ifdef because __USE_GNU seems to be "internal"

I think _GNU_SOURCE is meant to be #defined by the developer in order to enable certain features in glibc. So that's not what we want here either, I think, but maybe I'm wrong.

We want a #define that detects whether or not environ is declared, and I think _USE_GNU is the correct one for that, as shown in the snippet from your unistd.h.

So apparently the problem is that musl doesn't #define __USE_GNU unless _GNU_SOURCE is #defined, which it doesn't do by default, right? If so, I think what we need is something like

#if __MUSL
# define _GNU_SOURCE
#endif

at the top of this file. The problem is that musl doesn't #define a __MUSL or the like, according to this:

https://stackoverflow.com/questions/7296963/gnu-source-and-use-gnu EDIT: Pasted the wrong URL. I meant this one: https://stackoverflow.com/questions/58177815/how-to-actually-detect-musl-libc

That StackOverflow question has an interesting comment: "... consider checking for GNU_SOURCE but NOT _GLIBC (or _GLIB_MINOR, or __GNU_LIBRARY)...." That doesn't seem correct, but it might be close. Maybe #if defined(_GLIBC) && !defined(_GNU_SOURCE)?

EDIT: No, that's obviously wrong.... Ugh. Maybe this?

#if !defined(_GLIBC) && !defined(__USE_GNU) /* detect musl */
# define _GNU_SOURCE
#endif

Maybe need to add some conditions there for BSD and Mac?

EDIT: Ugh, I think I'm just completely confused and need something to eat... Sorry.

thibaultduponchelle commented 4 years ago

I don't know :hand_over_mouth:

I had the feeling that _GNU_SOURCE was more "standard" between various libc:

glibc = both USE_GNU and _GNU_SOURCE (in features.h if _GNU_SOURCE implies __USE_GNU) uClibc = USE_GNU and _GNU_SOURCE (same features.h mechanism where __USE_GNU is implied by _GNU_SOURCE) dietlibc = almost only _GNU_SOURCE (only 1 occurrence of __USE_GNU) musl = only _GNU_SOURCE

But honestly I repeat, I don't know :grin:

I leave you the final word to decide :+1:

esabol commented 4 years ago

Well, I think it should be clear by now that I don't know either. :)

As near as I can tell, nothing in the gearmand build apparatus #defines either _GNU_SOURCE or __USE_GNU, so I'm kind of befuddled as to how this code compiles at all.

Everything I've read online says that _GNU_SOURCE is supposed to be #defined by developers in order to enable GNU extensions in glibc. Source: https://lwn.net/Articles/590381/

thibaultduponchelle commented 4 years ago 
As near as I can tell, nothing in the gearmand build apparatus #defines either _GNU_SOURCE or __USE_GNU, so I'm kind of befuddled as to how this code compiles at all.

I think this is wrong.

In gearman, _GNU_SOURCE (and GNU extensions in general) seems to be set by configure "on systems that have them."

It appears in m4/extensions.m4 (but this "undef is not defined" is weird) then later in the gear_config.in (same weird logic) in the release tarball then later in the gear_config.h after the ./configure (but this time it sets well the _GNU_SOURCE).

So I think it is set "when it's possible" and I just checked with musl and it is also well set.

~Then this fix seems to be valid~ (see edit below), but you ~can~ should also target more specifically MacOS (as we believe it is the original goal of this ifdef).

EDIT: it appears that on MacOS, the configure also sets _GNU_SOURCE, I tested in a github action see the logs. I don't know what is doing the libc on MacOS, maybe it does not set __USE_GNU from _GNU_SOURCE and that's why the current code (before my fix) was valid.

EDIT: on my side I used -fpermissive and it seems to fix some CPANTesters reports running on alpine

Thibault

thibaultduponchelle commented 4 years ago

I edited several times the previous message, sorry for that, as we agreed both, this problem and fix not cristal clear for us :laughing:

thibaultduponchelle commented 4 years ago

I tested some variation on mac os github actions:

:heavy_check_mark: Vanilla 1.1.19.1: OK: https://github.com/thibaultduponchelle/messy-perl-ci-workflows/runs/902922433?check_suite_focus=true

:heavy_check_mark: No ifndef (but keep static char **environ= NULL;) : OK: https://github.com/thibaultduponchelle/messy-perl-ci-workflows/runs/902923039?check_suite_focus=true

:x: ifndef _GNU_SOURCE: KO: https://github.com/thibaultduponchelle/messy-perl-ci-workflows/runs/902795129?check_suite_focus=true libtest/cmdline.cc:264:78: error: use of undeclared identifier 'environ'

:x: No ifndef and no environ declaration: KO: https://github.com/thibaultduponchelle/messy-perl-ci-workflows/runs/902865330?check_suite_focus=true libtest/cmdline.cc:260:78: error: use of undeclared identifier 'environ'

Means:

Then #ifndef __USE_GNU will impact both musl and macOS which is bad :x: And #ifndef _USE_SOURCE will protect almost everybody which is bad :x:

My fix is wrong for mac os and current ifndef is wrong for musl.

The tarball used :

The github actions:

I hope this will help the debates :smile:

Maybe I can try also extern char **environ= NULL; instead of static char **environ= NULL; not tested yet.

thibaultduponchelle commented 4 years ago

Back again, replacing static char **environ= NULL; per extern char **environ= NULL; compiles on Mac OS, Alpine and Ubuntu.

See Mac OS build here from this github action using this modified gearman 1.1.19.1

I updated the PR, I remind you the original error was to redefine with static instead extern.

I'm not sure what other kind of impact it could have.

If you have better idea or simply prefer another solution, I let you decide, no problem :smile:

esabol commented 4 years ago

Thank you for extensive testing! You rock, @thibaultduponchelle. :)

Could you test something like this on macOS?

#if defined(__APPLE__) && __APPLE__
# include <crt_externs.h>
# define environ (*_NSGetEnviron ()) 
#elif !defined(_GNU_SOURCE)
extern char **environ= NULL;
#endif

There’s also FreeBSD. I don’t think we officially support FreeBSD, but I know we’ve made some efforts in that arena to get it to compile at least. Not expecting you to test this on FreeBSD, but it might be worth researching how FreeBSD defines environ in unistd.h, assuming it does.

EDIT: Hmmm, I don't see any mention of environ in FreeBSD's unistd.h, so I think the above should work. Anyone? I presume _GNU_SOURCE isn't defined on FreeBSD, right?

esabol commented 4 years ago

As near as I can tell, nothing in the gearmand build apparatus #defines either _GNU_SOURCE or __USE_GNU, so I'm kind of befuddled as to how this code compiles at all.

I think this is wrong.

Oh, I knew it was wrong, but I just couldn’t find where it happened. Thanks for giving me some clarity. :)

So I think it is set "when it's possible" and I just checked with musl and it is also well set.

Ok, that’s good to know!

thibaultduponchelle commented 4 years ago

:heavy_check_mark: Your code snippet compiles OK on Mac OS, Ubuntu and Alpine :man_dancing:

See github ci build using this tarball

Concerning freebsd, I don't know, but I think you could add a freebsd ci with cirrus-ci. And IMHO you can as well add a mac os in your travis for non-reg purposes.

Seems like we have a fix, right ? :smiley:

thibaultduponchelle commented 4 years ago

Wait @p-alik thank you for approval but this PR still not contains the final fix, will add it right now

esabol commented 4 years ago

Thank you, @thibaultduponchelle, for seeing it through and doing so much testing. Cheers!

SpamapS commented 4 years ago

Can somebody check and make sure the libgearman produced by this is ABI compatible with 1.1.19.1?

I worry that we may need to bump soname if we have changed this variable's declaration.

On Fri, Jul 24, 2020, 8:46 AM Ed Sabol notifications@github.com wrote:

Merged #289 https://github.com/gearman/gearmand/pull/289 into master.

— You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub https://github.com/gearman/gearmand/pull/289#event-3584481103, or unsubscribe https://github.com/notifications/unsubscribe-auth/AADS6YCHJMUVTVTMT6YKON3R5GUENANCNFSM4PCRBXTQ .

esabol commented 4 years ago

Can somebody check and make sure the libgearman produced by this is ABI compatible with 1.1.19.1? I worry that we may need to bump soname if we have changed this variable's declaration.

Hi, Clint. Is libtest code in libgearman.so? I assumed libtest was only used in the test suite.

This variable declaration has changed on macOS and other non-glibc platforms, like FreeBSD and alpine.