Closed Bug 1272488 Opened 8 years ago Closed 8 years ago

Move GenerateCSSPropsGenerated.py invocation to moz.build

Categories

(Firefox Build System :: General, defect)

defect
Not set
normal

Tracking

(firefox50 fixed)

RESOLVED FIXED
mozilla50
Tracking Status
firefox50 --- fixed

People

(Reporter: mshal, Assigned: mshal)

References

(Blocks 1 open bug)

Details

Attachments

(2 files)

The GenerateCSSPropsGenerated.py script needs to be cleaned up a bit before we can invoke it from moz.build. The Makefile also runs CPP to pass a preprocessed file in via stdin, but we can use buildconfig to lookup CPP and run it from the python script instead.

Fixing this also fixes this error message in automation build logs:

 0:05.23 /bin/sh: 1: -DOS_LINUX=1: not found
 0:05.25 Traceback (most recent call last):
 0:05.25   File "/home/mshal/mozilla-central-hg/layout/style/GenerateCSSPropsGenerated.py", line 10, in <module>
 0:05.25     for (i, p) in enumerate(eval(sys.stdin.read()))]
 0:05.25   File "<string>", line 0
 0:05.25
 0:05.25     ^
 0:05.25 SyntaxError: unexpected EOF while parsing

(CPP isn't set in this case, so we should only build this when COMPILE_ENVIRONMENT is set).
I split it into two patches for easier review - the first one is mostly indentation changes from putting things into functions.
Comment on attachment 8751904 [details]
MozReview Request: Bug 1272488 - Create a main() function in GenerateCSSPropsGenerated.py; r?ted

https://reviewboard.mozilla.org/r/52293/#review55484

Sorry for letting these sit so long.

::: layout/style/GenerateCSSPropsGenerated.py:78
(Diff revision 1)
>      return ",\n".join(map(lambda (p, position): "  %d" % position, ps))
>  
> -cppFile = open(sys.argv[1], "r")
> +def generate(output, cppTemplate):
> +    cppFile = open(cppTemplate, "r")
> -cppTemplate = cppFile.read()
> +    cppTemplate = cppFile.read()
> -cppFile.close()
> +    cppFile.close()

Maybe switch this to use a `with` block while you're here?
Attachment #8751904 - Flags: review?(ted) → review+
Comment on attachment 8751905 [details]
MozReview Request: Bug 1272488 - Move GenerateCSSPropsGenerated.py invocation to moz.build; r?ted

https://reviewboard.mozilla.org/r/52295/#review55488

::: layout/style/GenerateCSSPropsGenerated.py:12
(Diff revision 1)
>  import argparse
> +import subprocess
> +import buildconfig
>  
> -def get_properties():
> +def get_properties(preprocessorHeader):
> +    cpp = buildconfig.substs['CPP'].split()

I'm not 100% sure, but you might want to use `shlex.split` here.

::: layout/style/GenerateCSSPropsGenerated.py:17
(Diff revision 1)
> +    cpp = buildconfig.substs['CPP'].split()
> +    cpp.append(preprocessorHeader)
> +    preprocessed = subprocess.check_output(cpp)
>      properties = [{"name":p[0], "prop":p[1], "id":p[2],
>                     "flags":p[3], "pref":p[4], "proptype":p[5]}
> -                  for (i, p) in enumerate(eval(sys.stdin.read()))]
> +                  for (i, p) in enumerate(eval(preprocessed))]

Oh god, I never even noticed the `eval` here with all the other things going on. That's pretty ridiculous!

::: layout/style/Makefile.in:6
(Diff revision 1)
>  # This Source Code Form is subject to the terms of the Mozilla Public
>  # License, v. 2.0. If a copy of the MPL was not distributed with this
>  # file, You can obtain one at http://mozilla.org/MPL/2.0/.
>  
>  # TODO This list should be emitted to a .pp file via
>  # GenerateCSSPropsGenerated.py.

