Closed
Bug 1250297
Opened 9 years ago
Closed 9 years ago
Make python configure output config.status on its own
Categories
(Firefox Build System :: General, defect)
Firefox Build System
General
Tracking
(firefox48 fixed)
RESOLVED
FIXED
mozilla48
Tracking | Status | |
---|---|---|
firefox48 | --- | fixed |
People
(Reporter: glandium, Assigned: glandium)
References
(Blocks 1 open bug)
Details
Attachments
(2 files)
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.
Assignee | ||
Updated•9 years ago
|
Assignee: nobody → mh+mozilla
Assignee | ||
Comment 1•9 years ago
|
||
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/
Attachment #8723826 -
Flags: review?(ted)
Assignee | ||
Comment 2•9 years ago
|
||
Kimura-san, would you mind checking this patch doesn't break building with Japanese MSVC (wrt bug 587372)?
Flags: needinfo?(VYV03354)
Comment 3•9 years ago
|
||
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()
Flags: needinfo?(VYV03354)
Comment 4•9 years ago
|
||
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 {}`.
Assignee | ||
Comment 5•9 years ago
|
||
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/
Assignee | ||
Comment 6•9 years ago
|
||
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/
Attachment #8723826 -
Flags: review?(ted) → review?(gps)
Assignee | ||
Updated•9 years ago
|
Attachment #8723826 -
Flags: review+
Assignee | ||
Comment 7•9 years ago
|
||
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•9 years ago
|
||
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()?
Attachment #8723826 -
Flags: review?(gps)
Assignee | ||
Comment 9•9 years ago
|
||
(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.
Assignee | ||
Comment 10•9 years ago
|
||
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/
Attachment #8723826 -
Flags: review?(gps)
Assignee | ||
Comment 11•9 years ago
|
||
Kimura-san, can you give the last version of the patch a go?
Flags: needinfo?(VYV03354)
Assignee | ||
Comment 12•9 years ago
|
||
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•9 years ago
|
||
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.
Attachment #8723826 -
Flags: review?(gps) → review+
Comment 14•9 years ago
|
||
(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.
Flags: needinfo?(VYV03354)
Comment 15•9 years ago
|
||
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•9 years ago
|
||
FYI this patch fixed the issue for me.
Comment 17•9 years ago
|
||
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•9 years ago
|
||
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.
Attachment #8723826 -
Flags: review+
Assignee | ||
Comment 19•9 years ago
|
||
Note the coding line for the old config.status was mbcs on windows, not utf-8. That participates on making things more complicated.
Assignee | ||
Updated•9 years ago
|
Attachment #8723826 -
Attachment description: MozReview Request: Bug 1250297 - Make python configure output config.status instead of old-configure doing it → MozReview Request: Bug 1250297 - Make python configure output config.status instead of old-configure doing it. r=gps
Attachment #8723826 -
Flags: review?(gps)
Assignee | ||
Comment 20•9 years ago
|
||
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•9 years ago
|
||
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.
Attachment #8723826 -
Flags: review?(gps) → review+
Comment 22•9 years ago
|
||
Comment 23•9 years ago
|
||
bugherder |
Status: NEW → RESOLVED
Closed: 9 years ago
status-firefox48:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla48
Updated•7 years ago
|
Product: Core → Firefox Build System
You need to log in
before you can comment on or make changes to this bug.
Description
•