GetFlavor shouldn't fallback to 'linux'

[Jan Beich]

I'm trying to make WebRTC code work on BSDs. This needs adding
more 'if' statements which look ugly:

if sys.platform.startswith('dragonfly'):
return 'dragonfly'
if sys.platform.startswith('netbsd'):
return 'netbsd'
if sys.platform.startswith('openbsd'):
return 'openbsd'
if sys.platform.startswith('gnukfreebsd'):
return 'gnukfreebsd'
...

So, why not make life easier for people on non-linux?

Index: pylib/gyp/common.py
===================================================================
--- pylib/gyp/common.py (revision 1533)
+++ pylib/gyp/common.py (working copy)
@@ -366,20 +366,18 @@ def GetFlavor(params):
"""Returns |params.flavor| if it's set, the system's default flavor else."""
flavors = {
'cygwin': 'win',
- 'win32': 'win',
'darwin': 'mac',
+ 'sunos': 'solaris',
}
+ # strip major version or '32' from win32
+ platform = sys.platform.rstrip('0123456789')

if 'flavor' in params:
return params['flavor']
- if sys.platform in flavors:
- return flavors[sys.platform]
- if sys.platform.startswith('sunos'):
- return 'solaris'
- if sys.platform.startswith('freebsd'):
- return 'freebsd'
+ if platform in flavors:
+ return flavors[platform]

- return 'linux'
+ return platform


def CopyTool(flavor, out_path):

[Tony Chang]

Looking at the python docs, it looks like on Linux sys.platform can be linux2. linux3 or linux depending on the python version.  Also, I think in the past, on FreeBSD sys.platform would include the FreeBSD version number (probably on python versions before 2.7.3).

http://docs.python.org/2/library/sys.html#sys.platform

I'm not sure if it's important for gyp to support python versions older than 2.7.3, but it seems like it could be done with little effort. For example, we could do:

for platform_name in ('linux', 'dragonfly', 'netbsd', 'openbsd', 'gukfreebsd'):

    if sys.platform.startswith(platform_name):

        return platform_name

Which is easy to extend.

--

[Jan Beich]

Tony Chang <to...@chromium.org> writes:

> On Thu, Nov 8, 2012 at 5:26 PM, Jan Beich <jbe...@tormail.org> wrote:
>
>> I'm trying to make WebRTC code work on BSDs. This needs adding
>> more 'if' statements which look ugly:
>>
>> if sys.platform.startswith('dragonfly'):
>> return 'dragonfly'
>> if sys.platform.startswith('netbsd'):
>> return 'netbsd'
>> if sys.platform.startswith('openbsd'):
>> return 'openbsd'
>> if sys.platform.startswith('gnukfreebsd'):
>> return 'gnukfreebsd'
>> ...
>>
>> So, why not make life easier for people on non-linux?
>

> Looking at the python docs, it looks like on Linux sys.platform can be
> linux2. linux3 or linux depending on the python version. Also, I think in
> the past, on FreeBSD sys.platform would include the FreeBSD version number
> (probably on python versions before 2.7.3).
>
> http://docs.python.org/2/library/sys.html#sys.platform

Only 'linux' won the battle for special case.

