1250297 - Make python configure output config.status on its own

Status: RESOLVED FIXED

(Firefox Build System :: General, defect)

Description

[Mike Hommey [:glandium]]

We want the old-configure.sh to stop creating config.status itself, but instead get back data to the python configure script that would then output config.status after it adds its own data.

Comment 1

[Mike Hommey [:glandium]]

Attachment: MozReview Request: Bug 1250297 - Make python configure output config.status instead of old-configure doing it. r=gps

The nice side effect is that now we can have actual dicts for defines
and substs from the start, which simplifies so things, although it
requires adjustments to some unit tests.

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

Comment 2

[Mike Hommey [:glandium]]

Kimura-san, would you mind checking this patch doesn't break building with Japanese MSVC (wrt bug 587372)?

Comment 3

[Masatoshi Kimura [:emk]]

mach build failed with the following error:

$ mach build
Error running mach:

    ['build']

The error occurred in code that was called by the mach command. This is either
a bug in the called code itself or in the way that mach is calling it.

You should consider filing a bug for this issue.

If filing a bug, please include the full output of mach, including this error
message.

The details of the failure are as follows:

AttributeError: 'list' object has no attribute 'iteritems'

  File "f:\m\mozilla-central\python/mozbuild/mozbuild/mach_commands.py", line 412, in build
    if self.substs.get('MOZ_ARTIFACT_BUILDS', False):
  File "f:\m\mozilla-central\python/mozbuild\mozbuild\base.py", line 273, in substs
    return self.config_environment.substs
  File "f:\m\mozilla-central\python/mozbuild\mozbuild\base.py", line 263, in config_environment
    ConfigEnvironment.from_config_status(config_status)
  File "f:\m\mozilla-central\python/mozbuild\mozbuild\backend\configenvironment.py", line 191, in from_config_status
    config.defines, config.non_global_defines, config.substs, path)
  File "f:\m\mozilla-central\python/mozbuild\mozbuild\backend\configenvironment.py", line 131, in __init__
    global_defines = [name for name, value in defines.iteritems()

Comment 4

[Gregory Szorc [:gps]]

https://reviewboard.mozilla.org/r/36745/#review33571

::: python/mozbuild/mozbuild/backend/configenvironment.py:108
(Diff revision 1)
> -    def __init__(self, topsrcdir, topobjdir, defines=[], non_global_defines=[],
> -        substs=[], source=None):
> +    def __init__(self, topsrcdir, topobjdir, defines={}, non_global_defines=[],
> +        substs={}, source=None):

More default argument value badness.

::: python/mozbuild/mozbuild/config_status.py:66
(Diff revision 1)
> -        defines=[], non_global_defines=[], substs=[], source=None):
> +        defines={}, non_global_defines=[], substs={}, source=None):

Default argument values of [] and {} should be avoided because the first value passed in is used as the default value for all subsequent calls. Set the default to `None` and use e.g. `defines = defines or {}`.

Comment 5

[Mike Hommey [:glandium]]

Comment on attachment 8723826 [details]
MozReview Request: Bug 1250297 - Make python configure output config.status instead of old-configure doing it. r=gps

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

Comment 6

[Mike Hommey [:glandium]]

Comment on attachment 8723826 [details]
MozReview Request: Bug 1250297 - Make python configure output config.status instead of old-configure doing it. r=gps

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/36745/diff/2-3/

Comment 7

[Mike Hommey [:glandium]]

Comment on attachment 8723826 [details]
MozReview Request: Bug 1250297 - Make python configure output config.status instead of old-configure doing it. r=gps

https://reviewboard.mozilla.org/r/36745/#review34049

Sorry, just fooling around with mozreview

Comment 8

[Gregory Szorc [:gps]]

Comment on attachment 8723826 [details]
MozReview Request: Bug 1250297 - Make python configure output config.status instead of old-configure doing it. r=gps

https://reviewboard.mozilla.org/r/36745/#review34237

One of the concerns I have about turning substs into a dict is we lose ordering. And there are variables that reference other variables, so ordering could be important.

That being said, our generated autoconf.mk uses the `=` operator, which is lazy. So order shouldn't matter. Lucky us.

Also, I was diffing the objdir with this patch and surprisingly got the following in config/autoconf*.mk:

-HOST_CXX = /usr/bin/clang++
+HOST_CXX = /usr/local/bin/ccache /usr/bin/clang++

