Created attachment 66928 [details] [review]
remove null instruction in new rectangle tool

Created attachment 66929 [details] [review]
Remove _POSIX_SOURCE and _SVID_SOURCE macros

2006-06-07  Sven Neumann  <sven@gimp.org>

	* app/tools/gimpnewrectselecttool.c: removed extra semicolon to
	fix build on Solaris (bug #344203).

Applying the patch from comment #2 would reopen bug #342390.  I don't think we absolutely need to compile with -ansi -pedantic but it would be nice if we could find a solution that achieves this goal and works on Solaris.

For select(), the fix should be:
  1) fix configure.in to search for sys/select.h and not sys/types.h
  Here is the beginning of man select(2) on my Linux box
SYNOPSIS
       /* From Posix 1003.1-2001 */
       #include <sys/select.h>

       /* From earlier standards */
       #include <sys/time.h>
       #include <sys/types.h>
       #include <unistd.h>
  2) include sys/select.h in libgimp/gimp.c

For SA_RESTART, I did not find any reference in my POSIX document. And the way it is in sys/signal.h make me think it's not posix.
  Here is what I read in sys/signal.h on Solaris 8 (maybe it was changed later):
    #if !defined(_POSIX_C_SOURCE) && ...[other flags]
    #define SA_RESTART 0x00000004
    #endif
So here I'm stuck.

SA_RESTART is definitely non-POSIX. FreeBSD's sys/signal.h is pretty clear on this:

----BEGIN FreeBSD sys/signal.h excerpt----
#if !defined(_POSIX_SOURCE)

#define sa_sigaction    __sigaction_u.__sa_sigaction

#define SA_ONSTACK      0x0001  /* take signal on signal stack */
#define SA_RESTART      0x0002  /* restart system call on signal return */
----END FreeBSD sys/signal.h excerpt----

The Solaris headers are a bit more lax:

----BEGIN Solaris sys/signal.h excerpt----
#if defined(__EXTENSIONS__) || defined(_KERNEL) || \
        (!defined(_STRICT_STDC) && !defined(_POSIX_C_SOURCE)) || \
        defined(_XPG4_2)

                        /* non-conformant ANSI compilation      */

/* definitions for the sa_flags field */
#define SA_ONSTACK      0x00000001
#define SA_RESETHAND    0x00000002
#define SA_RESTART      0x00000004
#endif
----END Solaris sys/signal.h excerpt----

For reference, _XPG4_2 is defined as follows:

----BEGIN Solaris sys/feature_tests.h excerpt----
/* X/Open CAE Specification, Issue 4, Version 2 */
#elif (defined(_XOPEN_SOURCE) && _XOPEN_SOURCE_EXTENDED - 0 == 1)
#define _XPG4_2
#define _XPG4
#define _XPG3
----END Solaris sys/feature_tests.h excerpt----

(It also gets defined for later X/Open revs. By the way, the above is taken from Solaris 10, but I cross-checked with SunOS 5.6 and it is substantially the same.)

Perhaps an _XOPEN_SOURCE(_EXTENDED) definition will make everything happy? I only went with the _{POSIX,SVID}_SOURCE macros earlier as I viewed them as more minimalistic.

We should just use _GNU_SOURCE everywhere.

_GNU_SOURCE is a GNU extension, only recognized by glibc-based systems. If we want to be portable, we need to deal with the feature-macro mess.

Alternately: Is the use of SA_RESTART necessary? Could that bit be reimplemented with more standards-friendly functionality, if not something out of GLib?

Yes, but by default, *no* macro on the platforms we support does the right thing and supports the feature set we need. So we aren't really solving anything by introducing them, except to satisfy people who want to build with gcc using -ansi -pedantic. That's what _GNU_SOURCE solves.

I note the glibc documentationr recommends using _GNU_SOURCE in new programs. I don't know if that is just a reflection on FSF politics or not.

The demonstrated brittleness of using the other _FOO_SOURCE macros negates any theroretical advantages.

Certainly one could use _GNU_SOURCE on systems that recognize it, and a separate set of appropriate feature macros for those that don't, though that would be painful to maintain on a per-file basis. Personally, I'd avoid _GNU_SOURCE on the grounds that you generally don't want to allow GNU C library extensions in code that is intended to be portable.

