--enable-jsd=no fails to build

RESOLVED FIXED in mozilla14



7 years ago
7 years ago


(Reporter: prabu, Assigned: prabu)


Bug Flags:
in-testsuite -

Firefox Tracking Flags

(Not tracked)



(2 attachments, 1 obsolete attachment)



7 years ago
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

Attachment #605683 - Flags: review?(bobbyholley+bmo)
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:


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


.... other function


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. :-)
Attachment #605683 - Flags: review?(bobbyholley+bmo) → review-

Comment 2

7 years ago
Thanks for the comments - uploaded new patch at below location. Can you please confirm ?


Comment 3

7 years ago
Created attachment 605891 [details] [diff] [review]
Modified per comments from Bobby
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?
Attachment #605891 - Flags: review+

Comment 5

7 years ago
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 ?
(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 ?

Keywords: checkin-needed
Assignee: nobody → prabu
Version: 2.0 Branch → Trunk
Attachment #605683 - Attachment is obsolete: true

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!
Flags: in-testsuite-
Keywords: checkin-needed
Target Milestone: --- → mozilla14
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:


Can you do something like that for Makefile.in, and verify that the flag is actually defined for a vanilla browser build?

Comment 10

7 years ago
The flag gets defined in the generated autoconf.mk. Will update again with more information. This was validated on Linux ARM builds.
Duplicate of this bug: 711840
Ever confirmed: true
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.
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.
Attachment #607213 - Flags: review?(khuey)
Last Resolved: 7 years ago
Resolution: --- → FIXED

Comment 16

7 years ago
Thanks for adding this.
You need to log in before you can comment on or make changes to this bug.