Ditto for HOST_CC. I don't think that's incorrect but it was surprising.

::: configure.py:129
(Diff revision 3)
> +    with open('config.data') as fh:
> +        encoding = 'mbcs' if sys.platform == 'win32' else 'utf-8'
> +        code = compile(fh.read().decode(encoding), 'config.data', 'exec')

You may be interested in `codecs.open()`. It is like `open()` except it opens a file with a specified encoding and all read and write operations automatically do encoding foo. I also believe the file object has Python 3 semantics where it will e.g. disallow unencoded byte strings from being written.

::: configure.py:136
(Diff revision 3)
> +    os.remove('config.data')

Should this be in a finally block or is it important for the file to be preserved for debugging purposes?

::: configure.py:161
(Diff revision 3)
> +            json.dump(v, fh)

I was playing with this patch locally and I think it would be nicer if we passed sort_keys=True and indent=4 to json.dump() so output were deterministic and easier to read. As it is now, all the substs and defines are on single lines.

::: python/mozbuild/mozbuild/backend/configenvironment.py:131
(Diff revision 3)
> -        global_defines = [name for name, value in defines
> +        global_defines = [name for name, value in self.defines.iteritems()

You don't need "value" and "iteritems()" here. Use iterkeys()?

Comment 9

[Mike Hommey [:glandium]]

(In reply to Gregory Szorc [:gps] from comment #8)
> Comment on attachment 8723826 [details]
> MozReview Request: Bug 1250297 - Make python configure output config.status
> instead of old-configure doing it
> 
> https://reviewboard.mozilla.org/r/36745/#review34237
> 
> One of the concerns I have about turning substs into a dict is we lose
> ordering. And there are variables that reference other variables, so
> ordering could be important.

Did you miss the fact that the first thing we did with substs was dict(substs) before this patch? There is no ordering change coming from this patch.

> That being said, our generated autoconf.mk uses the `=` operator, which is
> lazy. So order shouldn't matter. Lucky us.
> 
> Also, I was diffing the objdir with this patch and surprisingly got the
> following in config/autoconf*.mk:
> 
> -HOST_CXX = /usr/bin/clang++
> +HOST_CXX = /usr/local/bin/ccache /usr/bin/clang++
> 
> Ditto for HOST_CC. I don't think that's incorrect but it was surprising.

This is likely not because of this patch, but due to the funkiness of our old-configure.in code.

Comment 10

[Mike Hommey [:glandium]]

Comment on attachment 8723826 [details]
MozReview Request: Bug 1250297 - Make python configure output config.status instead of old-configure doing it. r=gps

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/36745/diff/3-4/

Comment 11

[Mike Hommey [:glandium]]

Kimura-san, can you give the last version of the patch a go?

Comment 12

[Mike Hommey [:glandium]]

Comment on attachment 8723826 [details]
MozReview Request: Bug 1250297 - Make python configure output config.status instead of old-configure doing it. r=gps

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/36745/diff/4-5/

Comment 13

[Gregory Szorc [:gps]]

Comment on attachment 8723826 [details]
MozReview Request: Bug 1250297 - Make python configure output config.status instead of old-configure doing it. r=gps

https://reviewboard.mozilla.org/r/36745/#review34257

LGTM.

Comment 14

[Masatoshi Kimura [:emk]]

(In reply to Mike Hommey [:glandium] from comment #11)
> Kimura-san, can you give the last version of the patch a go?

The console spits out tons of "メモ: インクルード ファイル:" messages. In other words, this patch breaks CL_INCLUDES_PREFIX.

Comment 15

[Masatoshi Kimura [:emk]]

config.status has the following line if the patch is applied.
    "CL_INCLUDES_PREFIX": "\u30e1\u30e2: \u30a4\u30f3\u30af\u30eb\u30fc\u30c9 \u30d5\u30a1\u30a4\u30eb:  ", 
without the patch, this is:
    (''' CL_INCLUDES_PREFIX ''', r''' メモ: インクルード ファイル:   '''),
(in Shift_JIS)

Comment 16

[Masatoshi Kimura [:emk]]

Attachment: 1250297_fix_cl_include_prefix_encoding

FYI this patch fixed the issue for me.

Comment 17

[Gregory Szorc [:gps]]

Comment on attachment 8726226 [details] [diff] [review]
1250297_fix_cl_include_prefix_encoding

Review of attachment 8726226 [details] [diff] [review]:
-----------------------------------------------------------------

::: python/mozbuild/mozbuild/backend/configenvironment.py
@@ +134,5 @@
>              shell_quote(self.defines[name]).replace('$', '$$'))
>              for name in sorted(global_defines)])
>          def serialize(obj):
>              if isinstance(obj, StringTypes):
> +                return obj.decode('utf-8').encode(sys.getfilesystemencoding())

This scares me. Any time you rely on the current encoding, it is a good sign that somewhere you converted to Unicode when you should have treated things as bytes.

I'll take a fresh look at glandium's patch with an eye towards encoding foo.

Comment 18

[Gregory Szorc [:gps]]

Comment on attachment 8723826 [details]
MozReview Request: Bug 1250297 - Make python configure output config.status instead of old-configure doing it. r=gps

https://reviewboard.mozilla.org/r/36745/#review34513

::: configure.py:159
(Diff revision 5)
> +    with open('config.status', 'w') as fh:
> +        fh.write('#!%s\n' % config['substs']['PYTHON'])
> +        fh.write('# coding=utf-8\n')
> +        for k, v in config.iteritems():
> +            fh.write('%s = ' % k)
> +            json.dump(v, fh, sort_keys=True, indent=4)
> +            fh.write('\n')

First note that the `# coding=utf-8` directive only states how Python will interpret byte sequences in the source file. If you have a string literal in the file, the type of that string will still be `str` (unless `unicode_literals` is used, then it will be `unicode`). Python will interpret the literal as the encoding specified and then re-encode it to bytes for the value of the `str` type. Come to think of it, us using utf-8 here feels weird. I guess we've been lucky all values have been valid utf-8!

>>> json.dumps(u'こんにちは世界')
'"\\u3053\\u3093\\u306b\\u3061\\u306f\\u4e16\\u754c"'

>>> json.dumps(u'こんにちは世界', ensure_ascii=False)
u'"\u3053\u3093\u306b\u3061\u306f\u4e16\u754c"'

Note the difference between the str and unicode output types.

Also note that this file has unicode_literals.

And, config.data above is being opened with an encoding, so items of `config` will be the `unicode` type.

On one hand, I like using `unicode` everywhere because it makes Python 3 compatibility easier. On the other, some values - notably CL_INCLUDES_PREFIX really need to be treated as a bytes sequence because we really can't infer what the encoding is.

My recollection is the last time we dealt with encoding in config.status is that we went out of our way to treat everything as byte sequences in the file. We had code that attempted Unicode decoding of SUBSTS so we could use `unicode` types in moz.build files. That's why we have config.substs and config.substs_unicode.

This patch kinda/sorta undoes that. However, we had `# coding=utf-8` in config.status before, which meant we were performing a UTF-8 loop as part of source reading. What we weren't doing was the extra JSON encoding, which does its own normalization of data.

When it comes to Python, JSON, and Unicode or arbitrary byte sequences, I've had nothing but pain. If you care about preserving bytes, I'd avoid the json module. I'd consider `pickle` instead. I'd also consider storing the data in a standalone file so you don't have to worry about encoding when you read a Python source file.

Comment 19

[Mike Hommey [:glandium]]

Note the coding line for the old config.status was mbcs on windows, not utf-8. That participates on making things more complicated.

Comment 20

[Mike Hommey [:glandium]]

Comment on attachment 8723826 [details]
MozReview Request: Bug 1250297 - Make python configure output config.status instead of old-configure doing it. r=gps

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/36745/diff/5-6/

Comment 21

[Gregory Szorc [:gps]]

Comment on attachment 8723826 [details]
MozReview Request: Bug 1250297 - Make python configure output config.status instead of old-configure doing it. r=gps

https://reviewboard.mozilla.org/r/36745/#review34557

For the record, we should be using the latin1 encoding rather than utf-8 or mbcs, as latin1 is byte preserving in Python:

>>> b''.join(chr(x) for x in range(255)).decode('latin1').encode('latin1') == b''.join(chr(x) for x in range(255))
True

But this patch preserves what we do with config.status today (using utf-8 or mbcs). If it ain't broke don't fix it. LGTM.

Comment 22

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/49f9aa2b50f0

Comment 23

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/49f9aa2b50f0