Yes, feature macros are a minefield to navigate. We could use a Solaris-specific -D__EXTENSIONS__ directive to address the bug at hand, though I'm sure we would all prefer a more general solution. In which case... either we have to figure out the necessary feature macros, eliminate the use of SA_RESTART, or allow that GIMP won't build "out-of-the-box" on Solaris. Are there really any other options?

No, I mean just define _GNU_SOURCE and that's all. Simply to support -ansi -pedantic on glibc systems.

Using any other macros seems to *add* portability problems in practice, since the code has been just fine without them for years.

What would be a more "standards-friendly" way of getting
the effect of SA_RESTART?

Well, right now it's not building on Solaris. How shall this be resolved?

In the absolute worst case, we could put differing sets of feature macros into system-specific #ifdefs. It would suck, but it would suck less than requiring packagers to hack and chop at the source just to get it to build---especially on a not-so-obscure brand of Unix.

It shall be resolved by *not* using the feature macros. Before it built on solaris just fine.

I feel like a broken record here.

Okay, now I see what you're getting at. "No feature macros" is one of the possible combinations (albeit an empty one), and _GNU_SOURCE would effectively be the same thing on Solaris---while allowing an -ansi -pedantic build to go through on Linux.

I don't think this is going to hold broadly true across most POSIX systems, however, since the default C environment is not always the most permissive one. (Is there anyone doing builds on HP-UX, Tru64, AIX et al.? I have access to these, and could try a test build thereon, though with all the dependencies it would take some time.) I'd be surprised if GIMP builds everywhere without any feature tweaking, given all the different things it's using.

And then you have the issue of GNU extensions creeping into the code. _GNU_SOURCE will define __USE_GNU regardless of __STRICT_ANSI__, so you won't have a good way of ensuring that GNU-isms aren't present (other than building on a Unix machine).

GIMP has historically built on lots of places, and pretty much all the code in question has been in the tree for years. AIX has some issues due to libtool and it's different notion of handling shared libraries, but that's independent of this.

-ansi -pedantic doesn't catch all GNUisms anyway.

Yes, catching all the GNUisms is tricky, since those include nonstandard printf() formats et al. Still, #undef __USE_GNU cuts out a pretty wide swath of 'em.

Maybe we could say e.g.

    #ifdef __GLIBC__
    #  define _BLAH_SOURCE
    #endif

