1379961 - Add platform name to MozbuildObject

Status: RESOLVED FIXED

(Firefox Build System :: General, enhancement)

Description

[Andi [:andi]]

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 1

[Andi [:andi]]

Attachment: Bug 1379961 - Add platform and architecture name to MozbuildObject.

Review commit: https://reviewboard.mozilla.org/r/156110/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/156110/

Comment 2

[Gregory Szorc [:gps]]

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.

Comment 3

[Andi [:andi]]

(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?

Comment 4

[Gregory Szorc [:gps]]

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

Comment 5

[Andi [:andi]]

Attachment: 369118.patch

Comment 6

[Gregory Szorc [:gps]]

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 7

[Andi [:andi]]

Comment on attachment 8885244 [details]
Bug 1379961 - Add platform and architecture name to MozbuildObject.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/156110/diff/1-2/

Comment 8

[Gregory Szorc [:gps]]

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?

Comment 9

[(not currently active) Ted Mielczarek]

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

Comment 10

[Gregory Szorc [:gps]]

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

Comment 11

[Andi [:andi]]

(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.

Comment 12

[Andi [:andi]]

(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.

Comment 13

[Andi [:andi]]

Comment on attachment 8885244 [details]
Bug 1379961 - Add platform and architecture name to MozbuildObject.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/156110/diff/2-3/

Comment 14

[(not currently active) Ted Mielczarek]

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.

Comment 15

[Andi [:andi]]

(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 16

[Gregory Szorc [:gps]]

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 17

[Andi [:andi]]

Comment on attachment 8885244 [details]
Bug 1379961 - Add platform and architecture name to MozbuildObject.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/156110/diff/3-4/

Comment 18

[Gregory Szorc [:gps]]

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.

Comment 19

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/be5cbdf6f420
Add platform and architecture name to MozbuildObject. r=gps

Comment 20

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

https://hg.mozilla.org/mozilla-central/rev/be5cbdf6f420