1155816 - move NO_JS_MANIFEST to moz.build

Status: RESOLVED FIXED

(Firefox Build System :: General, defect)

Comment 1

[Nathan Froyd [:froydnj]]

Attachment: part 0 - remove NO_JS_MANIFEST from testing/mochitest/Makefile.in

The lone check for NO_JS_MANIFEST in rules.mk suggests that we need to
be looking at EXTRA_COMPONENTS/EXTRA_PP_COMPONENTS.  But
testing/mochitest/ doesn't define either of those variables, so
NO_JS_MANIFEST is just taking up space in this instance.

Comment 2

[Nathan Froyd [:froydnj]]

Attachment: part 1 - move NO_JS_MANIFEST to moz.build

This patch needs to be applied on top of the patch in bug 1155776.

I'm a little out of my depth here as far as the description in context.py goes,
so if you'd like to wordsmith that, be my guest.

Comment 3

[Nathan Froyd [:froydnj]]

Attachment: part 2 - move EXTRA_*COMPONENTS manifest check to build-backend time

Now that moz.build can see EXTRA_*COMPONENTS and NO_JS_MANIFEST, we can
move some logic from rules.mk (executed every build) to moz.build's
emitter.py (executed only at build-backend time).

This is, in my opinion, a very cool patch, and it's a shame it's hidden in this
mundane bug.

Comment 4

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

Comment on attachment 8594112 [details] [diff] [review]
part 1 - move NO_JS_MANIFEST to moz.build

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

::: python/mozbuild/mozbuild/frontend/context.py
@@ +1298,5 @@
> +        Normally, if you have .js files listed in ``EXTRA_COMPONENTS`` or
> +        ``EXTRA_PP_COMPONENTS``, you are expected to have a corresponding
> +        .manifest file to go with those .js files.  Setting ``NO_JS_MANIFEST``
> +        indicates that the relevant .manifest file and entries for those .js
> +        files are elsehwere (jar.mn, for instance) and this state of affairs

elsewhere

Comment 5

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

Comment on attachment 8594113 [details] [diff] [review]
part 2 - move EXTRA_*COMPONENTS manifest check to build-backend time

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

::: python/mozbuild/mozbuild/frontend/emitter.py
@@ +535,5 @@
> +                                         'or EXTRA_PP_COMPONENTS without a matching '
> +                                         '.manifest file.  See '
> +                                         'https://developer.mozilla.org/en/XPCOM/XPCOM_changes_in_Gecko_2.0 .',
> +                                         context);
> +            

ws

Comment 6

[Michael Shal [:mshal]]

Comment on attachment 8594112 [details] [diff] [review]
part 1 - move NO_JS_MANIFEST to moz.build

The description looks fine as far as I can tell :).

Comment 7

[Michael Shal [:mshal]]

Comment on attachment 8594113 [details] [diff] [review]
part 2 - move EXTRA_*COMPONENTS manifest check to build-backend time

Very nice! This also helps get the error message right at the end of the build output, rather than buried somewhere in the make invocation, which is awesome.

The line after "context);" has some extraneous whitespace... I think Ms2ger would be sad if I didn't point it out. Maybe 'mv .git/hooks/pre-commit.sample .git/hooks/pre-commit' would help?

Comment 8

[Brian O'Keefe [:bokeefe]]

Comment on attachment 8594113 [details] [diff] [review]
part 2 - move EXTRA_*COMPONENTS manifest check to build-backend time

This got pushed to try along with other patches I had on top of it, so I noticed a couple of failures. Looks like the mozbuild python tests need updating to work with the new manifest detecting:

TEST-UNEXPECTED-FAIL | c:/builds/moz2_slave/try-w32-d-00000000000000000000/build/src/python/mozbuild/mozbuild/test/backend/test_recursivemake.py | line 257, test_variable_passthru: A .js component was specified in EXTRA_COMPONENTS or EXTRA_PP_COMPONENTS without a matching .manifest file. See https://developer.mozilla.org/en/XPCOM/XPCOM_changes_in_Gecko_2.0 .
TEST-UNEXPECTED-FAIL | c:/builds/moz2_slave/try-w32-d-00000000000000000000/build/src/python/mozbuild/mozbuild/test/frontend/test_emitter.py | line 157, test_variable_passthru: A .js component was specified in EXTRA_COMPONENTS or EXTRA_PP_COMPONENTS without a matching .manifest file. See https://developer.mozilla.org/en/XPCOM/XPCOM_changes_in_Gecko_2.0 . 

Try push is https://treeherder.mozilla.org/#/jobs?repo=try&revision=3358947a05bd, although these patches are in https://treeherder.mozilla.org/#/jobs?repo=try&revision=23643eec8e8f (which was busted because of my patches)

Comment 9

[Nathan Froyd [:froydnj]]

(In reply to Brian O'Keefe [:bokeefe] from comment #8)
> This got pushed to try along with other patches I had on top of it, so I
> noticed a couple of failures. Looks like the mozbuild python tests need
> updating to work with the new manifest detecting:

Thanks for the report, I'll fix that up prior to landing (just have to declare a .manifest file in EXTRA_COMPONENTS).

Comment 10

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/dacd2e48fbfc
https://hg.mozilla.org/integration/mozilla-inbound/rev/00a8bcfe966d
https://hg.mozilla.org/integration/mozilla-inbound/rev/06d662c40b95

Comment 11

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/95ad98c71c5b

Comment 12

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/dacd2e48fbfc
https://hg.mozilla.org/mozilla-central/rev/00a8bcfe966d
https://hg.mozilla.org/mozilla-central/rev/06d662c40b95
https://hg.mozilla.org/mozilla-central/rev/95ad98c71c5b