Brian Hackett [Laid off!] Reporter
	Description • 10 years ago

As part of bug 979480 I changed js/src/builtin/TypedObjectConstants.h, which is included by the JS engine's self hosted code.  This did not seem to cause the selfhosted.js to be regenerated, leading to test failures and the push required a clobber.

	Nathan Froyd [:froydnj] Assignee
	Comment 1 • 10 years ago

Is the selfhosted.js actually used for anything?  It looks like it's simply there for debugging purposes; a quick grep doesn't show up selfhosted.js as actually being used.

Do you mean selfhosted.out.h?

	Brian Hackett [Laid off!] Reporter
	Comment 2 • 10 years ago

OK, yes, selfhosted.out.h looks like the right file.

	Nathan Froyd [:froydnj] Assignee
	Comment 3 • 10 years ago

TypedObject.js #includes TypedObjectConstants.h, but nothing in the build
system knows about that.  Let's fix that.

	Mike Hommey [:glandium]
	Comment 4 • 10 years ago

Comment on attachment 8392387 [details] [diff] [review]
make selfhosted.out.h explicitly depend on TypedObjectConstants.h

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

::: js/src/Makefile.in
@@ +382,5 @@
>  selfhosted_out_h_deps := \
>    $(selfhosting_srcs) \
>    $(srcdir)/js.msg \
>    $(srcdir)/builtin/embedjs.py \
> +  $(srcdir)/builtin/TypedObjectConstants.h \

Are you sure this is the only header needed here? It seems to me we should "just" get the preprocessor invocations out of embedjs.py, and have those generate proper depfiles. That would solve bug 980793 at the same time. Or have embedjs.py have the preprocessor generate depfiles, but this is trickier because it doesn't know the right flags, which, for instance, there isn't for msvc.

	Nathan Froyd [:froydnj] Assignee
	Comment 5 • 10 years ago

(In reply to Mike Hommey [:glandium] from comment #4)
> >  selfhosted_out_h_deps := \
> >    $(selfhosting_srcs) \
> >    $(srcdir)/js.msg \
> >    $(srcdir)/builtin/embedjs.py \
> > +  $(srcdir)/builtin/TypedObjectConstants.h \
> 
> Are you sure this is the only header needed here?

[froydnj@cerebro gecko-dev.git]$ grep '#include' js/src/builtin/*.js
js/src/builtin/TypedObject.js:#include "TypedObjectConstants.h"
[froydnj@cerebro gecko-dev.git]$

Yes.

> It seems to me we should
> "just" get the preprocessor invocations out of embedjs.py, and have those
> generate proper depfiles. That would solve bug 980793 at the same time. Or
> have embedjs.py have the preprocessor generate depfiles, but this is
> trickier because it doesn't know the right flags, which, for instance, there
> isn't for msvc.

Both of these are essentially "make the (real) preprocessor generate depfiles", which is a lot of pain for not very much gain IMHO.

Alternatively, we could mostly move things out of embedjs.py and have this whole process use preprocessor.py, which would get us most of the way to proper dependencies at the cost of using a sort-of-but-not-quite C-like preprocessor.

	Nathan Froyd [:froydnj] Assignee
	Comment 6 • 10 years ago

(In reply to Nathan Froyd (:froydnj) from comment #5)
> Alternatively, we could mostly move things out of embedjs.py and have this
> whole process use preprocessor.py, which would get us most of the way to
> proper dependencies at the cost of using a sort-of-but-not-quite C-like
> preprocessor.

Scratch that.  preprocessor.py doesn't strip comments, which ~doubles the size of the uncompressed selfhsoted.js.

	Mike Hommey [:glandium]
	Comment 7 • 10 years ago

Comment on attachment 8392387 [details] [diff] [review]
make selfhosted.out.h explicitly depend on TypedObjectConstants.h

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

I guess this is fine for now, then.

	Nathan Froyd [:froydnj] Assignee
	Comment 8 • 10 years ago

Thanks for relenting. :)

https://hg.mozilla.org/integration/mozilla-inbound/rev/73ca2e36b051

	Carsten Book [:Tomcat]
	Comment 9 • 10 years ago

https://hg.mozilla.org/mozilla-central/rev/73ca2e36b051