858543 - Creating "Warning Line Police" to reduce compile-time warning

Status: NEW

(Firefox Build System :: General, defect)

Description

[ISHIKAWA, Chiaki]

Attachment: AWK script to collect warning lines from LONG LOG of TB TryServer

Maybe "Warning Line Guard" might be a better name, you decide.

There came an issue of detecting new extra compile-time warning lines that may show up in one's submission to TryServer.
Letting the coder know that the submission added/caused certain warning
lines not seen before sounds to be a good practice.

https://bugzilla.mozilla.org/show_bug.cgi?id=563195#c19
--- begin quote
At this point, I think I agree that it's not worth anybody's time to get this fully working.  Something that I've wanted for a long time, though, is enabling the build system to tell you which warnings you have introduced compared to the previous build.  I think that goes a long way to help using compiler warnings locally when writing code.
--- end quote

(There is already a bugzilla entry
Bug 187528 - (buildwarning) [META] Fix compiler 'Build Warnings'
But that is about after-the-fact clean-up efforts.

This bugzilla entry is about NOT INTRODUCING NEW WARNINGS if possible.)


This is an attempt to start the discussion.

Attached is an awk script that collects warning lines from TB TryServer
LONG LOG for compilation UNDER LINUX using GCC
in the order of appearances.

(It should not be that difficult for someone may be able to write similar script for MS-C for Windows, and clang.)

egrep -3 warning: LONG-LOG-FILE | gawk -f find-warning.awk

produces output with a header line at the beginning, and each group of warning lines is separated by a blank line. (It should be easy for someone to
change the output format. After all it is an awk script.)
I will upload an example output from my TB TryServer run in the next post.

CAVEAT:

Note that the WARNING lines produced may not be the
responsibility of someone who sends a modification of source files to be
compiled and tested on TB TryServer.

The warnings may come from already existing headers:
The detected warning messages may well be caused by internal problems of
already existing header files that are included in newly modified and
compiled files.

(Since I noticed TB TryServer uses ccache, non-touched source files
don't cause re-compilation. So I assume that
most of the warning lines are caused by the compilation of
newly modified files, though.)

Some or most of the warning lines are due to the existing header lines in my
experience. --- these can be weeded out by comparing the output from the
script to the a base output of warning lines produced by running the script
against the LOG of the whole recompilation of the sources daily or
something. Let us call it a baseline (for comparison).

Yes, there are cases where the warning lines are caused by existing source
files that include OUR new modified header files.
These should be duly checked.

These issues  can be worked eventually.