$ python2.4
Python 2.4.5 (#2, Nov 10 2012, 00:40:11)
[GCC 4.7.3 20121027 (prerelease)] on freebsd10
Type "help", "copyright", "credits" or "license" for more information.
>>> import sys
>>> sys.platform
'freebsd10'

$ ./python
Python 3.4.0a0 (default:7cfe8cd4d65e, Nov 10 2012, 00:43:18)
[GCC 4.2.1 Compatible FreeBSD Clang 3.2 (trunk 162107)] on freebsd10
Type "help", "copyright", "credits" or "license" for more information.
>>> import sys
>>> sys.platform
'freebsd10'

> I'm not sure if it's important for gyp to support python versions older
> than 2.7.3, but it seems like it could be done with little effort. For
> example, we could do:
>
> for platform_name in ('linux', 'dragonfly', 'netbsd', 'openbsd',
> 'gukfreebsd'):
> if sys.platform.startswith(platform_name):
> return platform_name
>
> Which is easy to extend.

I'd like to avoid extra churn in common.py when porting a code base to
new system. This isn't limited to the one I'm currently working with.

My patch also puts all system renames in one place, dict 'flavors'.

[Evan Martin]

I don't know this code, but it's not clear to me what "GetFlavor" is
even supposed to *do*. (I think Mark had this same objection.) Does
gyp even care about which kind of Unix you are? The special handling
of Mac in this code I think is because gyp needs to understand about
Mac-specific stuff like .dylib, but as far as gyp goes I think
FreeBSD/sorlaris/etc. should all be treated identically to Linux.

On Thu, Nov 8, 2012 at 5:26 PM, Jan Beich <jbe...@tormail.org> wrote:

> --
>
>
>

[Jan Beich]

Evan Martin <ev...@chromium.org> writes:

> On Thu, Nov 8, 2012 at 5:26 PM, Jan Beich <jbe...@tormail.org> wrote:
>> I'm trying to make WebRTC code work on BSDs. This needs adding
>> more 'if' statements which look ugly:
>

> I don't know this code, but it's not clear to me what "GetFlavor" is
> even supposed to *do*. (I think Mark had this same objection.)

GetFlavor is currently used to set OS variable which can later be
checked in gyp files e.g.,

# A flag for BSD platforms
['OS=="dragonfly" or OS=="freebsd" or OS=="netbsd" or \
OS=="openbsd"', {
'os_bsd%': 1,
}, {
'os_bsd%': 0,
}],
...
['OS=="linux"', {
'defines': [
'WEBRTC_LINUX',
'WEBRTC_THREAD_RR',
],
}],
['os_bsd==1', {
'defines': [
'WEBRTC_BSD',
'WEBRTC_THREAD_RR',
],
}],
['OS=="dragonfly" or OS=="netbsd"', {
'defines': [
# doesn't support pthread_condattr_setclock
'WEBRTC_CLOCK_TYPE_REALTIME',
],
}],

> Does gyp even care about which kind of Unix you are?

Only in fringe cases. 'OS' variable is an example where things get ugly.

> The special handling of Mac in this code I think is because gyp needs
> to understand about Mac-specific stuff like .dylib, but as far as gyp
> goes I think FreeBSD/sorlaris/etc. should all be treated identically
> to Linux.

What to do with flock/sun_tool.py then?

def GenerateOutput(target_list, target_dicts, data, params):
options = params['options']
flavor = gyp.common.GetFlavor(params)
...
elif flavor == 'solaris':
header_params.update({
'flock': './gyp-sun-tool flock',
'flock_index': 2,
'extra_commands': SHARED_HEADER_SUN_COMMANDS,
})
elif flavor == 'freebsd':
header_params.update({
'flock': 'lockf',
})

Anyway, here's v2 for better flavoring. I've tried to reduce
proliferation of startswith() without changing fallback.

# - add 'bsd' flavor but keep old value for 'OS' to avoid churn
# - use platform.system(), it's more consistent than sys.platform
# - update tests

Index: pylib/gyp/common.py
===================================================================
--- pylib/gyp/common.py (revision 1533)
+++ pylib/gyp/common.py (working copy)

@@ -10,6 +10,7 @@ import os.path
import re
import tempfile
import sys
+import platform


# A minimal memoizing decorator. It'll blow up if the args aren't immutable,
@@ -364,20 +365,24 @@ def WriteOnDiff(filename):

def GetFlavor(params):
"""Returns |params.flavor| if it's set, the system's default flavor else."""

+ system = platform.system().lower()
flavors = {
- 'cygwin': 'win',
- 'win32': 'win',
- 'darwin': 'mac',
+ 'microsoft': 'win',
+ 'windows' : 'win',
+ 'darwin' : 'mac',
+ 'sunos' : 'solaris',
+ 'dragonfly': 'bsd',
+ 'freebsd' : 'bsd',
+ 'netbsd' : 'bsd',
+ 'openbsd' : 'bsd',

}

if 'flavor' in params:
return params['flavor']
- if sys.platform in flavors:
- return flavors[sys.platform]
- if sys.platform.startswith('sunos'):
- return 'solaris'
- if sys.platform.startswith('freebsd'):
- return 'freebsd'

+ if system.startswith('cygwin'):
+ return 'win'
+ if system in flavors:
+ return flavors[system]

return 'linux'

Index: pylib/gyp/common_test.py
===================================================================
--- pylib/gyp/common_test.py (revision 1533)
+++ pylib/gyp/common_test.py (working copy)
@@ -9,6 +9,7 @@
import gyp.common
import unittest
import sys
+import platform


class TestTopologicallySorted(unittest.TestCase):
@@ -43,28 +44,27 @@ class TestTopologicallySorted(unittest.TestCase):

class TestGetFlavor(unittest.TestCase):
"""Test that gyp.common.GetFlavor works as intended"""
- original_platform = ''
+ original_system = ''

def setUp(self):
- self.original_platform = sys.platform
+ self.original_system = platform.system

def tearDown(self):
- sys.platform = self.original_platform
+ platform.system = self.original_system

def assertFlavor(self, expected, argument, param):
- sys.platform = argument
+ platform.system = lambda: argument
self.assertEqual(expected, gyp.common.GetFlavor(param))

def test_platform_default(self):
- self.assertFlavor('freebsd', 'freebsd9' , {})
- self.assertFlavor('freebsd', 'freebsd10', {})
- self.assertFlavor('solaris', 'sunos5' , {});
- self.assertFlavor('solaris', 'sunos' , {});
- self.assertFlavor('linux' , 'linux2' , {});
- self.assertFlavor('linux' , 'linux3' , {});
+ self.assertFlavor('win' , 'CYGWIN_NT-6.1-WOW64', {})
+ self.assertFlavor('win' , 'Microsoft' , {})
+ self.assertFlavor('linux' , 'Linux' , {})
+ self.assertFlavor('solaris', 'SunOS' , {})
+ self.assertFlavor('bsd' , 'DragonFly' , {})

def test_param(self):
- self.assertFlavor('foobar', 'linux2' , {'flavor': 'foobar'})
+ self.assertFlavor('foobar', 'Linux' , {'flavor': 'foobar'})


if __name__ == '__main__':
Index: pylib/gyp/generator/make.py
===================================================================
--- pylib/gyp/generator/make.py (revision 1533)
+++ pylib/gyp/generator/make.py (working copy)
@@ -24,6 +24,7 @@
import os
import re
import sys
+import platform
import subprocess
import gyp
import gyp.common
@@ -85,8 +86,9 @@ def CalculateVariables(default_variables, params):
COMPILABLE_EXTENSIONS.update({'.m': 'objc', '.mm' : 'objcxx'})
else:
operating_system = flavor
- if flavor == 'android':
- operating_system = 'linux' # Keep this legacy behavior for now.
+ if flavor in ['android', 'bsd']:
+ # Keep this legacy behavior for now.
+ operating_system = platform.system().lower()
default_variables.setdefault('OS', operating_system)
default_variables.setdefault('SHARED_LIB_SUFFIX', '.so')
default_variables.setdefault('SHARED_LIB_DIR','$(builddir)/lib.$(TOOLSET)')
@@ -2000,7 +2002,7 @@ def GenerateOutput(target_list, target_dicts, data
'flock_index': 2,
'extra_commands': SHARED_HEADER_SUN_COMMANDS,
})
- elif flavor == 'freebsd':
+ elif flavor == 'bsd':
header_params.update({
'flock': 'lockf',
})
Index: pylib/gyp/generator/ninja.py
===================================================================
--- pylib/gyp/generator/ninja.py (revision 1533)
+++ pylib/gyp/generator/ninja.py (working copy)
@@ -10,6 +10,7 @@ import re
import signal
import subprocess
import sys
+import platform
import gyp
import gyp.common
import gyp.msvs_emulation
@@ -1295,8 +1296,9 @@ def CalculateVariables(default_variables, params):
default_variables['MSVS_OS_BITS'] = 32
else:
operating_system = flavor
- if flavor == 'android':
- operating_system = 'linux' # Keep this legacy behavior for now.
+ if flavor in ['android', 'bsd']:
+ # Keep this legacy behavior for now.
+ operating_system = platform.system().lower()
default_variables.setdefault('OS', operating_system)
default_variables.setdefault('SHARED_LIB_SUFFIX', '.so')
default_variables.setdefault('SHARED_LIB_DIR',