Early-Warning-System

Early Warning System

Once a new review is created or an old review has been updated, the Early Warning System kicks in, by watching for new patch reviews to process.

This system is basically a collection of bots that will run a wide array of tests on the patch. The crucial part about these tests is that they should all be quick to run, 5-10 minutes or perhaps even less. The idea is not to catch all errors, but to get immediate/early feedback on a patch.

If you get a -1 from a review bot, please ensure you look at the issues noted by the bot, and upload a new version. The review bots will then automatically review the update and give you feedback.

h2. The Simple Sanitizer

1. The line ending check. We are developing in a heterogeneous environment with both Unix and Windows machines. Therefore it is imperative to have all files in the repository in the canonical LF-only format. Windows users need to set the git option

   core.autocrlf
   

   to

   true
   

   to automatically get CRLF line endings which are suitable for the native tools.
2. Conflict marker check. Unless you are adding a text file which contains the patterns on purpose, you most probably botched a merge/rebase.
3. Generated file check. Don't commit generated files- they bloat the repository and create spurious changes. If a file is generated but you don't know how to automate the build process, ask an expert.
4. Backup file check. Don't commit backup files, for hopefully obvious reasons.
5. Copyright header check. Source files must declare copyright (unless they are very small). They should also name a licence, but the check does not cover that.
6. Check for project/config files of alien build systems. As we are using qmake, these will in most cases fall under the "generated file" rule anyway.
7. Big file addition check. Think three times whether you really need that huge media file, test binary or screenshot. As we are using git, such mistakes are not correctable - once you added a huge file, everyone who clones the repository will have to pay for it with network traffic and disk space for all times. Source code files are exempt from the check at all; for the rest it is a bit paranoid (it will complain about anything bigger than 50 KiB or an instantaneous file growth over 150% if the file is bigger than 20 KiB).
8. Huge file addition check. If you trigger that one, you almost certainly did something wrong.
9. Work In Progress push check.' This is meant to avoid that somebody accidentally submits something you marked for later cleanup. It checks for the strings "WIP" and "'*" in the summary line and for an empty Reviewed-by line. This check is disabled in the local post-commit hook for obvious reasons.
10. Some simple checks on the commit message:
    1. Compliance with "the expected structure":http://tbaggery.com/2008/04/19/a-note-about-git-commit-messages.html.
    2. That reverts are properly explained, which follows directly from the Commit Policy point 8.2.
    3. That the author email address does not reflect an unconfigured git setup.
    4. That the author real name looks reasonable. Use proper capitalization and periods after initials.
       1. Exceptions can be requested, but must be reasonably justified.
    5. That the summary does not refer to JIRA issues. Use Task-number footers for that. If the reader needs to know which issue a commit refers to, they most definitely should read more than just the commit summary anyway.
    6. That no Reviewed-by footers have been manually added, as they would stay behind even if the mentioned person did not actually make a review.
    7. That no Signed-off-by footers have been added, as they are redundant with the act of signing the "CLA":http://qt.io/legal.html (advisory only).
    8. That there are no empty lines between footers. The footers (including any "(cherry picked from …)" lines) should form one tidy set-off block. If you are getting an empty line before the automatically added Change-Id line, you are most probably using a bad commit-msg hook - get a fresh copy from our Gerrit, or use the one from our "qtrepotools repository":http://qt.gitorious.org/qt/qtrepotools/blobs/master/git-hooks/gerrit_commit_msg_hook. If you are using the "Qt supermodule":https://qt.gitorious.org/qt/qt5/, you can run

       init-repository -f —force-hooks
       

       to update your hooks.
    9. That "JIRA issues":https://bugreports.qt.io/ are not referenced by URL. URLs make the commit messages noisy and unnecessarily bind them to the current location of the bug tracker. Use the Task-number footer according to the "commit template":http://qt.gitorious.org/qt/qt5/blobs/stable/.commit-template to make "Gerrit":https://codereview.qt.io/ automatically link issues.
    10. That other commits are not referenced by Gerrit Change-Id or URL. Change-Ids have the disadvantage of introducing an unnecessary indirection when browsing history. URLs are also noisy, non-relocatable, and make off-line lookup impossible. Use the SHA1 of the commit that has been actually merged (the automatically added new PatchSet seen in Gerrit Changes).
    11. That [ChangeLog] is correctly formed if present:
        1. That the tag is camel-cased, and does not try to be a footer.
        2. That the paragraph is preceeded and followed by an empty line.
        3. That tags don't mention JIRA issues. Use Task-number footers for that.
        4. That tags don't mention the current repository, as that's redundant.
        5. That the last tag is followed by a space (or line break), for tidyness.
