GN: nacl_defines: Drop _BSD_SOURCE, _DEFAULT_SOURCE; conditionalize on OS

GN: nacl_defines: Drop _BSD_SOURCE, _DEFAULT_SOURCE; conditionalize on OS

Drop _BSD_SOURCE and _DEFAULT_SOURCE defines, which are redundant with
_GNU_SOURCE on Linux.  This brings the set affecting Linux in line with
the standalone NaCl build.

Conditionalize these defines to affect only Linux, Android, and NaCl
toolchains.  That is, none of these are used on Mac (or Windows).  This
makes the GN build consistent with the GYP and Scons builds.  On Mac,
defining _POSIX_C_SOURCE (without also defining _DARWIN_C_SOURCE) makes
the system headers disable some symbols that NaCl sources need.

BUG=545236
R=dpranke@chromium.org, mseaborn@chromium.org

Committed: https://crrev.com/2640ff9632d74c85964e100b34811f90569fe510
Cr-Commit-Position: refs/heads/master@{#355327}

[Mark Seaborn, 2015-10-20 00:21:00]

LGTM

https://codereview.chromium.org/1408273003/diff/1/build/config/nacl/BUILD.gn
File build/config/nacl/BUILD.gn (right):

https://codereview.chromium.org/1408273003/diff/1/build/config/nacl/BUILD.gn#...
build/config/nacl/BUILD.gn:13: "_DARWIN_C_SOURCE=1",
It seems like some of the reasoning from the commit message should go in
comments here.

In the commit message, can you point to
https://codereview.chromium.org/1411473004/ for context, and mention that you're
syncing the GN build with that?

[Dirk Pranke, 2015-10-20 01:01:00]

Adding command line defines that aren't even needed seems like  source of
confusion and potentially hidden weirdness, so I'd probably go the extra three
lines and make it conditional :).

But, then again, I have perhaps a much stronger distaste of command line defines
than most people.

[Dirk Pranke, 2015-10-20 01:01:22]

On 2015/10/20 01:01:00, Dirk Pranke wrote:
> Adding command line defines that aren't even needed seems like  source of
> confusion and potentially hidden weirdness, so I'd probably go the extra three
> lines and make it conditional :).
> 
> But, then again, I have perhaps a much stronger distaste of command line
defines
> than most people.

(i.e., lgtm if you decide to ignore my comment).

[Mark Seaborn, 2015-10-20 01:20:08]

On 19 October 2015 at 18:01, <dpranke@chromium.org> wrote:

> Adding command line defines that aren't even needed seems like  source of
> confusion and potentially hidden weirdness, so I'd probably go the extra
> three
> lines and make it conditional :).


Yeah.  Adding a conditional would be more self-documenting than adding the
comment (which I requested) that explains why you cut a corner and didn't
conditionalise it. :-)

Cheers,
Mark

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[Roland McGrath, 2015-10-21 17:13:03]

Description was changed from

==========
GN: nacl_defines: Drop _BSD_SOURCE, _DEFAULT_SOURCE; add _DARWIN_C_SOURCE

Drop _BSD_SOURCE and _DEFAULT_SOURCE defines, which are redundant with
_GNU_SOURCE on Linux.  This brings the set affecting Linux in line with
the standalone NaCl build.

Add _DARWIN_C_SOURCE define.  The system headers on Mac enable some
things with _DARWIN_C_SOURCE or with no _POSIX_C_SOURCE at all that they
disable with _POSIX_C_SOURCE but no _DARWIN_C_SOURCE.  The standalone
NaCl build does not use _DARWIN_C_SOURCE in GYP or SCons, but it also
doesn't use _POSIX_C_SOURCE et al that are in nacl_defines.  In those
builds, _POSIX_C_SOURCE et al are only used for Linux, not for Mac.  In
GN there is a single config providing all of these for NaCl code that is
not conditional on the host system.  Rather than conditionalizing the
nacl_defines set to differ by OS, just adding _DARWIN_C_SOURCE is
simpler; it's harmless on Linux, and the others are harmless on Mac.

BUG=545236
R=dpranke@chromium.org, mseaborn@chromium.org
==========

to

==========
GN: nacl_defines: Drop _BSD_SOURCE, _DEFAULT_SOURCE; conditionalize on OS

Drop _BSD_SOURCE and _DEFAULT_SOURCE defines, which are redundant with
_GNU_SOURCE on Linux.  This brings the set affecting Linux in line with
the standalone NaCl build.

Conditionalize these defines to affect only Linux, Android, and NaCl
toolchains.  That is, none of these are used on Mac (or Windows).  This
makes the GN build consistent with the GYP and Scons builds.  On Mac,
defining _POSIX_C_SOURCE (without also defining _DARWIN_C_SOURCE) makes
the system headers disable some symbols that NaCl sources need.

BUG=545236
R=dpranke@chromium.org, mseaborn@chromium.org
==========

[Roland McGrath, 2015-10-21 17:13:46]

The CQ bit was checked by mcgrathr@chromium.org

[Roland McGrath, 2015-10-21 17:13:46]

The patchset sent to the CQ was uploaded after l-g-t-m from
mseaborn@chromium.org, dpranke@chromium.org
Link to the patchset: https://codereview.chromium.org/1408273003/#ps20001
(title: "conditionalize on os; no _DARWIN_C_SOURCE")

[commit-bot: I haz the power, 2015-10-21 17:14:38]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1408273003/20001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1408273003/20001

[commit-bot: I haz the power, 2015-10-21 17:40:31]

Committed patchset #2 (id:20001)

[commit-bot: I haz the power, 2015-10-21 17:41:32]

Patchset 2 (id:??) landed as
https://crrev.com/2640ff9632d74c85964e100b34811f90569fe510
Cr-Commit-Position: refs/heads/master@{#355327}