Closed Bug 1258618 Opened 8 years ago Closed 8 years ago

Allow to use bools with set_config/set_define/add_old_configure_assignment

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

(5 files)

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.
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)
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/
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/
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/
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/
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+
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)
Product: Core → Firefox Build System
You need to log in before you can comment on or make changes to this bug.