Closed Bug 1005209 Opened 7 years ago Closed 7 years ago

Warn anyone looking at b2g-emulator stdout to look at logcat instead

Categories

(Firefox OS Graveyard :: Emulator, defect)

ARM
Gonk (Firefox OS)
defect
Not set
normal

Tracking

(Not tracked)

RESOLVED FIXED
2.0 S6 (18july)

People

(Reporter: botond, Assigned: reznord, Mentored)

Details

(Whiteboard: [good first bug][lang=C++])

Attachments

(1 file, 2 obsolete files)

When using b2g-emulator, a few pieces of output go to stdout/stderr, but most things go to logcat. Someone looking at stdout/stderr might mistakenly believe that that's all the output there is, and miss important things that only go to logcat (like MOZ_ASSERT messages (!), see bug 1004271).

I suggest that we print a big warning to stdout when b2g-emulator starts up, saying "LOOK AT 'adb logcat' INSTEAD!"
Whiteboard: [good first bug][mentor=dhylands][lang=C++]
Mentor: dhylands
Whiteboard: [good first bug][mentor=dhylands][lang=C++] → [good first bug][lang=C++]
Hi,

I am interested in working on this bug. So can you please assign this bug to me?

Thanks in advance,

Regards,
A. Anup
Assignee: nobody → allamsetty.anup
I think that a good place to put the print is here:
http://dxr.mozilla.org/mozilla-central/source/widget/gonk/nsAppShell.cpp#955

(inside the if) Inside the if only happens on the main b2g process (and not on child processes).

I don't think that we need to restrict this to emulator only. I think doing it for regular b2g builds is also fine (stdout on a real phone is normally redirected to /dev/null).

I was thinking that the message should look something like:

*******************************************************
***
*** Most of the useful output will be in logcat.
***
*******************************************************
Actually perhaps