11. Some simple checks for incorrect use of Apple brands/trademarks/terminology:
    1. That the author is aware that the define Q_OS_MAC and qmake scope mac refer to both the OS X and iOS operating systems, since it is commonly mistaken to refer only to OS X.
    2. That the OS X operating system is correctly named in all instances. OS X is the name of Apple's Darwin-based flagship Desktop operating system and is a trademark; while the 'X' is pronounced as 'ten' and did and does refer to the major version number of the operating system, this is always ignored when writing out the full product name, for example "OS X 10.9 Mavericks", never "OS 10.9 Mavericks". The operating system was formerly named "Mac OS X" but was changed to "OS X" in 2012 with the release of Mountain Lion. Many Apple marketing materials "backported" this name change to Lion as well. Whether a bump to major version 11 would have any impact on this cannot be determined until that happens, if it ever does. Terms such as "Mac", "Macintosh", "Mac OS", "Mac OS X", "OSX", etc., with any combination of spacing and punctuation, should not be used to refer to the operating system. Note that the hardware platform on which OS X runs is correctly termed "Mac" or "Macintosh". To re-emphasize, ONLY use "OS X" to refer to the software, and ONLY use "Mac" or "Macintosh" to refer to the hardware.
    3. That the deprecated define

       Q_OS_MACX
       

       and qmake scope

       macx
       

       (or similar deprecated constructs such as

       defined(Q_OS_MAC) && !defined(Q_OS_IOS)
       

       and

       mac:!ios
       

       ) are not being used. Their respective replacements,

       Q_OS_OSX
       

       and

       osx
       

       should be used instead. Note that some uses of

       macx
       

       in a qmake file may still be valid, for example

       macx-
       

       or

       macx*
       

       in reference to certain mkspecs (which cannot be renamed for backwards compatibility reasons).
12. A check about touching certain files. Some files are considered "endangered" - seemingly innocuous modifications almost always turn out to be bogus when not done by one of a few experts.
13. A check for common spelling mistakes.
14. Check for mixing whitespace and non-whitespace changes. As implied by the "don't mix unrelated changes" rule, a commit may contain only whitespace changes or "proper" changes (with whitespace cleanups only on already changed lines, and, as an exception, additions and removals of empty lines). This helps to ensure the usefulness of

    git blame
    

    (note that a clean history is better than using

    git blame -w
    

    , as the latter will also hide significant whitespace changes). A negative verdict may be overridden if it is a false positive resulting from imperfect heuristics, in particular if the change is fixing coding style issues beyond mere whitespace cleanup.
15. Various whitespace abuse checks. Tabs in files they are not approved in, trailing whitespace, missing newline at end of file, etc.
16. Some simple Coding Style checks. (currently advisory only)

The bot's verdict is a recommendation, and may be overridden by every Approver. However, don't ignore it unless you have a really good reason. "I need to get this done now" is not a valid reason. Neither is "I cannot be bothered to understand what it wants from me". If there is any doubt, discuss alternatives with somebody appropriate. Each override needs to come with a comment on Gerrit.

It is possible to run most of the checks locally before attempting to push. See the

qtrepotools/git-hooks/git_post_commit_hook

script (it contains installation instructions).

The sanitizer script is available from the "qtrepotools repository":https://qt.gitorious.org/qt/qtrepotools.