Define os_posix in gyp files to get new Unix platforms added quickly

The gyp files don't have a way to refer to POSIX-like OS's as a group, so I defined os_posix, toolkit_uses_gtk, and use_x11 in common.gypi to handle them.  I used something similar when porting Chromium 10 and 11 to OpenBSD and Solaris, plus a version of this patch has been tested on FreeBSD.

Chromium has also been built on other BSDs, so rather than adding each Unix to every gyp file individually every time another port is added, these broad defines can be used instead and modified with specific logic only where necessary.

I included a few modified gyp files so the usage can be seen.  I also added sunos5 to some grd/html files and set the default host_arch on i86pc solaris to ia32.

BUG=0
TEST={}

Patch by ruben <chromium@hybridsource.org>;.
Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=85154

[ruben, 2011-05-09 07:42:39]

I've added a define for cross-Unix support to the gyp files.  It was suggested
that Mark might be the person to review this patch.  Additional feedback welcome
from Tony and Peter.

[Mark Mentovai, 2011-05-09 13:52:53]

In C++ macro-land, we call this OS_POSIX, and in filename-land, we automatically
include files ending in _posix only on POSIXish platforms, so it’d probably be
wise for this to match (os_posix) in GYP-land too.

[ruben, 2011-05-09 19:48:49]

On 2011/05/09 13:52:53, Mark Mentovai wrote:
> In C++ macro-land, we call this OS_POSIX, and in filename-land, we
automatically
> include files ending in _posix only on POSIXish platforms, so it’d probably be
> wise for this to match (os_posix) in GYP-land too.
OK, it's os_posix now.

[Mark Mentovai, 2011-05-09 19:54:49]

http://codereview.chromium.org/6965007/diff/4001/app/app.gyp
File app/app.gyp (right):

http://codereview.chromium.org/6965007/diff/4001/app/app.gyp#newcode1
app/app.gyp:1: # Copyright (c) 2010 The Chromium Authors. All rights reserved.
Fix the change description too, which still refers to os_nix.

http://codereview.chromium.org/6965007/diff/4001/build/common.gypi
File build/common.gypi (right):

http://codereview.chromium.org/6965007/diff/4001/build/common.gypi#newcode47
build/common.gypi:47: 'os_posix%': 1,
Now we’re getting into a semantic difference thing. (Sorry I didn’t look at this
bit before.)

_posix.cc and OS_POSIX both mean “all of the OSes you’ve described here AND
Mac.”

We waffled on this a bit but ultimately decided that it wasn’t worth having a
generic “posix but not Mac” macro or filename extension because most of those
uses would be better served by choosing a name describing the real feature at
stake, like gtk or x11.

I think it would be confusing to choose a different definition for “posix” in
GYP-speak.

[ruben, 2011-05-09 19:55:19]

On 2011/05/09 19:48:49, ruben wrote:
> OK, it's os_posix now.
Oh, just so we're clear though, OS_POSIX includes OS_MACOSX, which is why I
replace OS_LINUX with OS_POSIX && !OS_MACOSX in a lot of the code with my ports.
 But in the gyp files, it depends if you want to follow that exact same pattern
or have a slightly different definition for os_posix, ie not including
OS=="mac".  I just defined os_nix instead.

[ruben, 2011-05-09 19:59:22]

On 2011/05/09 19:54:49, Mark Mentovai wrote:
> http://codereview.chromium.org/6965007/diff/4001/build/common.gypi#newcode47
> build/common.gypi:47: 'os_posix%': 1,
> Now we’re getting into a semantic difference thing. (Sorry I didn’t look at
this
> bit before.)
> 
> _posix.cc and OS_POSIX both mean “all of the OSes you’ve described here AND
> Mac.”
> 
> We waffled on this a bit but ultimately decided that it wasn’t worth having a
> generic “posix but not Mac” macro or filename extension because most of
those
> uses would be better served by choosing a name describing the real feature at
> stake, like gtk or x11.
> 
> I think it would be confusing to choose a different definition for “posix” in
> GYP-speak.
Looks like we remembered this at the same time. ;) I'll go back to os_nix or use
whatever replacement you want, just let me know what you think works best.

[Mark Mentovai, 2011-05-09 20:01:19]

If the features all fall into a few big buckets like “gtk” and “x11”,
you should use those, even if it winds up being more than one
variable, as long as it’s not too incredibly out of hand.

[ruben, 2011-05-09 20:13:13]