The attached AWK script (I think it sticks to the POSIX awk, but I didn't check) prints out the warning lines from LONG LOG of linux GCC compilation on TB Tryserver in the order of appearance.

With the limitation mentioned above in mind,
we can create a base warning line output file (from the session log of, say daily or something, that
compiles the whole set of files afresh. That is the key. We want the baseline to cover as many as warning lines possible to reduce false positives.)

Then we can record the warning lines from each TB TryServer submission using the script here and compare them to the output from the base line compilation. 

The warning lines that appear only in the submission are new to the
newly modified source files (OR from the existing OTHER files who include
the newly modified header files. This is tricky.)
Also, the warnings can be caused by the inclusion of problematic header
files that already exist.

But the last points need to be judged by the coder herself/himself.

If the line number and column position in the source file name
(:line#:col#:) in the warning output is problematic, we can easily remove them from the output of the script (simply remove them when we record the lines) so that comparison later would become easier (the line number change will not matter in the comparison hopefully).

Hope this helps to start a discussion.

Someone who are familiar with the TryServer infrastructure can
comment on the feasibility of
 - keeping a daily record of master warning lines (for the fresh compilation of all the source files, maybe for 10 days or something.)

 - after each run of TryServer submission (FF and TB),
   running  the script and comparing the output against the master warning
line file (maybe 7 days ago.)

Then filtering of the comparison:
 - If there are more outputs than the old, they ought to be scrutinized by the coder.
   Reduction,  means a win for all the community, but
   again, due to the difficulty of what change is causing the warning, we
   can't be sure whether a particular change diminished the number of the
warning or not.
(Trying to cook up something using standard "diff -u" should not be that difficult.)

 - Anyway, if we can implement this mechanism in TB and FF TryServer,
   please display such data/result as WARNING status in a PROMINENT place
right next to the test result.

I think the lack of emphasis to reduce warning has let people accumulate all
the crufts. Yes, I know that the GCC and other compilers have become picky
about these things recently with their warning options. So we are seeing
extra warnings now.

But let us face it, we already had more than 1000 warnings to start with,
say, for couple of years ago. (See Bug 187528 )
This is way beyond a number for a reasonably maintainable code base.

Output run against a LONG LOG of TB TryServer is attached in the
next upload.

Looking at it, I think I can create a series of good examples of
"How not to write C/C++ for Maintainable Code"

TIA

PS. For example, warning like the following makes me wonder. If this is OK, maybe we can add something like the following to the code to save future maintenance people's mental time.

#warn "the following warning about 'unsigned expression < 0 is always false'
can be ignored safely (by WHO-TO-BLAME.)

The comment in the code is very cryptic to me and not sure if this can be
ignored or not. It seems to be saying if parameter is unsigned, this
should not be bothered. In that case, I wonder if there is a compile time
trick to not to use is_negative at all.

../../../../mozilla/ipc/chromium/src/base/string_util.cc:993:27: warning:
comparison of unsigned expression < 0 is always false

If the prominent display about compiler warning is shown in TB TryServer
result, then MORE people will start thinking about this and other warning issues.

That will be good.

Comment 1

[ISHIKAWA, Chiaki]

Attachment: Output from a real TB TryServer LONG LOG

This is an output from the LONG LOG of real TB TryServer run.

https://tbpl.mozilla.org/php/getParsedLog.php?id=21459977&tree=Thunderbird-Try

TIA

Comment 2

[ISHIKAWA, Chiaki]

Attachment: Improved Awk Script

My quick hack did not handle some lines very well.
Here is a modified version although the previous version works well enough for the purpose of extracting warning lines for comparison purposes.

Comment 3

[Gregory Szorc [:gps]]

We have code for detecting compiler warnings in the tree:

https://hg.mozilla.org/mozilla-central/file/139b6ba547fa/python/mozbuild/mozbuild/compilation/warnings.py

This is what mach uses to count warnings. The only known issue with the code is it raises if it fails to attribute a warning to a specific file. Although, you could change the code to optionally not enforce this behavior.

This code should be robust enough for your needs. It even has a database to persist warnings between compile invocations and to prune the database of stale entries. I highly recommend you leverage it.

Comment 4

[ISHIKAWA, Chiaki]

Attachment: Output from the improved AWK script

The second version produces the attached output.

For comparison purposes, the sparse output from the previous version
probably works well enough.

Grouping is more aggressively reported now.

I know that the header inclusion relationship is not reported in the
output. But, again, the current output is good enough
for later comparison purposes to detect new warning lines.

TIA

Comment 5

[ISHIKAWA, Chiaki]

(In reply to Gregory Szorc [:gps] from comment #3)
> We have code for detecting compiler warnings in the tree:
> 
> https://hg.mozilla.org/mozilla-central/file/139b6ba547fa/python/mozbuild/
> mozbuild/compilation/warnings.py
> 
> This is what mach uses to count warnings. The only known issue with the code
> is it raises if it fails to attribute a warning to a specific file.
> Although, you could change the code to optionally not enforce this behavior.
> 
> This code should be robust enough for your needs. It even has a database to
> persist warnings between compile invocations and to prune the database of
> stale entries. I highly recommend you leverage it.

Sorry, I missed this post earlier.

I will take a look and see what we can do with this to
create warning checker/guard/police, etc.

TIA

Comment 6

[ISHIKAWA, Chiaki]

I tested "mach" on my local compilation of TB (comm-central) code.
It took me a while to figure out where "mach" lives in comm-central.
It turns out to reside in ./mozilla/ subdirectory.

So I needed to invoke it as
./mozilla/mach
after setting some environmental variable to tell MOZCONFIG position, PATH to pick up "ld.gold" instead of "ld".
These settings have been wrapped inside a shell script to invoke make client.mk directly.

I liked the output:
The print out of elapsed time give me clues how long linking of libxul.so for linux32 build takes: About 5-6 minutes. 

Also, I like the short summary and warning picked up by the tool.

I read a few discussions about enabling WARNING_as_ERROR discussion in the bugzilla.
It seems that, once upon a time, the warning lines　(or its summary) were shown in the
quickly reviewable log, but now we have to download the file (a mental hurdle that involves a certain amout of time). 

After my experience of mozilla/mach tool, my initial proposal is this.

Without making the feature too complex (forgetting about the comparison with
base line warning database, etc.) , why not
 - compile TB TryServer submission using mozilla/mach tool,
 - and show the warning line summary after a successful compilation.

The second point needs more explanation.
At the end of successful compilation,
we pick up the output of  "mozilla/mach warnings-summary" and
the first 100 lines of "mozilla/mach warnings-list" 
and show it when the submitter of jobs click on the
"B" symbol (B for build) on the status monitor web page.
People click on the "B" symbol to see if the compilation is successful, etc.
(Also, the same content should be added to the Brief Log toward the begining.)

Here is a concrete example using the output of my own compilation.

--- begin quote ---
One line about successful compilation. Then comes
Your job generated the following warnings. 
The warning type summary:
77 compiler warnings present.  <--- This actually is picked up from [1] or
                                    you can use the Total in the summary below.
1	-Wtype-limits	       <--- This is the output of
2	-Wmaybe-uninitialized	    mozilla/mach warnings-summary
2	-Wparentheses
2	-Wformat
3	-Woverloaded-virtual
5	-Wswitch
9	-Wsign-compare
23	-Wunused-but-set-variable
30	-Wdelete-non-virtual-dtor
77	Total

Please consider removing the warnings even if they are caused by
header files that you include (or source files that include your
modified headers).  Future and current maintainers will appreciate the
reduction of warnings.
See Bug 187528 - (buildwarning) [META] Fix compiler 'Build Warnings'
for the current state of source tree.

The first 100 lines of warnings recorded by mozilla/mach tool follows [2]

mailnews/base/search/src/nsMsgLocalSearch.cpp:278:9 [-Wswitch] case value '2153054214' not in enumerated type 'nsresult {aka tag_nsresult}'
mailnews/base/search/src/nsMsgLocalSearch.cpp:279:9 [-Wswitch] case value '2153054213' not in enumerated type 'nsresult {aka tag_nsresult}'
mailnews/base/search/src/nsMsgLocalSearch.cpp:404:14 [-Wunused-but-set-variable] variable 'err' set but not used
mailnews/base/search/src/nsMsgSearchAdapter.cpp:794:2848 [-Wdelete-non-virtual-dtor] deleting object of polymorphic class type 'nsMsgSearchValidityTable' which has non-virtual destructor might cause undefined behaviour
mailnews/base/search/src/nsMsgSearchNews.cpp:406:14 [-Wunused-but-set-variable] variable 'err' set but not used
mailnews/base/src/nsMessengerUnixIntegration.cpp:110:107 [-Wdelete-non-virtual-dtor] deleting object of polymorphic class type 'nsMessengerUnixIntegration' which has non-virtual destructor might cause undefined behaviour
mailnews/base/src/nsMsgAccountManager.cpp:2578:2840 [-Wdelete-non-virtual-dtor] deleting object of polymorphic class type 'VirtualFolderChangeListener' which has non-virtual destructor might cause undefined behaviour
mailnews/base/src/nsMsgDBView.cpp:6251:10 [-Wunused-but-set-variable] variable 'threadElided' set but not used
mailnews/base/src/nsMsgDBView.cpp:7649:2856 [-Wdelete-non-virtual-dtor] deleting object of polymorphic class type 'nsMsgDBView::nsMsgViewHdrEnumerator' which has non-virtual destructor might cause undefined behaviour
mailnews/base/src/nsMsgFolderCache.cpp:84:13 [-Wunused-but-set-variable] variable 'mdberr' set but not used
mailnews/base/src/nsMsgFolderCompactor.cpp:431:41 [-Wsign-compare] comparison between signed and unsigned integer expressions
mailnews/base/src/nsMsgFolderNotificationService.cpp:17:2930 [-Wdelete-non-virtual-dtor] deleting object of polymorphic class type 'nsMsgFolderNotificationService' which has non-virtual destructor might cause undefined behaviour
mailnews/base/src/nsMsgPurgeService.cpp:351:65 [-Wmaybe-uninitialized] 'purgeIntervalToUse' may be used uninitialized in this function
mailnews/base/src/nsMsgQuickSearchDBView.cpp:841:53 [-Wparentheses] suggest parentheses around arithmetic in operand of '^'
mailnews/base/src/nsMsgTagService.cpp:73:2627 [-Wdelete-non-virtual-dtor] deleting object of polymorphic class type 'nsMsgTag' which has non-virtual destructor might cause undefined behaviour
mailnews/base/src/nsMsgTagService.cpp:122:2697 [-Wdelete-non-virtual-dtor] deleting object of polymorphic class type 'nsMsgTagService' which has non-virtual destructor might cause undefined behaviour
mailnews/base/src/nsMsgXFViewThread.cpp:160:48 [-Wmaybe-uninitialized] 'parentIndex' may be used uninitialized in this function
mailnews/base/util/nsImapMoveCoalescer.cpp:166:2840 [-Wdelete-non-virtual-dtor] deleting object of polymorphic class type 'nsMoveCoalescerCopyListener' which has non-virtual destructor might cause undefined behaviour
mailnews/base/util/nsMsgDBFolder.cpp:5209:47 [-Wformat] format '%p' expects argument of type 'void*', but argument 3 has type 'nsIMsgWindow*'
mailnews/base/util/nsMsgIdentity.cpp:31:228 [-Wdelete-non-virtual-dtor] deleting object of polymorphic class type 'nsMsgIdentity' which has non-virtual destructor might cause undefined behaviour
mailnews/base/util/nsMsgIncomingServer.cpp:2249:37 [-Wsign-compare] comparison between signed and unsigned integer expressions
mailnews/base/util/nsMsgTxn.cpp:96:2693 [-Wdelete-non-virtual-dtor] deleting object of polymorphic class type 'nsMailSimpleProperty' which has non-virtual destructor might cause undefined behaviour
mailnews/base/util/nsMsgUtils.cpp:622:15 [-Wunused-but-set-variable] variable 'last' set but not used
mailnews/base/util/nsMsgUtils.cpp:772:6 [-Wparentheses] suggest explicit braces to avoid ambiguous 'else'
mailnews/compose/src/nsMsgComposeService.cpp:899:107 [-Wdelete-non-virtual-dtor] deleting object of polymorphic class type 'nsMsgTemplateReplyHelper' which has non-virtual destructor might cause undefined behaviour
mailnews/compose/src/nsMsgSend.cpp:429:8 [-Wunused-but-set-variable] variable 'multipart_p' set but not used
mailnews/compose/src/nsMsgSend.cpp:430:8 [-Wunused-but-set-variable] variable 'plaintext_is_mainbody_p' set but not used
mailnews/compose/src/nsMsgSend.cpp:432:9 [-Wunused-but-set-variable] variable 'buffer_tail' set but not used
mailnews/compose/src/nsMsgSend.cpp:434:8 [-Wunused-but-set-variable] variable 'tonews' set but not used
mailnews/compose/src/nsMsgSend.cpp:3375:9 [-Wswitch] case value '2153066792' not in enumerated type 'nsresult {aka tag_nsresult}'
mailnews/compose/src/nsMsgSend.cpp:4985:2719 [-Wdelete-non-virtual-dtor] deleting object of polymorphic class type 'nsMsgAttachmentData' which has non-virtual destructor might cause undefined behaviour
mailnews/compose/src/nsMsgSend.cpp:5093:2701 [-Wdelete-non-virtual-dtor] deleting object of polymorphic class type 'nsMsgAttachedFile' which has non-virtual destructor might cause undefined behaviour
mailnews/compose/src/nsMsgSendLater.cpp:1019:57 [-Wsign-compare] comparison between signed and unsigned integer expressions
mailnews/db/msgdb/src/nsDBFolderInfo.cpp:268:19 [-Wunused-but-set-variable] variable 'folderInfoTableOID' set but not used
mailnews/db/msgdb/src/nsMailDatabase.cpp:404:12 [-Wunused-but-set-variable] variable 'key' set but not used
mailnews/db/msgdb/src/nsMsgDatabase.cpp:80:2695 [-Wdelete-non-virtual-dtor] deleting object of polymorphic class type 'nsMsgDBService' which has non-virtual destructor might cause undefined behaviour
mailnews/db/msgdb/src/nsMsgDatabase.cpp:1692:15 [-Wunused-but-set-variable] variable 'mdberr' set but not used
mailnews/extensions/mdn/src/nsMsgMdnGenerator.cpp:1100:7 [-Wswitch] case value '2153066792' not in enumerated type 'nsresult {aka tag_nsresult}'
mailnews/imap/src/nsAutoSyncManager.cpp:28:2821 [-Wdelete-non-virtual-dtor] deleting object of polymorphic class type 'nsDefaultAutoSyncMsgStrategy' which has non-virtual destructor might cause undefined behaviour
mailnews/imap/src/nsAutoSyncManager.cpp:115:2855 [-Wdelete-non-virtual-dtor] deleting object of polymorphic class type 'nsDefaultAutoSyncFolderStrategy' which has non-virtual destructor might cause undefined behaviour
mailnews/imap/src/nsAutoSyncManager.cpp:1413:2736 [-Wdelete-non-virtual-dtor] deleting object of polymorphic class type 'nsAutoSyncManager' which has non-virtual destructor might cause undefined behaviour
mailnews/imap/src/nsAutoSyncState.cpp:689:2697 [-Wdelete-non-virtual-dtor] deleting object of polymorphic class type 'nsAutoSyncState' which has non-virtual destructor might cause undefined behaviour
mailnews/imap/src/nsImapMailFolder.cpp:826:7 [-Wswitch] case value '2153054228' not in enumerated type 'nsresult {aka tag_nsresult}'
mailnews/imap/src/nsSyncRunnableHelpers.cpp:12:944 [-Wdelete-non-virtual-dtor] deleting object of polymorphic class type 'StreamListenerProxy' which has non-virtual destructor might cause undefined behaviour
mailnews/imap/src/nsSyncRunnableHelpers.cpp:13:960 [-Wdelete-non-virtual-dtor] deleting object of polymorphic class type 'ImapMailFolderSinkProxy' which has non-virtual destructor might cause undefined behaviour
mailnews/imap/src/nsSyncRunnableHelpers.cpp:14:944 [-Wdelete-non-virtual-dtor] deleting object of polymorphic class type 'ImapServerSinkProxy' which has non-virtual destructor might cause undefined behaviour
mailnews/imap/src/nsSyncRunnableHelpers.cpp:15:235 [-Wdelete-non-virtual-dtor] deleting object of polymorphic class type 'ImapMessageSinkProxy' which has non-virtual destructor might cause undefined behaviour
mailnews/imap/src/nsSyncRunnableHelpers.cpp:17:236 [-Wdelete-non-virtual-dtor] deleting object of polymorphic class type 'ImapProtocolSinkProxy' which has non-virtual destructor might cause undefined behaviour
mailnews/local/src/nsLocalMailFolder.cpp:1988:86 [-Wtype-limits] comparison of unsigned expression >= 0 is always true
mailnews/local/src/nsLocalUndoTxn.cpp:435:47 [-Wsign-compare] comparison between signed and unsigned integer expressions
mailnews/local/src/nsLocalUndoTxn.cpp:443:54 [-Wsign-compare] comparison between signed and unsigned integer expressions
mailnews/local/src/nsMsgBrkMBoxStore.cpp:50:2729 [-Wdelete-non-virtual-dtor] deleting object of polymorphic class type 'nsMsgBrkMBoxStore' which has non-virtual destructor might cause undefined behaviour
mailnews/local/src/nsMsgBrkMBoxStore.cpp:245:21 [-Wsign-compare] comparison between signed and unsigned integer expressions
mailnews/local/src/nsMsgMaildirStore.cpp:50:2729 [-Wdelete-non-virtual-dtor] deleting object of polymorphic class type 'nsMsgMaildirStore' which has non-virtual destructor might cause undefined behaviour
mailnews/local/src/nsParseMailbox.cpp:2147:16 [-Wunused-but-set-variable] variable 'matchTermStatus' set but not used
mailnews/local/src/nsParseMailbox.cpp:2288:59 [-Wformat] format '%p' expects argument of type 'void*', but argument 3 has type 'nsIMsgWindow*'
mailnews/local/src/nsPop3IncomingServer.cpp:661:2784 [-Wdelete-non-virtual-dtor] deleting object of polymorphic class type 'nsPop3GetMailChainer' which has non-virtual destructor might cause undefined behaviour
mailnews/mime/emitters/src/nsMimeBaseEmitter.h:63:1090 [-Woverloaded-virtual] 'virtual nsresult nsMimeBaseEmitter::EndHeader(const nsACString_internal&)' was hidden
mailnews/mime/emitters/src/nsMimeHtmlEmitter.cpp:55:2830 [-Wdelete-non-virtual-dtor] deleting object of polymorphic class type 'nsMimeStringEnumerator' which has non-virtual destructor might cause undefined behaviour
mailnews/mime/emitters/src/nsMimePlainEmitter.h:25:62 [-Woverloaded-virtual]   by 'virtual nsresult nsMimePlainEmitter::EndHeader()'
mailnews/mime/emitters/src/nsMimeXmlEmitter.h:27:62 [-Woverloaded-virtual]   by 'virtual nsresult nsMimeXmlEmitter::EndHeader()'
mailnews/mime/src/mimedrft.cpp:460:42 [-Wsign-compare] comparison between signed and unsigned integer expressions
mailnews/mime/src/mimedrft.cpp:467:25 [-Wdelete-non-virtual-dtor] deleting object of polymorphic class type 'nsMsgAttachedFile' which has non-virtual destructor might cause undefined behaviour
mailnews/mime/src/mimedrft.cpp:1567:15 [-Wdelete-non-virtual-dtor] deleting object of polymorphic class type 'nsMsgAttachedFile' which has non-virtual destructor might cause undefined behaviour
mailnews/mime/src/mimedrft.cpp:1569:47 [-Wsign-compare] comparison between signed and unsigned integer expressions
mailnews/mime/src/mimedrft.cpp:1672:8 [-Wunused-but-set-variable] variable 'needURL' set but not used
mailnews/mime/src/mimedrft.cpp:1674:8 [-Wunused-but-set-variable] variable 'bodyPart' set but not used
mailnews/mime/src/mimedrft.cpp:1924:24 [-Wsign-compare] comparison between signed and unsigned integer expressions
mailnews/news/src/nsNNTPNewsgroupList.cpp:504:9 [-Wunused-but-set-variable] variable 'dateStr' set but not used
mailnews/news/src/nsNNTPNewsgroupList.cpp:505:9 [-Wunused-but-set-variable] variable 'authorStr' set but not used
mailnews/news/src/nsNNTPNewsgroupList.cpp:753:8 [-Wunused-but-set-variable] variable 'read_p' set but not used
mailnews/news/src/nsNNTPProtocol.cpp:2560:57 [-Wunused-but-set-variable] variable 'flag' set but not used
mailnews/news/src/nsNNTPProtocol.cpp:2561:11 [-Wunused-but-set-variable] variable 'oldest' set but not used
mailnews/news/src/nsNNTPProtocol.cpp:2561:19 [-Wunused-but-set-variable] variable 'youngest' set but not used
mailnews/news/src/nsNNTPProtocol.cpp:2832:9 [-Wunused-but-set-variable] variable 'description' set but not used
mailnews/news/src/nsNewsFolder.cpp:131:2694 [-Wdelete-non-virtual-dtor] deleting object of polymorphic class type '{anonymous}::AsyncAuthMigrator' which has non-virtual destructor might cause undefined behaviour
mailnews/news/src/nsNewsFolder.cpp:781:8 [-Wunused-but-set-variable] variable 'newsrcHasChanged' set but not used

[end of warning summary]


[1] At the end of the compilation mozilla/mach printed out the following.
77 compiler warnings present.
We know it took a while, but your build finally finished successfully!

[2] This is the output from "mozilla/mach warnings-list". We can certainly create a database of known warnings from daily (or known good weekly build that compiles everything freshly without ccache), and mark the known warning in the output.
But after reading the disucssion of WARNING_AS_ERRORS, and noticed a comment that
there used to be warning summary from tryserver output and it was handy, 
I feel we can keep it simple for now. 

Just showing the warnings in a prominent place and
let us trust the programmers to fix maybe one warning now and then.
1000 programmers will eliminate 1000 warnings in no time :-)


One problem:

But before proceeding with Thunderbird TryServer, I think we need
to fix the following strange message that was printed at the end of the successful compilation.
I suspect that for firefox compilation, this does something useful.

  [... build is approaching the end. ...]

^[[34m13:18.51^[[m     Installing mozmill-thunderbird script to /TEST-MAIL-DIR/objdir-tb3/mozilla/_tests/mozmill-virtualenv/bin
^[[34m13:18.51^[[m     Installing mozmill-restart script to /TEST-MAIL-DIR/objdir-tb3/mozilla/_tests/mozmill-virtualenv/bin
^[[34m13:18.52^[[m     Installing mozmill script to /TEST-MAIL-DIR/objdir-tb3/mozilla/_tests/mozmill-virtualenv/bin
^[[34m13:18.53^[[m Successfully installed ManifestDestiny simplejson mozrunner jsbridge mozmill
^[[34m13:18.53^[[m Cleaning up...
^[[34m13:18.59^[[m Python: 2.7.3 (default, Jan	2 2013, 16:53:07)
^[[34m13:18.59^[[m [GCC 4.7.2]
^[[34m13:19.78^[[m 77 compiler warnings present.
We know it took a while, but your build finally finished successfully!
Error running mach:

    ['build']

The error occurred in code that was called by the mach command. This is either
a bug in the called code itself or in the way that mach is calling it.

You should consider filing a bug for this issue.

If filing a bug, please include the full output of mach, including this error
message.

The details of the failure are as follows:

Exception: Binary expected at /COMM-CENTRAL/comm-central/../objdir-tb3/dist/bin/thunderbird does not exist.

  File "/COMM-CENTRAL/comm-central/mozilla/python/mozbuild/mozbuild/mach_commands.py", line 134, in build
    app_path = self.get_binary_path('app')
  File "/COMM-CENTRAL/comm-central/mozilla/python/mozbuild/mozbuild/base.py", line 161, in get_binary_path
    raise Exception('Binary expected at %s does not exist.' % path)


You may notice that that I used "script" command to record the compilation.
The elapsed time was shown in light-blue using a particular terminal's escape sequence to
cotrol color. We need to set the terminal type to "dumb" or something during TryServer job, which I think is already done or something. I replaced ESC with literal "^[" above.

My tentative conclusion:
usage of mozilla/mach is very promising.

Any comments from people, especially the long-term maintainer of the mozilla source code base itself, and the trybuilder management?

Honestly speaking, when I fetched TB source code some years ago, after seeing
a severe bug and decided to fix it myself, and saw SO MANY WARNING LINES, I could not
believe that people would put up with the source code base. Honest.

TIA

Comment 7

[Gregory Szorc [:gps]]

Most of your ideas are good and should be implemented.

I filed bug 860963 earlier today to get client.mk building with the warnings capturing code. Once that is in place, dumping output for TBPL consumption, etc is much easier.

Regarding issues with mach, it's not exactly well-integrated with comm-central. In theory someone just needs to copy m-c's mach into c-c and change a few paths and it should "just work." I think there is a bug on that...

Comment 8

[ISHIKAWA, Chiaki]

(In reply to Gregory Szorc [:gps] from comment #7)
> Most of your ideas are good and should be implemented.
> 
> I filed bug 860963 earlier today to get client.mk building with the warnings
> capturing code. Once that is in place, dumping output for TBPL consumption,
> etc is much easier.
> 
> Regarding issues with mach, it's not exactly well-integrated with
> comm-central. In theory someone just needs to copy m-c's mach into c-c and
> change a few paths and it should "just work." I think there is a bug on
> that...

Thank you for the feedback.

> Exception: Binary expected at /COMM-CENTRAL/comm-central/../objdir-tb3/dist/bin/thunderbird does not exist.

Re this error, thunderbird binary lives under
/COMM-CENTRAL/comm-central/../objdir-tb3/mozilla/dist/bin/thunderbird, i.e., "/mozilla"
was missing before "/dist", and I assume that is indeed an installation issue of mach tool. I will investigate this a little further.

TIA

Comment 9

[ISHIKAWA, Chiaki]

I think once c-c and m-c merge in one way or the other occurs maybe later this year(?), this can be taken care of more
smoothly.

Comment 10

[ISHIKAWA, Chiaki]

(In reply to ISHIKAWA, Chiaki from comment #9)
> I think once c-c and m-c merge in one way or the other occurs maybe later
> this year(?), this can be taken care of more
> smoothly.

I am afraid that this has to wait for another year?
In any case, M-C and C-C merge would have a merit in these build infrastructure issues.

TIA