The `file_generate` script we use under the hood supports scripts returning dependencies as a set from the method it calls:
https://dxr.mozilla.org/mozilla-central/rev/ec20b463c04f57a4bfca1edb987fcb9e9707c364/python/mozbuild/mozbuild/action/file_generate.py#67

You could wire this up by having `generate` return the set of files the preprocessor read by parsing the preprocessor output. The preprocessor spits out line numbers from input files on lines starting with `#`, which looks like:

for gcc/clang:
```
# 1 "/build/mozilla-central/layout/style/PythonCSSProps.h"
# 1 "<built-in>"
# 1 "<command-line>"
# 1 "/usr/include/stdc-predef.h" 1 3 4
# 1 "<command-line>" 2
# 1 "/build/mozilla-central/layout/style/PythonCSSProps.h"
```

for msvc:
```
#line 1 "c:/build/mozilla-central/layout/style/PythonCSSProps.h"
#line 1 "c:\\build\\mozilla-central\\layout\\style\\nsCSSPropList.h"
#line 168 "c:\\build\\mozilla-central\\layout\\style\\nsCSSPropList.h"
```

Feel free to punt this to a followup, but I think it should be straightforward.

::: layout/style/moz.build:248
(Diff revision 1)
>  ]
>  
>  style_struct_list = GENERATED_FILES['nsStyleStructList.h']
>  style_struct_list.script = 'generate-stylestructlist.py'
>  
> +if CONFIG['COMPILE_ENVIRONMENT']:

