614128 - remove obsolete/redundant commands and options

Status: RESOLVED FIXED

(Add-on SDK Graveyard :: General, defect)

Description

[Myk Melez [:myk] [@mykmelez]]

We should remove obsolete commands and options, along with options that are redundant with conventional best practices, from the cfx tool.

Specifically, we should remove the "xpcom" command and its --srcdir and --objdir options, as Mozilla is deprecating XPCOM in favor of js-ctypes, and we're removing the nsjetpack package, on which the "xpcom" command appears to depend.

And we should remove --profile-memory, which also depends on nsjetpack.

Finally, we should remove --logfile, as conventional best practice is to use shell redirection to obtain this functionality, although we may need to leave it in to direct output to the user's terminal on Windows (as otherwise it unusefully goes to a separate console window), in which case we should mark it as an internal command that might change or go away in the future.

Also cc:ing Lukas for feedback on that last change, in case the continuous integration infrastructure relies on --logfile, and we should keep it around for that reason.

Comment 1

[Atul Varma [:atul]]

So one thing I realized when fixing bug 610507 is that '--profile-memory' actually does include some functionality that doesn't require nsjetpack: by default, the memory.track() function stores a weak reference to an object, and --profile-memory makes the Add-on SDK display statistics after each test-run iteration about which weak references are still valid after tests have run. Using this flag also displays about:memory statistics at the end of each iteration.

Aside from strange exceptions like bug 592180, this is generally a useful tool we can use to find some kinds of memory leaks, so I'm tempted to say we should actually keep the non-nsijetpack-using functionality of --profile-memory.

Comment 2

[Atul Varma [:atul]]

Regarding --logfile, I looked at cfx's source code again and it looks like cfx on Windows effectively uses this feature "under the hood" to effectively tail a temporary logfile in order to direct console.log() and other output to the terminal in which cfx was executed. However, it doesn't use it in a way that necessitates we keep the command-line option, so that is good.

I believe that this feature isn't used by linux/cygwin "under the hood", but one thing I've just noticed when experimenting with shell redirection on OS X is that all console output coming from the XULRunner/Firefox process--which under-the-hood just uses dump()--is being sent to stderr instead of stdout. Yet all non-error output coming from cfx itself is sent to stdout, which means that both stderr *and* stdout need to be redirected in order to get normal console output.

I don't have a Windows machine/VM on-hand, but I expect that on Windows, due to the fact that cfx is tailing the logfile and printing its contents to sys.stdout, things work a bit more "as expected" on it: all normal output from both cfx and the XULRunner process should go to stdout, but errors from the XULRunner process will also go to stdout because the XULRunner process doesn't have a notion of a separate stream for errors.

I don't like this divergent behavior between Windows and the other OS's. We could change cfx to also do the "tailing the logfile" thing on OSX and Linux, in which case all platforms would behave the same--that is, all output from the XULRunner process would be sent to stdout.

Either way, we'll be able to get rid of the --logfile command-line option unless Lukas really needs it.

Comment 3

[Atul Varma [:atul]]

I've separated out the removal of the 'xpcom' command and associated command-line options to bug 614568, which I can tackle separately from these other concerns.

Comment 4

[Drew Willcoxon :adw]

(In reply to comment #2)
> I believe that this feature isn't used by linux/cygwin "under the hood", but
> one thing I've just noticed when experimenting with shell redirection on OS X
> is that all console output coming from the XULRunner/Firefox process--which
> under-the-hood just uses dump()--is being sent to stderr instead of stdout.

I noticed when fixing a dump() bug (bug 570291 comment 3 last para) that SandboxDump uses stderr for some reason. :\

http://mxr.mozilla.org/mozilla-central/source/js/src/xpconnect/src/xpccomponents.cpp#3019

Comment 5

[Myk Melez [:myk] [@mykmelez]]

(In reply to comment #1)
> Aside from strange exceptions like bug 592180, this is generally a useful tool
> we can use to find some kinds of memory leaks, so I'm tempted to say we should
> actually keep the non-nsijetpack-using functionality of --profile-memory.

I'm ok with that, but it sounds like the kind of thing to which we haven't yet dedicated the time and attention it would take to call it a supported option for regular users of the Add-on SDK, so I would then instead make it either experimental or internal (depending on who the first "we" is in your quoted sentence).


(In reply to comment #2)
> Either way, we'll be able to get rid of the --logfile command-line option
> unless Lukas really needs it.

Ok, great, let's get rid of it, then.

Comment 6

[Myk Melez [:myk] [@mykmelez]]

Atul: is this something you can take on?  If removing --logfile is complicated, because of the extant uses of the underlying implementation, then I'm ok with moving it to Internals for the time being while we work out the longer term fix.

Comment 7

[Atul Varma [:atul]]

Yeah, due to the extant uses of --logfile, it's a little less trivial than I wish it was. So it looks like --logfile and --profile-memory should both be thrown in the Internals bin.

Comment 8

[Atul Varma [:atul]]

How should this bug be resolved, by the way, since some options were actually removed while others were moved to Internals? It seems like the bug should be resolved in some way at this point, since the only changes left need to happen to documentation, which are covered by bug 614343 and bug 614130.

Comment 9

[Myk Melez [:myk] [@mykmelez]]

Let's resolve it fixed.