1258618 - Allow to use bools with set_config/set_define/add_old_configure_assignment

Status: RESOLVED FIXED

(Firefox Build System :: General, defect)

Description

[Mike Hommey [:glandium]]

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.

Comment 1

[Mike Hommey [:glandium]]

Attachment: MozReview Request: Bug 1258618 - Serialize substs/configs and defines bools as '1' or '' in config.status

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/

Comment 2

[Mike Hommey [:glandium]]

Attachment: MozReview Request: Bug 1258618 - Allow to use bools as values given to add_old_configure_assignment

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

Comment 3

[Mike Hommey [:glandium]]

Attachment: MozReview Request: Bug 1258618 - Use True instead of '1' for set_config

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

Comment 4

[Mike Hommey [:glandium]]

Attachment: MozReview Request: Bug 1258618 - Use True instead of '1' for set_define

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

Comment 5

[Mike Hommey [:glandium]]

Attachment: MozReview Request: Bug 1258618 - Use True instead of '1' for add_old_configure_assignment

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

Comment 6

[Mike Hommey [:glandium]]

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 7

[Mike Hommey [:glandium]]

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 8

[Mike Hommey [:glandium]]

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 9

[Mike Hommey [:glandium]]

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 10

[Mike Hommey [:glandium]]

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 11

[Nick Alexander :nalexander [he/him]]

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

Comment 12

[Nick Alexander :nalexander [he/him]]

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

Comment 13

[Nick Alexander :nalexander [he/him]]

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

https://reviewboard.mozilla.org/r/41663/#review38281

Comment 14

[Nick Alexander :nalexander [he/him]]

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

https://reviewboard.mozilla.org/r/41665/#review38283

Comment 15

[Nick Alexander :nalexander [he/him]]

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

Comment 16

[Nick Alexander :nalexander [he/him]]

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

Comment 17

[Nick Alexander :nalexander [he/him]]

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

Comment 18

[Mike Hommey [:glandium]]

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.

Comment 19

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/aa4bcd9eadcb
https://hg.mozilla.org/integration/mozilla-inbound/rev/fa9a23bf9e17
https://hg.mozilla.org/integration/mozilla-inbound/rev/0f1ddce63248
https://hg.mozilla.org/integration/mozilla-inbound/rev/34d422497ef9
https://hg.mozilla.org/integration/mozilla-inbound/rev/43d66eea833b

Comment 20

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/aa4bcd9eadcb
https://hg.mozilla.org/mozilla-central/rev/fa9a23bf9e17
https://hg.mozilla.org/mozilla-central/rev/0f1ddce63248
https://hg.mozilla.org/mozilla-central/rev/34d422497ef9
https://hg.mozilla.org/mozilla-central/rev/43d66eea833b