Last Comment Bug 735594 - --enable-jsd=no fails to build
: --enable-jsd=no fails to build
Status: RESOLVED FIXED
:
Product: Core
Classification: Components
Component: XPConnect (show other bugs)
: Trunk
: All All
: -- normal (vote)
: mozilla14
Assigned To: prabindh
:
Mentors:
: 711840 (view as bug list)
Depends on:
Blocks: 738879
  Show dependency treegraph
 
Reported: 2012-03-14 01:42 PDT by prabindh
Modified: 2012-03-23 20:13 PDT (History)
6 users (show)
ryanvm: in‑testsuite-
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Patch for fix (1001 bytes, patch)
2012-03-14 01:42 PDT, prabindh
bobbyholley: review-
Details | Diff | Splinter Review
Modified per comments from Bobby (1.03 KB, patch)
2012-03-14 12:23 PDT, prabindh
bobbyholley: review+
Details | Diff | Splinter Review
With more build goop (4.06 KB, patch)
2012-03-19 10:46 PDT, Phil Ringnalda (:philor)
khuey: review+
Details | Diff | Splinter Review

Description prabindh 2012-03-14 01:42:12 PDT
Created attachment 605683 [details] [diff] [review]
Patch for fix

With --enable-jsd=no, jsdIDebuggerService should not be used. Since there is no check for MOZ_JSDEBUGGER, compilation fails.

mozilla-central/js/xpconnect/src/nsXPConnect.cpp:2464: error: 'jsdIDebuggerService' was not declared in this scope

Patch added for this at below location - also attached

https://github.com/prabindh/mozilla-embedded/blob/master/patches/13Mar/07_qws_compile_nsXPConnect.cpp.diff
Comment 1 Bobby Holley (:bholley) (busy with Stylo) 2012-03-14 11:47:27 PDT
Comment on attachment 605683 [details] [diff] [review]
Patch for fix

This works, but is a bit confusing. Can we just have 2 separate versions of nsXPConnect::CheckForDebugMode, one for each version of the ifdef? I'm thinking:

#ifndef MOZ_JSDEBUGGER

void
nsXPConnect::CheckForDebugMode(JSRuntime *rt)
{
gDesiredDebugMode = gDebugMode = false;
}

#else

.... other function

#endif


Also, if you wouldn't mind correcting the open-brace on the existing nsXPConnect::CheckForDebugMode to be on its own line, I'd much appreciate it. ;-)

FWIW jsd is going away sometime soon. :-)
Comment 2 prabindh 2012-03-14 12:22:01 PDT
Thanks for the comments - uploaded new patch at below location. Can you please confirm ?

https://github.com/prabindh/mozilla-embedded/blob/master/patches/13Mar/08_qws_compile_nsXPConnect.cpp.diff
Comment 3 prabindh 2012-03-14 12:23:23 PDT
Created attachment 605891 [details] [diff] [review]
Modified per comments from Bobby
Comment 4 Bobby Holley (:bholley) (busy with Stylo) 2012-03-14 12:30:44 PDT
Comment on attachment 605891 [details] [diff] [review]
Modified per comments from Bobby

Looks good. r=bholley

Do you need someone to land the patch to mozilla-central for you?
Comment 5 prabindh 2012-03-14 21:03:18 PDT
Thanks, I have no idea how to "land the patch to mozilla-central" :) If there is some documentation on how to do that please point me, or if someone else doing it is simpler, please do so.

BTW - what is going to replace jsd ? Any roadmap publicly available ?
Comment 6 Bobby Holley (:bholley) (busy with Stylo) 2012-03-14 22:58:17 PDT
(In reply to prabindh from comment #5)
> Thanks, I have no idea how to "land the patch to mozilla-central" :) If
> there is some documentation on how to do that please point me, or if someone
> else doing it is simpler, please do so.

Yeah, you need commit access. I set the checkin-needed keyword, which should cause somebody to do it in the next few days. If you don't see any activity on this bug within a week, let me know and I'll make something happen. :-)

> BTW - what is going to replace jsd ? Any roadmap publicly available ?

https://developer.mozilla.org/en/SpiderMonkey/JS_Debugger_API_Guide
Comment 7 Ryan VanderMeulen [:RyanVM] 2012-03-15 17:14:27 PDT
https://hg.mozilla.org/integration/mozilla-inbound/rev/ca1873b20652

Thanks for the patch! To make life easier for those of us checking in your patches for you, please follow the directions below in the future. Thanks!
https://developer.mozilla.org/en/Mercurial_FAQ#How_can_I_generate_a_patch_for_somebody_else_to_check-in_for_me.3F
Comment 8 Ryan VanderMeulen [:RyanVM] 2012-03-15 18:29:13 PDT
Backed out due to mochitest-4 orange.
https://hg.mozilla.org/integration/mozilla-inbound/rev/e5fb2d7b5108
Comment 9 Bobby Holley (:bholley) (busy with Stylo) 2012-03-15 18:43:18 PDT
prabindh - So, this task broke the jsd tests, so it was backed out. :-(

It looks like that ifdef isn't actually available to the source file, so it's never defined. Apparently, you have to manually add it to DEFINES in the Makefile, like the other code here:

http://mxr.mozilla.org/mozilla-central/search?string=MOZ_JSDEBUGGER

Can you do something like that for Makefile.in, and verify that the flag is actually defined for a vanilla browser build?
Comment 10 prabindh 2012-03-15 19:27:33 PDT
The flag gets defined in the generated autoconf.mk. Will update again with more information. This was validated on Linux ARM builds.
Comment 11 Phil Ringnalda (:philor) 2012-03-18 21:55:41 PDT
*** Bug 711840 has been marked as a duplicate of this bug. ***
Comment 12 Phil Ringnalda (:philor) 2012-03-18 22:07:58 PDT
Right, it gets defined in autoconf.mk, and autoconf.mk gets included in generated Makefiles, and that's why the ifdef in a Makefile like http://mxr.mozilla.org/mozilla-central/source/dom/base/Makefile.in#151 works, but then to have it work for the preprocessor for an #ifdef in .cpp, it has to be in DEFINES. Your patch would build with both --enable-jsd and --disable-jsd, it just wouldn't ever work for --enable-jsd because MOZ_JSDEBUGGER would never be DEFINEd, so CheckForDebugMode would always return false.
Comment 13 Phil Ringnalda (:philor) 2012-03-19 10:46:16 PDT
Created attachment 607213 [details] [diff] [review]
With more build goop

The interdiff is all build goop, so that's what I want a second review on.

https://tbpl.mozilla.org/?tree=Try&rev=fb3b77c4c486 is tryserver doing a Linux32 opt --disable-jsd and everything else --enable, passing the tests that failed before.
Comment 15 Mounir Lamouri (:mounir) 2012-03-20 03:56:27 PDT
https://hg.mozilla.org/mozilla-central/rev/41143e1cae85
Comment 16 prabindh 2012-03-20 04:56:06 PDT
Thanks for adding this.

Note You need to log in before you can comment on or make changes to this bug.