Allow to use bools with set_config/set_define/add_old_configure_assignment

RESOLVED FIXED in Firefox 48

Status

defect
RESOLVED FIXED
3 years ago
Last year

People

(Reporter: glandium, Assigned: glandium)

Tracking

(Blocks 1 bug)

unspecified
mozilla48
Dependency tree / graph

Firefox Tracking Flags

(firefox48 fixed)

Details

Attachments

(5 attachments)

Assignee

Description

3 years ago
bug 1254884 allows boolean values to be dumped in config.data and set in config.status, but it's further down the road that there still are road blocks.
Assignee

Comment 1

3 years ago
This allows to use True and False as values given to
set_config/set_define in moz.configure files, while postponing having to
deal with the long tail of things depending on the values from substs
and defines.

Ideally, everything would handle the bools just fine, but there are too
many things involved to deal with this right now: scripts using
buildconfig.{substs,defines}, scripts using ConfigEnvironment (e.g.
process_define_files.py), ConfigEnvironment itself, etc.

Review commit: https://reviewboard.mozilla.org/r/41659/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/41659/
Attachment #8733231 - Flags: review?(gps)
Assignee

Comment 6

3 years ago
Comment on attachment 8733231 [details]
MozReview Request: Bug 1258618 - Serialize substs/configs and defines bools as '1' or '' in config.status

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

Comment 7

3 years ago
Comment on attachment 8733232 [details]
MozReview Request: Bug 1258618 - Allow to use bools as values given to add_old_configure_assignment

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

Comment 8

3 years ago
Comment on attachment 8733233 [details]
MozReview Request: Bug 1258618 - Use True instead of '1' for set_config

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

Comment 9

3 years ago
Comment on attachment 8733234 [details]
MozReview Request: Bug 1258618 - Use True instead of '1' for set_define

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

Comment 10

3 years ago
Comment on attachment 8733235 [details]
MozReview Request: Bug 1258618 - Use True instead of '1' for add_old_configure_assignment

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/41667/diff/1-2/
Comment on attachment 8733231 [details]
MozReview Request: Bug 1258618 - Serialize substs/configs and defines bools as '1' or '' in config.status

https://reviewboard.mozilla.org/r/41659/#review38277

nits.

::: configure.py:35
(Diff revision 2)
>  def config_status(config):
>      # Sanitize config data to feed config.status
> +    # Ideally, all the backend and frontend code would handle the booleans, but
> +    # there are so many things involved, that it's easier to keep config.status
> +    # untouched for now.
> +    def sanitized_bool(v):

This is a little misleading, since the input doesn't have to be a `bool`.  Perhaps `sanitize_bools`?  I guess this doesn't escape this scope, so not a big deal.

::: configure.py:40
(Diff revision 2)
> +    def sanitized_bool(v):
> +        if v is True:
> +            return '1'
> +        if v is False:
> +            return ''
> +        return v

Is this a chance to assert something else about our data?  Is it always a string?  Always '1' or ''?
Attachment #8733231 - Flags: review+
Comment on attachment 8733233 [details]
MozReview Request: Bug 1258618 - Use True instead of '1' for set_config

https://reviewboard.mozilla.org/r/41663/#review38281
Attachment #8733233 - Flags: review+
Comment on attachment 8733234 [details]
MozReview Request: Bug 1258618 - Use True instead of '1' for set_define

https://reviewboard.mozilla.org/r/41665/#review38283
Attachment #8733234 - Flags: review+
Comment on attachment 8733235 [details]
MozReview Request: Bug 1258618 - Use True instead of '1' for add_old_configure_assignment

https://reviewboard.mozilla.org/r/41667/#review38285
Attachment #8733235 - Flags: review+
Comment on attachment 8733232 [details]
MozReview Request: Bug 1258618 - Allow to use bools as values given to add_old_configure_assignment

https://reviewboard.mozilla.org/r/41661/#review38287
Attachment #8733232 - Flags: review+
Assignee

Comment 18

3 years ago
https://reviewboard.mozilla.org/r/41659/#review38277

> Is this a chance to assert something else about our data?  Is it always a string?  Always '1' or ''?

Mmmmm I'd rather do that in a followup in set_config itself, that would give a better traceback pointing to where the unwanted typed value is being set.
Attachment #8733231 - Flags: review?(gps)
Attachment #8733232 - Flags: review?(gps)
Attachment #8733233 - Flags: review?(gps)
Attachment #8733234 - Flags: review?(gps)
Attachment #8733235 - Flags: review?(gps)

Updated

Last year
Product: Core → Firefox Build System
You need to log in before you can comment on or make changes to this bug.