I guess it would be nice if we had a way to express real dependencies for `SOURCES` on `GENERATED_FILES`.
Attachment #8751905 - Flags: review?(ted) → review+
(In reply to Ted Mielczarek [:ted.mielczarek] from comment #5)
> Comment on attachment 8751905 [details]
> MozReview Request: Bug 1272488 - Move GenerateCSSPropsGenerated.py
> invocation to moz.build; r?ted
> 
> https://reviewboard.mozilla.org/r/52295/#review55488
> 
> ::: layout/style/GenerateCSSPropsGenerated.py:12
> (Diff revision 1)
> >  import argparse
> > +import subprocess
> > +import buildconfig
> >  
> > -def get_properties():
> > +def get_properties(preprocessorHeader):
> > +    cpp = buildconfig.substs['CPP'].split()
> 
> I'm not 100% sure, but you might want to use `shlex.split` here.

If you do, don't use shlex.split, use mozbuild.shellutil.split
(In reply to Ted Mielczarek [:ted.mielczarek] from comment #5)
> ::: layout/style/GenerateCSSPropsGenerated.py:12
> > -def get_properties():
> > +def get_properties(preprocessorHeader):
> > +    cpp = buildconfig.substs['CPP'].split()
> 
> I'm not 100% sure, but you might want to use `shlex.split` here.

Done! (with mozbuild.shellutil.split)

> ::: layout/style/Makefile.in:6
> (Diff revision 1)
> >  # This Source Code Form is subject to the terms of the Mozilla Public
> >  # License, v. 2.0. If a copy of the MPL was not distributed with this
> >  # file, You can obtain one at http://mozilla.org/MPL/2.0/.
> >  
> >  # TODO This list should be emitted to a .pp file via
> >  # GenerateCSSPropsGenerated.py.
> 
> The `file_generate` script we use under the hood supports scripts returning
> dependencies as a set from the method it calls:
> https://dxr.mozilla.org/mozilla-central/rev/
> ec20b463c04f57a4bfca1edb987fcb9e9707c364/python/mozbuild/mozbuild/action/
> file_generate.py#67
>
> Feel free to punt this to a followup, but I think it should be
> straightforward.

Filed bug 1281614.

> ::: layout/style/moz.build:248
> (Diff revision 1)
> >  ]
> >  
> >  style_struct_list = GENERATED_FILES['nsStyleStructList.h']
> >  style_struct_list.script = 'generate-stylestructlist.py'
> >  
> > +if CONFIG['COMPILE_ENVIRONMENT']:
> 
> I guess it would be nice if we had a way to express real dependencies for
> `SOURCES` on `GENERATED_FILES`.

Hmm, I'm not sure what you mean here. Can you clarify?
I replied on IRC, but for the record, I believe I meant that it would be nice if we could specify that specific SOURCES depend on GENERATED_FILES, so that the generation would be run as a prerequisite for compilation instead of just blindly during the export tier like we do now, so that in the --disable-compile-environment case they just wouldn't get run.
(In reply to Ted Mielczarek [:ted.mielczarek] from comment #8)
> I replied on IRC, but for the record, I believe I meant that it would be
> nice if we could specify that specific SOURCES depend on GENERATED_FILES, so
> that the generation would be run as a prerequisite for compilation instead
> of just blindly during the export tier like we do now, so that in the
> --disable-compile-environment case they just wouldn't get run.

Ahh, I think I see what you mean. That would help make it a more general solution - in this case it wasn't really an attempt to get it to build only when needed, but because it would fail without the compile environment since CPP isn't defined, which we need in the python script.
Pushed by mshal@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/cdf2830f4dde
Create a main() function in GenerateCSSPropsGenerated.py; r=ted
https://hg.mozilla.org/integration/mozilla-inbound/rev/7ed69db97223
Move GenerateCSSPropsGenerated.py invocation to moz.build; r=ted
Sure, but if we were only building it when needed then presumably it'd work in that case without explicitly having to wrap it in `if CONFIG['COMPILE_ENVIRONMENT']`, right? I think at some point we're going to have to grow some more features in moz.build to encode some information that's currently baked into the recursive make backend (like the tier ordering etc), this is just one bit of it.
(In reply to Ted Mielczarek [:ted.mielczarek] from comment #11)
> Sure, but if we were only building it when needed then presumably it'd work
> in that case without explicitly having to wrap it in `if
> CONFIG['COMPILE_ENVIRONMENT']`, right? I think at some point we're going to
> have to grow some more features in moz.build to encode some information
> that's currently baked into the recursive make backend (like the tier
> ordering etc), this is just one bit of it.

Yeah, I think that should work if we are able to generate something like a '%.o: <some generated file>' rule in the backend. Though I guess we'd still have to specify something in the GENERATED_FILES entry as for whether or not it's needed for compilation, since I believe there are some generated files that we want even without the compilation environment. Or were you thinking it's automatic somehow based on what is being generated?

Note that this dom/bindings Makefile rule was already inside an 'ifdef COMPILE_ENVIRONMENT', but the layout/style rule from bug 1272488 was not, which caused a weird error message in artifact builds since CPP wasn't defined:

 0:43.41 /bin/sh: 1: -DOS_LINUX=1: not found
 0:43.45 labelsencodings.properties.h
 0:43.48 Traceback (most recent call last):
 0:43.48   File "/home/mshal/mozilla-central-hg/layout/style/GenerateCSSPropsGenerated.py", line 10, in <module>
 0:43.48     for (i, p) in enumerate(eval(sys.stdin.read()))]
 0:43.48   File "<string>", line 0

Curiously this didn't actually fail the build (I guess because of the pipe?), but just made for some noise.
https://hg.mozilla.org/mozilla-central/rev/cdf2830f4dde
https://hg.mozilla.org/mozilla-central/rev/7ed69db97223
Status: NEW → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla50
Blocks: 1284169
Hi Michael, could you help to take a look at bug 1284169 ? We need this bug being fixed before enabling mask shorthand feature. Thanks.
Flags: needinfo?(mshal)
Ahh, when I was moving it to moz.build I tried to check if there was anything in ACDEFINES that could affect the output and didn't find anything. I guess I either missed that, or only looked at the similar script in dom/bindings. Sorry for breaking that feature!
Flags: needinfo?(mshal)
Product: Core → Firefox Build System
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: