Closed Bug 1155816 Opened 5 years ago Closed 5 years ago

move NO_JS_MANIFEST to moz.build

Categories

(Firefox Build System :: General, defect)

defect
Not set

Tracking

(firefox40 fixed)

RESOLVED FIXED
mozilla40
Tracking Status
firefox40 --- fixed

People

(Reporter: froydnj, Assigned: froydnj)

References

Details

Attachments

(3 files)

No description provided.
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.
Attachment #8594111 - Flags: review?(mshal)
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.
Attachment #8594112 - Flags: review?(mshal)
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.
Attachment #8594113 - Flags: review?(mshal)
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 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
Attachment #8594111 - Flags: review?(mshal) → review+
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 :).
Attachment #8594112 - Flags: review?(mshal) → review+
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?
Attachment #8594113 - Flags: review?(mshal) → review+
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)
(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).
See Also: → 1158751
Product: Core → Firefox Build System
You need to log in before you can comment on or make changes to this bug.