1220731 - Use moz.build GENERATED_FILES directive to generate embedded JS files

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Jon Coppeard (:jonco)]

As pointed out in review comments for bug 1215063 it should be possible to use moz.build facilities to generate embedded JS files (e.g. for self hosted JS) rather than doing this in the Makefile.

Comment 1

[Jon Coppeard (:jonco)]

Attachment: refactor-embed-script

Here's my first attempt at refactoring the embedjs.py script along the lines discussed briefly on IRC last week.

I'm having some problems with this however:
 - how can I get the value of variables present in the makefile but not in buildconfig? (PREPROCESS_OPTION, DEFINES and ACDEFINES in this case).
 - the makefile exports the generated file, but adding it to EXPORTS in moz.build complains that it doesn't exist.

Any ideas?

Comment 2

[Nathan Froyd [:froydnj]]

(In reply to Jon Coppeard (:jonco) from comment #1)
> Here's my first attempt at refactoring the embedjs.py script along the lines
> discussed briefly on IRC last week.

Cool!

> I'm having some problems with this however:
>  - how can I get the value of variables present in the makefile but not in
> buildconfig? (PREPROCESS_OPTION, DEFINES and ACDEFINES in this case).

For PREPROCESS_OPTION, I would suggest moving it to an AC_SUBST or something like that in configure.in so |buildconfig| can access it.

ACDEFINES is essentially generated from |buildconfig|, so if you have access to defines from |buildconfig|, you've taken care of ACDEFINES.

For DEFINES (which I guess would include things defined in moz.build files), I don't know that there's a good solution right now.  I guess you could duplicate the logic from the moz.build file temporarily, but that's obviously an ugly solution.  mshal, do you have ideas along these lines?

>  - the makefile exports the generated file, but adding it to EXPORTS in
> moz.build complains that it doesn't exist.

Ah, yes.  The current practice is to use INSTALL_TARGETS, rather than listing the generated file in EXPORTS:

http://mxr.mozilla.org/mozilla-central/source/toolkit/components/telemetry/Makefile.in#9

That's obviously something moz.build could (and should!) generate for you.

Comment 3

[Nathan Froyd [:froydnj]]

Comment on attachment 8682042 [details] [diff] [review]
refactor-embed-script

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

::: js/src/builtin/embedjs.py
@@ +136,5 @@
> +  import buildconfig
> +  cxx = shlex.split(buildconfig.substs['CXX'])
> +  cxx_option = "-E -o" # todo: PREPROCESS_OPTION
> +  env = {}
> +  for name in ['MOZ_DEBUG_DEFINES']: # todo: add DEFINES and ACDEFINES

ACDEFINES is just buildconfig.defines, with appropriate massaging.

@@ +142,5 @@
> +      assert value[:2] == "-D"
> +      pair = value[2:].split('=', 1)
> +      if len(pair) == 1:
> +        pair.append(1)
> +        env[pair[0]] = pair[1]

Does this line need to be unindented?  Otherwise, you're not doing anything for the len(pair) == 2 case.

@@ +143,5 @@
> +      pair = value[2:].split('=', 1)
> +      if len(pair) == 1:
> +        pair.append(1)
> +        env[pair[0]] = pair[1]
> +  embed(cxx, cxx_option, "js.msg", sources, out, None, env)

Maybe provide the full path to js.msg as the first input (so it's easy to grab it out of all the .js inputs) to generate_selfhosted?

Comment 4

[Michael Shal [:mshal]]

(In reply to Nathan Froyd [:froydnj] from comment #2)
> For DEFINES (which I guess would include things defined in moz.build files),
> I don't know that there's a good solution right now.  I guess you could
> duplicate the logic from the moz.build file temporarily, but that's
> obviously an ugly solution.  mshal, do you have ideas along these lines?

Are we sure that any of the DEFINES are actually needed for this step? Just grepping for the DEFINES set in js/src/moz.build in the list of selfhosted.inputs doesn't show any results, but that's not definitive. If we don't actually need them that would be simplest :). Otherwise, I'd guess we need some way to pass them in to the py_action.

Comment 5

[Jon Coppeard (:jonco)]

Attachment: refactor-embed-script v2

OK, here's an updated patch with review comments applied.

I removed use of DEFINES since we didn't actually use any of them, although this is a possible gotcha for future changes.

I had to define PREPROCESS_OPTION in both configure.in and js/src/configure.in to make all the builds work.  Maybe this can be factored out somehow?

Finally I had to add some extra arguments to the python script so that they would be treated as dependencies.  Is there a better way to do this?

Comment 6

[Nathan Froyd [:froydnj]]

(In reply to Jon Coppeard (:jonco) from comment #5)
> I had to define PREPROCESS_OPTION in both configure.in and
> js/src/configure.in to make all the builds work.  Maybe this can be factored
> out somehow?

That would be nice, maybe mshal will have ideas on where to put that.

> Finally I had to add some extra arguments to the python script so that they
> would be treated as dependencies.  Is there a better way to do this?

Not really, no.  You'll note that we do the same thing in the current Makefile.in code.  The only two ways to make this better are:

1) Extract information from the preprocessor run, which is complicated and varies across compilers.
2) Enhance preprocessor.py to be able to cope with more C-like preprocessor constructs and use that instead.  (This might not be that bad if you restrict it to non-varargs macros via #define and no token pasting...)

Comment 7

[Nathan Froyd [:froydnj]]

Comment on attachment 8683194 [details] [diff] [review]
refactor-embed-script v2

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

This looks fine to me, with the changes below.  I'm not a build peer, though, so redirecting this to mshal.  If mshal wants to consider my approval as r+, that's up to him.

::: configure.in
@@ +9138,5 @@
> +else
> +    PREPROCESS_OPTION="-E -o "
> +fi
> +
> +echo "PREPROCESS_OPTION '$GNU_CC' '$OS_ARCH' '$PREPROCESS_OPTION'"

Presumably you want to take this debugging code out?

::: js/src/builtin/embedjs.py
@@ +144,5 @@
> +def generate_selfhosted(c_out, msg_file, *inputs):
> +  # Called from moz.build to embed selfhosted JS.
> +  import buildconfig
> +  deps = [path for path in inputs if re.search(r"\.h$", path)]
> +  sources = [path for path in inputs if re.search(r"\.js$", path)]

These can use path.endswith(".h") and path.endswith(".js"), respectively.

::: js/src/configure.in
@@ +3816,5 @@
> +else
> +    PREPROCESS_OPTION="-E -o "
> +fi
> +
> +echo "PREPROCESS_OPTION '$GNU_CC' '$OS_ARCH' '$PREPROCESS_OPTION'"

Presumably you want to take this debugging code out?

Comment 8

[Shu-yu Guo [:shu]]

Comment on attachment 8683194 [details] [diff] [review]
refactor-embed-script v2

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

My review is mainly for embedjs.py. The rest looks fine to me, but I'm no expert in the build system.

::: js/src/builtin/embedjs.py
@@ +180,5 @@
>      sys.exit(1)
>    cxx = shlex.split(options.c)
>    msgs = messages(options.m)
> +  with open(options.s, 'w') as js_out, open(options.o, 'w') as c_out:
> +    embed(cxx, options.p, msgs, sources, c_out, js_out, env)

I'd like there to be only one way to use the script. If from now on it's only supposed to be called from the build infra via |generate_selfhosted|, then just don't let it be called from the commandline.

Comment 9

[Michael Shal [:mshal]]

Comment on attachment 8683194 [details] [diff] [review]
refactor-embed-script v2

glandium, can you take a look? I'm out until Monday.

Comment 10

[Mike Hommey [:glandium]]

Comment on attachment 8683194 [details] [diff] [review]
refactor-embed-script v2

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

I agree with Nathan's and Shu's comments, too.

::: js/src/Makefile.in
@@ +278,5 @@
>  endif
>  
> +INSTALL_TARGETS += selfhosted_out_h
> +selfhosted_out_h_FILES = selfhosted.out.h
> +selfhosted_out_h_DEST = $(DIST)/include  # this doesn't do anything?

It should be doing something. Now the question is whether that's actually desired. As a matter of fact, the file is not exported to DIST/include currently, and is only included by SelfHosting.cpp, so I'd say this whole INSTALL_TARGETS section is not necessary.

Comment 11

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/6d7d90a28e05

Comment 12

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/6d7d90a28e05