On 2011/05/09 20:01:19, Mark Mentovai wrote:
> If the features all fall into a few big buckets like “gtk” and “x11”,
> you should use those, even if it winds up being more than one
> variable, as long as it’s not too incredibly out of hand.
Hmm, I will define those too, but there's still a group of gyp files that refer
to just posix-ish APIs that aren't compiled on the mac.  Inside the code, the
ifdef commonly used for that now is

#if defined(OS_LINUX)

which I've been replacing with

#if defined(OS_POSIX) && !defined(OS_MACOSX)

I could mirror that in the gyp files with

os_posix==1 and OS!="mac" 

everywhere or use the shorter os_nix==1, up to you which reads better.

[tony, 2011-05-09 20:13:17]

http://codereview.chromium.org/6965007/diff/4001/app/resources/app_resources.grd
File app/resources/app_resources.grd (right):

http://codereview.chromium.org/6965007/diff/4001/app/resources/app_resources....
app/resources/app_resources.grd:21: <if expr="not pp_ifdef('toolkit_views') and
(os == 'linux2' or os.find('bsd') != -1 or os == 'sunos5')">
This is fine for now.  In the long run, we might want to make grit smarter about
this.

http://codereview.chromium.org/6965007/diff/4001/build/common.gypi
File build/common.gypi (right):

http://codereview.chromium.org/6965007/diff/4001/build/common.gypi#newcode1040
build/common.gypi:1040: ['OS=="solaris"', {'werror%': '',}],
Nit: Can we put freebsd/openbsd/solaris in the else part of the OS=="linux"
condition?

http://codereview.chromium.org/6965007/diff/4001/build/common.gypi#newcode1634
build/common.gypi:1634: ['disable_nacl==1', {
This looks like a functional change.  Looks like if we're on bsd/solaris, we
should force disable_nacl=1 in an above condition.  Can you make this change in
a separate patch?

http://codereview.chromium.org/6965007/diff/4001/build/features_override.gypi
File build/features_override.gypi (right):

http://codereview.chromium.org/6965007/diff/4001/build/features_override.gypi...
build/features_override.gypi:91: ['use_accelerated_compositing==1', {
Same thing as disable_nacl, seems like we should be setting
use_accelerated_compositing properly common.gypi.

[Mark Mentovai, 2011-05-09 20:16:07]

I think it’s best to mimic the logic we use elsewhere, since it’s
well-understood and even if it’s not as precise as it could be, I
don’t want to introduce confusion.

[ruben, 2011-05-10 07:41:09]

On 2011/05/09 20:16:07, Mark Mentovai wrote:
> I think it’s best to mimic the logic we use elsewhere, since it’s
> well-understood and even if it’s not as precise as it could be, I
> don’t want to introduce confusion.
Alright, I modified the patch to use more specific defines like toolkit_gtk and
use_x11, while POSIX-ish files get 'os_posix==1 and OS!="mac"'.  Where there was
a combination of POSIX and Gtk files listed in a gyp file, I used toolkit_gtk. 
For the time being all three are interchangeable on Unix platforms but I used
the one that seemed most appropriate, while the POSIX/Gtk mixed locations can be
split up later, if and when that's necessary.

I ended up having to use os_nix locally in common.gypi because of the nested
scoping rules in gyp files, but os_nix isn't defined globally.

Tony, I made the werror change you asked for.  As for disable_nacl, I thought it
might be better not to put a list of Unix OS's that it doesn't build on, and
instead people can just set the disable_nacl flag externally on those OS's. 
Regarding use_accelerated_compositing, it seemed useless to list every supported
OS there, so I cleaned it up.  I pulled in a small handful of these gyp OS
clean-ups into this patch: I can remove them if you think they're distracting.

[Mark Mentovai, 2011-05-10 14:20:27]

Much improved.

Have you signed the CLA? http://code.google.com/legal/individual-cla-v1.0.html
if you’re an individual contributor, or
http://code.google.com/legal/corporate-cla-v1.0.html if your contributions
belong to another entity.

Once the CLA bits are taken care of, you can add yourself to the AUTHORS file
and include that in this change.

http://codereview.chromium.org/6965007/diff/12001/app/resources/app_resources...
File app/resources/app_resources.grd (right):

http://codereview.chromium.org/6965007/diff/12001/app/resources/app_resources...
app/resources/app_resources.grd:21: <if expr="not pp_ifdef('toolkit_views') and
(os == 'linux2' or os.find('bsd') != -1 or os == 'sunos5')">
I’m deferring to Tony’s review in this file.

http://codereview.chromium.org/6965007/diff/12001/base/allocator/allocator.gyp
File base/allocator/allocator.gyp (right):

http://codereview.chromium.org/6965007/diff/12001/base/allocator/allocator.gy...
base/allocator/allocator.gyp:312: ['OS=="linux" or OS=="freebsd" or
OS=="solaris"', {
Why have you dropped OS=="openbsd" from this expression?

http://codereview.chromium.org/6965007/diff/12001/base/base.gypi
File base/base.gypi (right):

http://codereview.chromium.org/6965007/diff/12001/base/base.gypi#newcode488
base/base.gypi:488: 'process_linux.cc',
Ideally, _linux files wouldn’t be built if OS!="linux".

http://codereview.chromium.org/6965007/diff/12001/build/common.gypi
File build/common.gypi (right):

http://codereview.chromium.org/6965007/diff/12001/build/common.gypi#newcode48
build/common.gypi:48: }, {  # OS=="win" or OS=="mac"
This comment isn’t really correct. Since keeping it in sync with the four OSes
you list explicitly above will be painful, it’s best to just take out the "win"
and "mac" bit and just write |# else|. As evidence that keeping this in sync
will be a problem, the comment you’re replacing just said |# OS!="linux"|, which
was correct once upon a time, but became wrong once other OSes were added to the
list.

http://codereview.chromium.org/6965007/diff/12001/build/common.gypi#newcode508
build/common.gypi:508: ['OS=="mac" or (os_posix==1 and chromeos==0 and
target_arch!="arm")', {
Since mac is os_posix, this can be simplified.

http://codereview.chromium.org/6965007/diff/12001/build/common.gypi#newcode1055
build/common.gypi:1055: 'conditions': [['OS=="linux"', {'werror%': '-Werror',
Can you reformat this to match the traditional formatting used for GYP
conditions? The original collapsed formatting was fine when everything fit on
one line and there weren’t any else clauses, but now that this has matured, it
looks sloppy and is hard to follow.

[tony, 2011-05-10 17:16:28]

Evan tells me that in build/build_config.h, we have TOOLKIT_GTK and
TOOLKIT_VIEWS, which are mutually exclusive.  However, there's also
TOOLKIT_USES_GTK which is for gtk code in both TOOLKIT_GTK and TOOLKIT_VIEWS.

The current patch has toolkit_gtk, but it's not mutually exclusive to
toolkit_views.  I'm sorry for asking for another rewrite, but I think the
easiest thing to do is to just rename toolkit_gtk to toolkit_uses_gtk and the
current usage would be correct.  We can add toolkit_gtk if it turns out we need
it.

http://codereview.chromium.org/6965007/diff/12001/app/resources/app_resources...
File app/resources/app_resources.grd (right):

http://codereview.chromium.org/6965007/diff/12001/app/resources/app_resources...
app/resources/app_resources.grd:21: <if expr="not pp_ifdef('toolkit_views') and
(os == 'linux2' or os.find('bsd') != -1 or os == 'sunos5')">
On 2011/05/10 14:20:27, Mark Mentovai wrote:
> I’m deferring to Tony’s review in this file.

Yeah, I mentioned in an earlier review that this is fine for now.  We need to
modify grit to make this cleaner.

http://codereview.chromium.org/6965007/diff/12001/base/base.gypi
File base/base.gypi (right):

http://codereview.chromium.org/6965007/diff/12001/base/base.gypi#newcode357
base/base.gypi:357: [ 'os_posix!=1 or OS=="mac"', {
Nit: I would make this OS=="win" or OS=="mac".

http://codereview.chromium.org/6965007/diff/12001/base/base.gypi#newcode445
base/base.gypi:445: [ 'toolkit_gtk==1', {
This would become toolkit_uses_gtk.

[ruben, 2011-05-10 21:40:39]

On 2011/05/10 14:20:27, Mark Mentovai wrote:
> Have you signed the CLA? 
I signed the individual CLA more than a year ago and have had several patches
for FreeBSD and Unix support committed since then. 
 
>
http://codereview.chromium.org/6965007/diff/12001/base/allocator/allocator.gy...
> base/allocator/allocator.gyp:312: ['OS=="linux" or OS=="freebsd" or
> OS=="solaris"', {
> Why have you dropped OS=="openbsd" from this expression?
tcmalloc wouldn't compile for me on OpenBSD.  It builds on Solaris but wouldn't
run.
 
> http://codereview.chromium.org/6965007/diff/12001/base/base.gypi
> File base/base.gypi (right):
> 
> http://codereview.chromium.org/6965007/diff/12001/base/base.gypi#newcode488
> base/base.gypi:488: 'process_linux.cc',
> Ideally, _linux files wouldn’t be built if OS!="linux".
Removed.

> http://codereview.chromium.org/6965007/diff/12001/build/common.gypi#newcode48
> build/common.gypi:48: }, {  # OS=="win" or OS=="mac"
> This comment isn’t really correct. Since keeping it in sync with the four OSes
> you list explicitly above will be painful, it’s best to just take out the
"win"
> and "mac" bit and just write |# else|. As evidence that keeping this in sync
> will be a problem, the comment you’re replacing just said |# OS!="linux"|,
which
> was correct once upon a time, but became wrong once other OSes were added to
the
> list.
I don't understand why the new comment isn't correct.  The defines below have
always been set only on win and mac.  Adding another Unix to the list above
wouldn't change that.

> http://codereview.chromium.org/6965007/diff/12001/build/common.gypi#newcode508
> build/common.gypi:508: ['OS=="mac" or (os_posix==1 and chromeos==0 and
> target_arch!="arm")', {
> Since mac is os_posix, this can be simplified.
Fixed.

>
http://codereview.chromium.org/6965007/diff/12001/build/common.gypi#newcode1055
> build/common.gypi:1055: 'conditions': [['OS=="linux"', {'werror%': '-Werror',
> Can you reformat this to match the traditional formatting used for GYP
> conditions? The original collapsed formatting was fine when everything fit on
> one line and there weren’t any else clauses, but now that this has matured, it
> looks sloppy and is hard to follow.
Fixed.

On 2011/05/10 17:16:28, tony wrote:
> The current patch has toolkit_gtk, but it's not mutually exclusive to
> toolkit_views.  I'm sorry for asking for another rewrite, but I think the
> easiest thing to do is to just rename toolkit_gtk to toolkit_uses_gtk and the
> current usage would be correct.  We can add toolkit_gtk if it turns out we
need
> it.
Switched the define to toolkit_uses_gtk.
 
> http://codereview.chromium.org/6965007/diff/12001/base/base.gypi#newcode357
> base/base.gypi:357: [ 'os_posix!=1 or OS=="mac"', {
> Nit: I would make this OS=="win" or OS=="mac".
Fixed.

[Mark Mentovai, 2011-05-10 21:45:09]

I can’t find your CLA on file and I don’t see you in the AUTHORS file. You can
contact me privately to help me locate your CLA.

http://codereview.chromium.org/6965007/diff/12001/build/common.gypi
File build/common.gypi (right):

http://codereview.chromium.org/6965007/diff/12001/build/common.gypi#newcode48
build/common.gypi:48: }, {  # OS=="win" or OS=="mac"
This "else" comment isn’t correct because it doesn’t match the logic of the "if"
above.

[ruben, 2011-05-10 22:03:44]

On 2011/05/10 21:45:09, Mark Mentovai wrote:
> I can’t find your CLA on file and I don’t see you in the AUTHORS file. You can
> contact me privately to help me locate your CLA.
I sent you info on how to find my CLA.

> http://codereview.chromium.org/6965007/diff/12001/build/common.gypi#newcode48
> build/common.gypi:48: }, {  # OS=="win" or OS=="mac"
> This "else" comment isn’t correct because it doesn’t match the logic of the
"if"
> above.
I suppose it is not explicitly the converse, but it is in practice given the
current set of OS possibilities.  Since any new Unix port will have to add their
OS to the list above, I thought explicitly saying here that win/mac go below
might be more clear for those reading.  If you think it's better to leave it
blank, I'll take it out.

[tony, 2011-05-10 22:14:26]

I defer final judgement to Mark, but looks fine to me.

http://codereview.chromium.org/6965007/diff/10006/base/allocator/allocator.gyp
File base/allocator/allocator.gyp (right):

http://codereview.chromium.org/6965007/diff/10006/base/allocator/allocator.gy...
base/allocator/allocator.gyp:312: ['OS=="linux" or OS=="freebsd" or
OS=="solaris"', {
Nit: Can you add a comment here saying that tcmalloc doesn't work on openbsd?

http://codereview.chromium.org/6965007/diff/10006/build/all.gyp
File build/all.gyp (right):

http://codereview.chromium.org/6965007/diff/10006/build/all.gyp#newcode62
build/all.gyp:62: ['OS=="mac" or OS=="linux"', {
Nit: Maybe add a comment here about why *bsd/solaris don't work?

[Mark Mentovai, 2011-05-10 22:19:00]

Fix Tony’s nits too and I think we’ll be good to go.

http://codereview.chromium.org/6965007/diff/10006/base/base.gypi
File base/base.gypi (right):

http://codereview.chromium.org/6965007/diff/10006/base/base.gypi#newcode357
base/base.gypi:357: [ 'OS=="win" or OS=="mac"', {
I think this is best as "toolkit_uses_gtk==0"

[ruben, 2011-05-10 22:30:25]

On 2011/05/10 22:14:26, tony wrote:
>
http://codereview.chromium.org/6965007/diff/10006/base/allocator/allocator.gy...
> base/allocator/allocator.gyp:312: ['OS=="linux" or OS=="freebsd" or
> OS=="solaris"', {
> Nit: Can you add a comment here saying that tcmalloc doesn't work on openbsd?
I think tcmalloc should work, just may be an old version or whatever, didn't
bother debugging it.
 
> http://codereview.chromium.org/6965007/diff/10006/build/all.gyp#newcode62
> build/all.gyp:62: ['OS=="mac" or OS=="linux"', {
> Nit: Maybe add a comment here about why *bsd/solaris don't work?
Dunno why but when it didn't compile on FreeBSD, I just started using the system
yasm.  I think these were blindly added in the past so I'm just removing them.

> http://codereview.chromium.org/6965007/diff/10006/base/base.gypi#newcode357
> base/base.gypi:357: [ 'OS=="win" or OS=="mac"', {
> I think this is best as "toolkit_uses_gtk==0"
But that isn't defined till below.  I'll just remove it like you originally
said.

[ruben, 2011-05-10 23:09:54]

On 2011/05/10 22:19:00, Mark Mentovai wrote:
> Fix Tony’s nits too and I think we’ll be good to go.
I think it's better not to put in comments for that stuff.  Adding a comment for
every OS/feature combination saying that it "might work" would just add too much
noise.  We're a long way from even getting the browser to compile on those
Unices after this patch: I have multiple gyp file patches alone that I'll submit
once this first one is in.
 
> http://codereview.chromium.org/6965007/diff/10006/base/base.gypi#newcode357
> base/base.gypi:357: [ 'OS=="win" or OS=="mac"', {
> I think this is best as "toolkit_uses_gtk==0"
Removed like you originally asked.

[ruben, 2011-05-10 23:16:16]

On 2011/05/10 22:19:00, Mark Mentovai wrote:
> http://codereview.chromium.org/6965007/diff/10006/base/base.gypi#newcode357
> base/base.gypi:357: [ 'OS=="win" or OS=="mac"', {
> I think this is best as "toolkit_uses_gtk==0"
Sorry, I got this confused with the common.gypi comment.  Fixed both.

[Mark Mentovai, 2011-05-11 16:36:55]

LGTM. You can try the “commit” button at your leisure.

[commit-bot: I haz the power, 2011-05-12 02:02:46]

Presubmit check for 6965007-17002 failed and returned exit status 1.

Running presubmit commit checks ...

** Presubmit Messages **
If this change has an associated bug, add BUG=[bug number].

If this change requires manual test instructions to QA team, add
TEST=[instructions].

** Presubmit Warnings **
Found lines longer than 80 characters (first 5 shown).
  build/common.gypi, line 46, 107 chars \
  build/all.gyp, line 288, 82 chars \
  chrome/browser/resources/options/personal_options.html, line 106, 104 chars \
  app/resources/app_resources.grd, line 21, 110 chars \
  app/resources/app_resources.grd, line 45, 110 chars

Presubmit checks took 1.2s to calculate.

[ruben, 2011-05-12 02:14:57]

On 2011/05/12 02:02:46, commit-bot wrote:
> ** Presubmit Warnings **
> Found lines longer than 80 characters (first 5 shown).
>   build/common.gypi, line 46, 107 chars \
>   build/all.gyp, line 288, 82 chars \
>   chrome/browser/resources/options/personal_options.html, line 106, 104 chars
\
>   app/resources/app_resources.grd, line 21, 110 chars \
>   app/resources/app_resources.grd, line 45, 110 chars
> 
> Presubmit checks took 1.2s to calculate.
Can someone with commit access force this through?  All the files in question
have other lines already that are even longer, and I'm not even sure what the
syntax would be for some of them, like the .grd files, to split them up.

[Mark Mentovai, 2011-05-12 16:57:31]

I’ve got the try bots crankin’.

[Mark Mentovai, 2011-05-12 18:19:20]

Committed r85154, thanks!