In order to have something more generic to get the platform name from different mach commands that are implemented by deriving MozbuildObject, the patch add property platformname.

Comment on attachment 8885244 [details] Bug 1379961 - Add platform and architecture name to MozbuildObject. https://reviewboard.mozilla.org/r/156110/#review161806 I like the spirit of this patch. But let's not attempt to add architecture to it. Or if we do, make the function return a tuple with the architecture as a separate component. And don't use `platform.architecture()` because it returns the architecture of a specific binary. By default, it uses Python itself. It is not uncommon to have a 32-bit Python executable on a 64-bit operating system.

(In reply to Gregory Szorc [:gps] from comment #2) > Comment on attachment 8885244 [details] > Bug 1379961 - Add platform name to MozbuildObject. > > https://reviewboard.mozilla.org/r/156110/#review161806 > > I like the spirit of this patch. But let's not attempt to add architecture > to it. Or if we do, make the function return a tuple with the architecture > as a separate component. And don't use `platform.architecture()` because it > returns the architecture of a specific binary. By default, it uses Python > itself. It is not uncommon to have a 32-bit Python executable on a 64-bit > operating system. Thanks for pointing this out to me. Should the tuple consist or os architecture + python architecture?

OS architecture. Nobody really cares what Python's architecture is.

Comment on attachment 8887059 [details] [diff] [review] 369118.patch Review of attachment 8887059 [details] [diff] [review]: ----------------------------------------------------------------- This is on the right track. ::: python/mozbuild/mozbuild/base.py @@ +269,5 @@ > + @property > + def platform(self): > + """Returns current platform and architecture name""" > + platformName = None > + architectureName = None Please follow the naming convention in this file. Use platform_name instead of platformName for variables. Only classes/types get CamelCase. @@ +279,5 @@ > + elif sys.platform.startswith('win'): > + if machine == 'AMD64': > + platformName = "win64" > + architectureName = '64bit' > + else: Strictly speaking, there are other machine types that are 64-bit. Perhaps change this to `if machine.endswith('64')`? @@ +284,5 @@ > + platformName = "win32" > + architectureName = '32bit' > + elif sys.platform == 'darwin': > + platformName = 'macosx64' > + architectureName = '64bit' Strictly speaking, Darwin does not imply MacOS nor 64-bit. For all intents and purposes, I'm fine with Darwin implying MacOS. But let's make the architecture check actually dependent on the actual architecture. @@ +286,5 @@ > + elif sys.platform == 'darwin': > + platformName = 'macosx64' > + architectureName = '64bit' > + > + return (platformName, architectureName) Nit: don't need parenthesis here.

Comment on attachment 8885244 [details] Bug 1379961 - Add platform and architecture name to MozbuildObject. https://reviewboard.mozilla.org/r/156110/#review171898 ::: python/mozbuild/mozbuild/base.py:275 (Diff revision 2) > + if sys.platform == 'linux2': > + if machine.endswith('64'): > + platform_name = "linux64" > + architecture_name = '64bit' This is missing a 32 bit block. ::: python/mozbuild/mozbuild/base.py:281 (Diff revision 2) > + platform_name = "win64" > + architecture_name = '64bit' The "64" in both the platform and architecture name seems a bit redundant. Can we eliminate that?

This seems very similar to what mozinfo provides: http://gecko.readthedocs.io/en/latest/build/buildsystem/mozinfo.html

Is there a chicken-and-egg problem with using mozinfo? We want this as a generic Python API available to mach commands.

(In reply to Gregory Szorc [:gps] from comment #10) > Is there a chicken-and-egg problem with using mozinfo? We want this as a > generic Python API available to mach commands. I'm not sure if we can use mozinfo in our context, specially that we want to have the correct naming to direct pass it to artifact toolchain.

(In reply to Gregory Szorc [:gps] from comment #8) > Comment on attachment 8885244 [details] > Bug 1379961 - Add platform and architecture name to MozbuildObject. > > https://reviewboard.mozilla.org/r/156110/#review171898 > > ::: python/mozbuild/mozbuild/base.py:275 > (Diff revision 2) > > + if sys.platform == 'linux2': > > + if machine.endswith('64'): > > + platform_name = "linux64" > > + architecture_name = '64bit' > > This is missing a 32 bit block. Why do we need the 32 bit block since we don't support this architecture? I was thinking of returning None in case archiecture is not supported. > > ::: python/mozbuild/mozbuild/base.py:281 > (Diff revision 2) > > + platform_name = "win64" > > + architecture_name = '64bit' > > The "64" in both the platform and architecture name seems a bit redundant. > Can we eliminate that? I'll update the patch and post it for your review.

mozinfo does provide defaults for most of its properties. We override those with info from the build system when you run configure. If all you're trying to provide is the platform + architecture on which `mach build` is running, then I'd suggest reusing mozinfo for simplicity + consistency.

(In reply to Ted Mielczarek [:ted.mielczarek] from comment #14) > mozinfo does provide defaults for most of its properties. We override those > with info from the build system when you run configure. If all you're trying > to provide is the platform + architecture on which `mach build` is running, > then I'd suggest reusing mozinfo for simplicity + consistency. thanks for the info, in the latest patch i've used mozinfo since, as you said, it has everything that i need.

Comment on attachment 8885244 [details] Bug 1379961 - Add platform and architecture name to MozbuildObject. https://reviewboard.mozilla.org/r/156110/#review183970 This is mostly good. Just need the minor import fixup. Then this can land. ::: python/mozbuild/mozbuild/base.py:9 (Diff revision 3) > > from __future__ import absolute_import, print_function, unicode_literals > > import json > import logging > +import mozinfo Please defer this import to the added method. We do this for performance reasons because this file is imported in a bunch of places and we try (but often fail) to keep the import list down.

Comment on attachment 8885244 [details] Bug 1379961 - Add platform and architecture name to MozbuildObject. https://reviewboard.mozilla.org/r/156110/#review184078 W00t. Thank you for your persistence. This should land shortly.

Pushed by gszorc@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/be5cbdf6f420 Add platform and architecture name to MozbuildObject. r=gps