If you think a bug might affect users in the 57 release, please set the correct tracking and status flags for Release Management.

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

RESOLVED FIXED in Firefox 48

Status

()

Core
Build Config
RESOLVED FIXED
2 years ago
a year ago

People

(Reporter: glandium, Assigned: glandium)

Tracking

(Blocks: 1 bug)

unspecified
mozilla48
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox48 fixed)

Details

MozReview Requests

()

Submitter Diff Changes Open Issues Last Updated
Loading...
Error loading review requests:

Attachments

(6 attachments)

Comment hidden (empty)
(Assignee)

Comment 1

2 years ago
Created attachment 8741218 [details]
MozReview Request: Bug 1264482 - Fake an arbitrary __name__ in sandboxed global. r?ted

Review commit: https://reviewboard.mozilla.org/r/46297/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/46297/
Attachment #8741218 - Flags: review?(ted)
Attachment #8741219 - Flags: review?(ted)
Attachment #8741220 - Flags: review?(ted)
Attachment #8741221 - Flags: review?(ted)
Attachment #8741222 - Flags: review?(ted)
Attachment #8741223 - Flags: review?(ted)
(Assignee)

Comment 2

2 years ago
Created attachment 8741219 [details]
MozReview Request: Bug 1264482 - Add a string type with a limited set of possible values. r?ted

Review commit: https://reviewboard.mozilla.org/r/46299/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/46299/
(Assignee)

Comment 3

2 years ago
Created attachment 8741220 [details]
MozReview Request: Bug 1264482 - Use the limited string type for the compiler type. r?ted

Review commit: https://reviewboard.mozilla.org/r/46301/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/46301/
(Assignee)

Comment 4

2 years ago
Created attachment 8741221 [details]
MozReview Request: Bug 1264482 - Use the limited string type for the different values we get out of split_triplet. r?ted

Review commit: https://reviewboard.mozilla.org/r/46303/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/46303/
(Assignee)

Comment 5

2 years ago
Created attachment 8741222 [details]
MozReview Request: Bug 1264482 - Move compiler invocation for preprocessing to a separate function. r?ted

Review commit: https://reviewboard.mozilla.org/r/46305/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/46305/
(Assignee)

Comment 6

2 years ago
Created attachment 8741223 [details]
MozReview Request: Bug 1264482 - Move adding -std=gnu99 and -std=gnu++11 to moz.configure. r?ted

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+
(Assignee)

Comment 13

a year ago
(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.
(Assignee)

Updated

a year ago
Blocks: 1265627

Comment 15

a year ago
https://hg.mozilla.org/integration/mozilla-inbound/rev/f464affb2948
https://hg.mozilla.org/integration/mozilla-inbound/rev/bb77bbe1fbd7
https://hg.mozilla.org/integration/mozilla-inbound/rev/177380345f2f
https://hg.mozilla.org/integration/mozilla-inbound/rev/4e3809b7b919
https://hg.mozilla.org/integration/mozilla-inbound/rev/3abd062e590c
https://hg.mozilla.org/integration/mozilla-inbound/rev/a1a03c58109d

Comment 16

a year ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/f464affb2948
https://hg.mozilla.org/mozilla-central/rev/bb77bbe1fbd7
https://hg.mozilla.org/mozilla-central/rev/177380345f2f
https://hg.mozilla.org/mozilla-central/rev/4e3809b7b919
https://hg.mozilla.org/mozilla-central/rev/3abd062e590c
https://hg.mozilla.org/mozilla-central/rev/a1a03c58109d
Status: NEW → RESOLVED
Last Resolved: a year ago
status-firefox48: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla48
You need to log in before you can comment on or make changes to this bug.