Closed Bug 1264482 Opened 8 years ago Closed 8 years ago

Move adding -std=gnu99 and -std=gnu++11 to moz.configure

Categories

(Firefox Build System :: General, defect)

defect
Not set
normal

Tracking

(firefox48 fixed)

RESOLVED FIXED
mozilla48
Tracking Status
firefox48 --- fixed

People

(Reporter: glandium, Assigned: glandium)

References

(Blocks 1 open bug)

Details

Attachments

(6 files)

      No description provided.
We were unconditionally adding them, now actually check what the
compilers default to and add the flags if they are necessary.
This will, in the future, allow finer grained policy changes, where
we can decide that C++11 and C++14 are fine, downgrading compilers
that do C++17, etc.

Review commit: https://reviewboard.mozilla.org/r/46307/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/46307/
Comment on attachment 8741218 [details]
MozReview Request: Bug 1264482 - Fake an arbitrary __name__ in sandboxed global. r?ted

https://reviewboard.mozilla.org/r/46297/#review43871
Attachment #8741218 - Flags: review?(ted) → review+
Attachment #8741219 - Flags: review?(ted) → review+
Comment on attachment 8741219 [details]
MozReview Request: Bug 1264482 - Add a string type with a limited set of possible values. r?ted

https://reviewboard.mozilla.org/r/46299/#review43873

::: python/mozbuild/mozbuild/test/test_util.py:869
(Diff revision 1)
>          )
>  
> +class TestLimitedString(unittest.TestCase):
> +    def test_string(self):
> +        class CompilerType(LimitedString):
> +            POSSIBLE_VALUES = ('msvc', 'gcc', 'clang', 'clang-cl')

I wonder if this wouldn't look nicer if you had a helper function that returned the type, so you could write like:
```
CompilerType = make_limited_string('msvc', 'gcc', 'clang', 'clang-cl')
```

::: python/mozbuild/mozbuild/util.py:1130
(Diff revision 1)
> +
> +class LimitedComparisonError(Exception):
> +    pass
> +
> +
> +class LimitedString(unicode):

Maybe this would be more usefully named `EnumString` or something like that?
Comment on attachment 8741220 [details]
MozReview Request: Bug 1264482 - Use the limited string type for the compiler type. r?ted

https://reviewboard.mozilla.org/r/46301/#review43877
Attachment #8741220 - Flags: review?(ted) → review+
Comment on attachment 8741221 [details]
MozReview Request: Bug 1264482 - Use the limited string type for the different values we get out of split_triplet. r?ted

https://reviewboard.mozilla.org/r/46303/#review43879

Did you consider going all-in and making these actual enums instead of enums-of-strings? I'm not entirely sure what that would look like, and I suspect we'd have to stringify at some point to work with the current build backend, but that feels like the logical endgame. We've got an implementation of Python enums in the WebIDL parser:
https://dxr.mozilla.org/mozilla-central/rev/1f16d3da9280e40ada252acf8110b91ee1edbb08/dom/bindings/parser/WebIDL.py#42
and using them looks like:
https://dxr.mozilla.org/mozilla-central/rev/1f16d3da9280e40ada252acf8110b91ee1edbb08/dom/bindings/parser/WebIDL.py#2885

::: build/moz.configure/init.configure:381
(Diff revision 1)
> +    class Kernel(LimitedString):
> +        POSSIBLE_VALUES = ('Linux', 'kFreeBSD', 'WINNT', 'Darwin',
> +                           'DragonFly', 'FreeBSD', 'NetBSD', 'OpenBSD')
> +
> +    class CPU(LimitedString):
> +        POSSIBLE_VALUES = ('x86', 'x86_64', 'ia64', 's390', 's390x', 'ppc64',

You should format all of these lists-of-values so that they're one entry to a line, for ease of future editing.

::: build/moz.configure/init.configure:385
(Diff revision 1)
> +    class CPU(LimitedString):
> +        POSSIBLE_VALUES = ('x86', 'x86_64', 'ia64', 's390', 's390x', 'ppc64',
> +                           'ppc', 'Alpha', 'hppa', 'sparc64', 'sparc', 'arm',
> +                           'mips32', 'mips64', 'aarch64')
> +
> +    class Endianness(LimitedString):

I think it'd be nice to define these in a separate file, so that we'd have a simple place to locate them, but I'm not 100% sure what that would look like. Having them treated as effectively enums is great, but it'd be even better if the set of known values was simple to find.
Attachment #8741221 - Flags: review?(ted) → review+
Comment on attachment 8741222 [details]
MozReview Request: Bug 1264482 - Move compiler invocation for preprocessing to a separate function. r?ted

https://reviewboard.mozilla.org/r/46305/#review43889
Attachment #8741222 - Flags: review?(ted) → review+
Comment on attachment 8741223 [details]
MozReview Request: Bug 1264482 - Move adding -std=gnu99 and -std=gnu++11 to moz.configure. r?ted

https://reviewboard.mozilla.org/r/46307/#review43891

::: build/moz.configure/toolchain.configure:244
(Diff revision 1)
> +        return
> +
> +    data = {}
> -        for line in result.splitlines():
> +    for line in result.splitlines():
> -            if line.startswith('COMPILER '):
> -                _, type, version = line.split(None, 2)
> +        if line.startswith('%') and ' ' in line:
> +            k, v = line.split(' ', 1)

You could use `line.partition(' ')` and  save yourself the `' ' in line` check.
Attachment #8741223 - Flags: review?(ted) → review+
(In reply to Ted Mielczarek [:ted.mielczarek] from comment #10)
> Comment on attachment 8741221 [details]
> MozReview Request: Bug 1264482 - Use the limited string type for the
> different values we get out of split_triplet. r?ted
> 
> https://reviewboard.mozilla.org/r/46303/#review43879
> 
> Did you consider going all-in and making these actual enums instead of
> enums-of-strings?

I did consider them, but that's more verbose, since you need to use EnumName.value.
I find using strings more convenient for sandboxed code.

> ::: build/moz.configure/init.configure:385
> (Diff revision 1)
> > +    class CPU(LimitedString):
> > +        POSSIBLE_VALUES = ('x86', 'x86_64', 'ia64', 's390', 's390x', 'ppc64',
> > +                           'ppc', 'Alpha', 'hppa', 'sparc64', 'sparc', 'arm',
> > +                           'mips32', 'mips64', 'aarch64')
> > +
> > +    class Endianness(LimitedString):
> 
> I think it'd be nice to define these in a separate file, so that we'd have a
> simple place to locate them, but I'm not 100% sure what that would look
> like. Having them treated as effectively enums is great, but it'd be even
> better if the set of known values was simple to find.

something like mozbuild.configure.constants?
(In reply to Mike Hommey [:glandium] from comment #13)
> I did consider them, but that's more verbose, since you need to use
> EnumName.value.
> I find using strings more convenient for sandboxed code.

OK, that's fair. At least with this method we still get an enumerated list of allowed values, which is most of the benefit.

> something like mozbuild.configure.constants?

I'm not hung up on the name, I just think a special file that contains only these enumerated lists would be a good idea.
Blocks: 1265627
Product: Core → Firefox Build System
You need to log in before you can comment on or make changes to this bug.