to allow Linux ANSI builds with a minimal namespace, without causing lossage on Solaris and elsewhere. (And so far, it would be just that one file that needs this, wouldn't it?)

I think the equivalent of -ansi -pedantic fot Solaris cc is -Xc
> cc -flags
[...]
-Xc             Compile assuming strict ANSI C conformance

maybe we could try a build using this flag.

Build with strict ANSI on Solaris is rather quick...

checking for GLIB - version >= 2.8.2... no
*** Could not run GLIB test program, checking why...
*** The test program failed to compile or link. See the file config.log for the
*** exact error that occured. This usually means GLIB is incorrectly installed.
configure: error: Test for GLIB failed. See the file 'INSTALL' for help.


Extract from config.log

"/tmp/newgimp/bindev/lib/glib-2.0/include/glibconfig.h", line 46: long long not allowed in Xc mode
"/tmp/newgimp/bindev/lib/glib-2.0/include/glibconfig.h", line 46: invalid type combination
"/tmp/newgimp/bindev/lib/glib-2.0/include/glibconfig.h", line 47: long long not allowed in Xc mode
"/tmp/newgimp/bindev/lib/glib-2.0/include/glibconfig.h", line 47: invalid type combination

For the other issue, I don't really understand the _POSIX flag for ANSI compliance. Does it mean that headers are ANSI compliant depending of POSIX or not?

I think it might be necessary to build GLib in ANSI mode as well. IIRC, glibconfig.h is generated at compile time, and so would reflect the C environment at that point (where "long long" was available).

Would be great if GIMP can build in ANSI mode on Unix, though I'd wonder if the headers might irretrievably disable some needed APIs in that case (no matter what _BLARG_SOURCE directives are given). It's a higher bar than I'd aim for, anyway :-)

this is also an issue on darwin 6
only removing the #define _POSIX_SOURCE from app/main.c and libgimp/gimp.c is required to get things moving again

Daniel, what exactly is the benefit of being able to build GIMP in ANSI mode on UNIX?  I wonder if we would best back out the changes that were introduced for compiling with -ansi -pedantic. I don't see why anyone would want to do this. GIMP used to compile just fine on a large range of platform before we did these changes.

Setting milestone to 2.4 so that we sort this out before the 2.4 release.

[Sven: I'm assuming you meant to say "... GIMP in ANSI mode on _Linux_?"]

Catering to -ansi -pedantic, broadly, is a code hygiene thing. It keeps you aware of when and where you are using POSIX/BSD/SVID/etc.-specific functionality (especially when you don't specify any feature macros globally), alerts you to potential porting issues (e.g. GNU and C99 extensions that most Unix compilers choke on), and generally shows that you've dotted all your i's and crossed all your t's codewise. Most ANSI-averse C code is that way either because it's crap, or because the author doesn't care. (I'd be leery of any "important" code that openly states that it won't compile in ANSI mode, and doesn't want to. It's like refusing to handle a missing snprintf() implementation---willful sloppiness.)

Normally, the feature macros should enable similar functionality across a broad range of Unices (if not all). SA_RESTART is one of those annoying corner cases that confound the effort, however. And I see here that, contrary to my presumptions, folks have been building CVS GIMP successfully on the weird Unices without any feature tweaking---even though this same situation caused -ansi -pedantic to fail gratuitously on Linux.

So the feature macros, at this time, don't buy us any enhanced portability; in fact, they hurt it. The compilers seem to be giving a sufficiently generous namespace in the absence of any macros. But the macros are still what enable ANSI builds to go through on a glibc-based system, which is useful in a development context, and gives the code a sheen of quality that the stable releases really ought to have.

(The whole reason why I filed bug 344203 is that I wanted to hack on GIMP, but I hack with -ansi -pedantic, and the ANSI-unfriendly bits were so few that it made clear sense to address them. The whole "better Unix portability!" thing was supposed to be a freebie, but, oh well.)

That's why I'm suggesting the #ifdef __GLIBC__ bit. It lets -ansi -pedantic work on Linux, without affecting the Unix builds. I assume this would make everyone happy.

See also bug #345062 about compile problems on IRIX.

Daniel, can you provide a patch that adds #ifdef __GLIBC__ where needed so that we can get this sorted out for the upcoming 2.3.10 release ?

This bug report has become a bit confusing, so I ask, is the summary accurate at this point?  Is it true that GIMP won't build on Solaris 8?  (This would make the bug a blocker for the 2.4 release.)  Or is it only true that it won't build when certain non-default compiler flags are used?  (This would make it something desirable to fix but not a blocker.)

*** Bug 345062 has been marked as a duplicate of this bug. ***

GIMP won't build on Solaris 8 because of the attempted fix for bug #342390 is harmful.

#ifdef __GLIBC__ won't work, since you don't get __GLIBC__ defined until you include a glibc header, and by that time it's too late to get any _FOO_SOURCE defines to take effect. (They are in fact, both processed in the same header file: features.h)

2006-06-16  Manish Singh  <yosh@gimp.org>

        * app/errors.c
        * app/main.c
        * app/file/gimprecentlist.c
        * libgimpbase/gimpsignal.c
        * libgimp/gimp.c
        * modules/controller_midi.c
        * plug-ins/common/gqbist.c: use _GNU_SOURCE instead of the other
        _FOO_SOURCE variables to support -ansi -pedantic on glibc systems,
        since anything else breaks compilation of otherwise working code.
        Fixes bug #344203.

Sigh... this is what happens when you write code in your head, and leave the testing for later :]

You're right about the catch-22 definition. The #ifdef would have to key off a macro on the compiler command line (e.g. DEFS += -DUSING_GLIBC), and that would have to be set in the configure script after checking for glibc... admittedly messier than just defining _GNU_SOURCE.

I suppose as long as folks are doing regular Unix builds, GNU extensions can be whacked from the tree before they become entrenched....

*** Bug 345781 has been marked as a duplicate of this bug. ***