****************************************************************
***
*** This is stdout. Most of the useful output will be in logcat.
***
****************************************************************
(In reply to Botond Ballo [:botond] (away until July 5) from comment #0)
> When using b2g-emulator, a few pieces of output go to stdout/stderr, but
> most things go to logcat. Someone looking at stdout/stderr might mistakenly
> believe that that's all the output there is, and miss important things that
> only go to logcat (like MOZ_ASSERT messages (!), see bug 1004271).
> 
> I suggest that we print a big warning to stdout when b2g-emulator starts up,
> saying "LOOK AT 'adb logcat' INSTEAD!"

Botond - so when you said stdout/stderr were you referring the qemu's stdout/stderr? which looks like this: https://pastebin.mozilla.org/5515045

Or did you mean b2g's stdout? which looks like this: https://pastebin.mozilla.org/5515048

I'm going to guess that you were referring to the qemu stdout, and I'm not sure that we can write there.
Flags: needinfo?(botond)
(In reply to Dave Hylands [:dhylands] from comment #4)
> (In reply to Botond Ballo [:botond] (away until July 5) from comment #0)
> > When using b2g-emulator, a few pieces of output go to stdout/stderr, but
> > most things go to logcat. Someone looking at stdout/stderr might mistakenly
> > believe that that's all the output there is, and miss important things that
> > only go to logcat (like MOZ_ASSERT messages (!), see bug 1004271).
> > 
> > I suggest that we print a big warning to stdout when b2g-emulator starts up,
> > saying "LOOK AT 'adb logcat' INSTEAD!"
> 
> Botond - so when you said stdout/stderr were you referring the qemu's
> stdout/stderr? which looks like this: https://pastebin.mozilla.org/5515045

The pastebin link posted above will be expired. Attaching a new pastebin file:
https://pastebin.mozilla.org/5516116

> 
> Or did you mean b2g's stdout? which looks like this:
> https://pastebin.mozilla.org/5515048

And the link for the above file is https://pastebin.mozilla.org/5516122
> 
> I'm going to guess that you were referring to the qemu stdout, and I'm not
> sure that we can write there.
> And the link for the above file is https://pastebin.mozilla.org/5516122

https://pastebin.mozilla.org/5516123

> > 
> > I'm going to guess that you were referring to the qemu stdout, and I'm not
> > sure that we can write there.
(In reply to Dave Hylands [:dhylands] from comment #4)
> (In reply to Botond Ballo [:botond] (away until July 5) from comment #0)
> > When using b2g-emulator, a few pieces of output go to stdout/stderr, but
> > most things go to logcat. Someone looking at stdout/stderr might mistakenly
> > believe that that's all the output there is, and miss important things that
> > only go to logcat (like MOZ_ASSERT messages (!), see bug 1004271).
> > 
> > I suggest that we print a big warning to stdout when b2g-emulator starts up,
> > saying "LOOK AT 'adb logcat' INSTEAD!"
> 
> Botond - so when you said stdout/stderr were you referring the qemu's
> stdout/stderr? which looks like this: https://pastebin.mozilla.org/5515045
> 
> Or did you mean b2g's stdout? which looks like this:
> https://pastebin.mozilla.org/5515048
> 
> I'm going to guess that you were referring to the qemu stdout, and I'm not
> sure that we can write there.

I was referring to the output of './run-emulator.sh' in the terminal. Looking at your pastebins, it looks like the second one.
Flags: needinfo?(botond)
(In reply to Botond Ballo [:botond] from comment #7)
...snip...
> I was referring to the output of './run-emulator.sh' in the terminal.
> Looking at your pastebins, it looks like the second one.

So the output from run-emulator.sh is the output from the emulator itself, and not the output of b2g.
(In reply to Dave Hylands [:dhylands] from comment #8)
> (In reply to Botond Ballo [:botond] from comment #7)
> ...snip...
> > I was referring to the output of './run-emulator.sh' in the terminal.
> > Looking at your pastebins, it looks like the second one.
> 
> So the output from run-emulator.sh is the output from the emulator itself,
> and not the output of b2g.

Sorry, I misremembered. The command I was running was "./mach mochitest-remote <testname>", not "./run-emulator.sh".
OK - that does indeed seem to be showing b2g's stdout.

Anup - that means that your patch should work fine.

To test, you'll need to follow the steps here: https://bugzilla.mozilla.org/show_bug.cgi?id=1002613#c2

Note: just copy bin/ssltunnel from the firefox zip into gaia/xulrunner-sdk-30/xulrunner-sdk/bin/
And I just ran

> mach mochitest-remote

which will try to run all of the tests, and just hit Control-C when I saw the stdout output from the patch.
Attached patch printing the stdout (obsolete) — Splinter Review
printing the stdout to the b2g emulator.
Attachment #8452971 - Flags: review?(dhylands)
Comment on attachment 8452971 [details] [diff] [review]
printing the stdout

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

::: widget/gonk/nsAppShell.cpp
@@ +981,5 @@
>  
>      InitGonkMemoryPressureMonitoring();
>  
>      if (XRE_GetProcessType() == GeckoProcessType_Default) {
> +        printf("%s", "*****************************************************************\n***\n*** This is stdout. Most of the useful output will be in logcat.\n***\n*****************************************************************\n");

nit: Our coding standards prefer shorter line lengths: https://developer.mozilla.org/en-US/docs/Mozilla/Developer_guide/Coding_Style#Line_Length

Putting it all on one line makes it very hard to see what will actually be printed.

So I would recode this one of 2 ways:

> printf("*****************************************************************\n"
>        "***\n"
>        "*** This is stdout. Most of the useful output will be in logcat.\n"
>        "***\n"
>        "*****************************************************************\n");

> printf("*****************************************************************\n");
> printf("***\n");
> printf("*** This is stdout. Most of the useful output will be in logcat.\n");
> printf("***\n");
> printf("*****************************************************************\n");

In this particular case, the extra overhead of making multiple print calls is essentially irrelevant, and the appearance in the source code is more important.

Since there are no "%" in the string, you also don't need to use "%s"
Attachment #8452971 - Flags: review?(dhylands)
Attached patch Formatted the stdout. (obsolete) — Splinter Review
Formatted the stdout code as requested
Attachment #8452971 - Attachment is obsolete: true
Attachment #8453570 - Flags: review?(dhylands)
Removed the tab and inserted white spaces.
Attachment #8453570 - Attachment is obsolete: true
Attachment #8453570 - Flags: review?(dhylands)
Attachment #8453575 - Flags: review?(dhylands)
Comment on attachment 8453575 [details] [diff] [review]
Indentation is corrected.

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

Looks good - Thanks.
Attachment #8453575 - Flags: review?(dhylands) → review+
The try request link for the patch: 

https://tbpl.mozilla.org/?tree=Try&rev=7835b0150575
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/5442d1ac6f1d
Status: NEW → RESOLVED
Closed: 7 years ago
Flags: in-testsuite-
Resolution: --- → FIXED
Target Milestone: --- → 2.0 S6 (18july)
You need to log in before you can comment on or make changes to this bug.