chiark / gitweb /
output= is now a glob path and can do gradle
[fdroidserver.git] / fdroidserver / lint.py
index d8d6a962586ecc45ef8c71037e295a9f63fe30c2..8760e1db527dc0e4f999f6807b5c69b8b3ace0d5 100644 (file)
 # You should have received a copy of the GNU Affero General Public Licen
 # along with this program.  If not, see <http://www.gnu.org/licenses/>.
 
-from optparse import OptionParser
+from argparse import ArgumentParser
 import re
-import logging
-import common
-import metadata
 import sys
-from collections import Counter
 from sets import Set
 
+import common
+import metadata
+import rewritemeta
+
 config = None
 options = None
 
@@ -37,11 +37,10 @@ def enforce_https(domain):
 https_enforcings = [
     enforce_https('github.com'),
     enforce_https('gitlab.com'),
-    enforce_https('gitorious.org'),
+    enforce_https('bitbucket.org'),
     enforce_https('apache.org'),
     enforce_https('google.com'),
     enforce_https('svn.code.sf.net'),
-    enforce_https('googlecode.com'),
 ]
 
 
@@ -55,32 +54,31 @@ http_url_shorteners = [
     forbid_shortener('ur1.ca'),
 ]
 
-http_warnings = https_enforcings + http_url_shorteners + [
+http_checks = https_enforcings + http_url_shorteners + [
     (re.compile(r'.*github\.com/[^/]+/[^/]+\.git'),
      "Appending .git is not necessary"),
-    (re.compile(r'(.*/blob/master/|.*raw\.github.com/[^/]*/[^/]*/master/)'),
-     "Use /HEAD/ instead of /master/ to point at a file in the default branch"),
-    # TODO enable in August 2015, when Google Code goes read-only
-    # (re.compile(r'.*://code\.google\.com/.*'),
-    #  "code.google.com will be soon switching down, perhaps the project moved to github.com?"),
+    (re.compile(r'.*://[^/]*(github|gitlab|bitbucket|rawgit)[^/]*/([^/]+/){1,3}master'),
+     "Use /HEAD instead of /master to point at a file in the default branch"),
 ]
 
-regex_warnings = {
-    'Web Site': http_warnings + [
-    ],
-    'Source Code': http_warnings + [
-    ],
-    'Repo': https_enforcings + [
-    ],
-    'Issue Tracker': http_warnings + [
+regex_checks = {
+    'Web Site': http_checks,
+    'Source Code': http_checks,
+    'Repo': https_enforcings,
+    'Issue Tracker': http_checks + [
         (re.compile(r'.*github\.com/[^/]+/[^/]+[/]*$'),
          "/issues is missing"),
     ],
-    'Donate': http_warnings + [
+    'Donate': http_checks + [
         (re.compile(r'.*flattr\.com'),
          "Flattr donation methods belong in the FlattrID flag"),
     ],
-    'Changelog': http_warnings + [
+    'Changelog': http_checks,
+    'Author Name': [
+        (re.compile(r'^\s'),
+         "Unnecessary leading space"),
+        (re.compile(r'.*\s$'),
+         "Unnecessary trailing space"),
     ],
     'License': [
         (re.compile(r'^(|None|Unknown)$'),
@@ -95,6 +93,10 @@ regex_warnings = {
          "No need to specify that the app is for Android"),
         (re.compile(r'.*[a-z0-9][.!?]( |$)'),
          "Punctuation should be avoided"),
+        (re.compile(r'^\s'),
+         "Unnecessary leading space"),
+        (re.compile(r'.*\s$'),
+         "Unnecessary trailing space"),
     ],
     'Description': [
         (re.compile(r'^No description available$'),
@@ -105,204 +107,262 @@ regex_warnings = {
          "Unnecessary leading space"),
         (re.compile(r'.*\s$'),
          "Unnecessary trailing space"),
+        (re.compile(r'.*([^[]|^)\[[^:[\]]+( |\]|$)'),
+         "Invalid link - use [http://foo.bar Link title] or [http://foo.bar]"),
+        (re.compile(r'(^|.* )https?://[^ ]+'),
+         "Unlinkified link - use [http://foo.bar Link title] or [http://foo.bar]"),
     ],
 }
 
-categories = Set([
-    "Children",
+
+def check_regexes(app):
+    for f, checks in regex_checks.iteritems():
+        for m, r in checks:
+            v = app.get_field(f)
+            t = metadata.fieldtype(f)
+            if t == metadata.TYPE_MULTILINE:
+                for l in v.splitlines():
+                    if m.match(l):
+                        yield "%s at line '%s': %s" % (f, l, r)
+            else:
+                if v is None:
+                    continue
+                if m.match(v):
+                    yield "%s '%s': %s" % (f, v, r)
+
+
+def get_lastbuild(builds):
+    lowest_vercode = -1
+    lastbuild = None
+    for build in builds:
+        if not build.disable:
+            vercode = int(build.vercode)
+            if lowest_vercode == -1 or vercode < lowest_vercode:
+                lowest_vercode = vercode
+        if not lastbuild or int(build.vercode) > int(lastbuild.vercode):
+            lastbuild = build
+    return lastbuild
+
+
+def check_ucm_tags(app):
+    lastbuild = get_lastbuild(app.builds)
+    if (lastbuild is not None
+            and lastbuild.commit
+            and app.UpdateCheckMode == 'RepoManifest'
+            and not lastbuild.commit.startswith('unknown')
+            and lastbuild.vercode == app.CurrentVersionCode
+            and not lastbuild.forcevercode
+            and any(s in lastbuild.commit for s in '.,_-/')):
+        yield "Last used commit '%s' looks like a tag, but Update Check Mode is '%s'" % (
+            lastbuild.commit, app.UpdateCheckMode)
+
+
+def check_char_limits(app):
+    limits = config['char_limits']
+
+    if len(app.Summary) > limits['Summary']:
+        yield "Summary of length %s is over the %i char limit" % (
+            len(app.Summary), limits['Summary'])
+
+    if len(app.Description) > limits['Description']:
+        yield "Description of length %s is over the %i char limit" % (
+            len(app.Description), limits['Description'])
+
+
+def check_old_links(app):
+    usual_sites = [
+        'github.com',
+        'gitlab.com',
+        'bitbucket.org',
+    ]
+    old_sites = [
+        'gitorious.org',
+        'code.google.com',
+    ]
+    if any(s in app.Repo for s in usual_sites):
+        for f in ['Web Site', 'Source Code', 'Issue Tracker', 'Changelog']:
+            v = app.get_field(f)
+            if any(s in v for s in old_sites):
+                yield "App is in '%s' but has a link to '%s'" % (app.Repo, v)
+
+
+def check_useless_fields(app):
+    if app.UpdateCheckName == app.id:
+        yield "Update Check Name is set to the known app id - it can be removed"
+
+filling_ucms = re.compile(r'^(Tags.*|RepoManifest.*)')
+
+
+def check_checkupdates_ran(app):
+    if filling_ucms.match(app.UpdateCheckMode):
+        if not app.AutoName and not app.CurrentVersion and app.CurrentVersionCode == '0':
+            yield "UCM is set but it looks like checkupdates hasn't been run yet"
+
+
+def check_empty_fields(app):
+    if not app.Categories:
+        yield "Categories are not set"
+
+all_categories = Set([
+    "Connectivity",
     "Development",
     "Games",
+    "Graphics",
     "Internet",
+    "Money",
     "Multimedia",
     "Navigation",
-    "Office",
     "Phone & SMS",
     "Reading",
     "Science & Education",
     "Security",
+    "Sports & Health",
     "System",
-    "Wallpaper",
+    "Theming",
+    "Time",
+    "Writing",
 ])
 
-desc_url = re.compile("[^[]\[([^ ]+)( |\]|$)")
 
+def check_categories(app):
+    for categ in app.Categories:
+        if categ not in all_categories:
+            yield "Category '%s' is not valid" % categ
 
-def main():
 
-    global config, options, curid, count
-    curid = None
+def check_duplicates(app):
+    if app.Name and app.Name == app.AutoName:
+        yield "Name '%s' is just the auto name - remove it" % app.Name
 
-    count = Counter()
+    links_seen = set()
+    for f in ['Source Code', 'Web Site', 'Issue Tracker', 'Changelog']:
+        v = app.get_field(f)
+        if not v:
+            continue
+        v = v.lower()
+        if v in links_seen:
+            yield "Duplicate link in '%s': %s" % (f, v)
+        else:
+            links_seen.add(v)
+
+    name = app.Name or app.AutoName
+    if app.Summary and name:
+        if app.Summary.lower() == name.lower():
+            yield "Summary '%s' is just the app's name" % app.Summary
+
+    if app.Summary and app.Description and len(app.Description) == 1:
+        if app.Summary.lower() == app.Description[0].lower():
+            yield "Description '%s' is just the app's summary" % app.Summary
+
+    seenlines = set()
+    for l in app.Description.splitlines():
+        if len(l) < 1:
+            continue
+        if l in seenlines:
+            yield "Description has a duplicate line"
+        seenlines.add(l)
+
+
+desc_url = re.compile(r'(^|[^[])\[([^ ]+)( |\]|$)')
+
+
+def check_mediawiki_links(app):
+    wholedesc = ' '.join(app.Description)
+    for um in desc_url.finditer(wholedesc):
+        url = um.group(1)
+        for m, r in http_checks:
+            if m.match(url):
+                yield "URL '%s' in Description: %s" % (url, r)
 
-    def warn(message):
-        global curid, count
-        if curid:
-            print "%s:" % curid
-            curid = None
-            count['app'] += 1
-        print '    %s' % message
-        count['warn'] += 1
+
+def check_bulleted_lists(app):
+    validchars = ['*', '#']
+    lchar = ''
+    lcount = 0
+    for l in app.Description.splitlines():
+        if len(l) < 1:
+            lcount = 0
+            continue
+
+        if l[0] == lchar and l[1] == ' ':
+            lcount += 1
+            if lcount > 2 and lchar not in validchars:
+                yield "Description has a list (%s) but it isn't bulleted (*) nor numbered (#)" % lchar
+                break
+        else:
+            lchar = l[0]
+            lcount = 1
+
+
+def check_builds(app):
+    for build in app.builds:
+        if build.disable:
+            continue
+        for s in ['master', 'origin', 'HEAD', 'default', 'trunk']:
+            if build.commit and build.commit.startswith(s):
+                yield "Branch '%s' used as commit in build '%s'" % (s, build.version)
+            for srclib in build.srclibs:
+                ref = srclib.split('@')[1].split('/')[0]
+                if ref.startswith(s):
+                    yield "Branch '%s' used as commit in srclib '%s'" % (s, srclib)
+        if build.target and build.build_method() == 'gradle':
+            yield "target= has no gradle support"
+
+
+def main():
+
+    global config, options
+
+    anywarns = False
 
     # Parse command line...
-    parser = OptionParser(usage="Usage: %prog [options] [APPID [APPID ...]]")
-    parser.add_option("-v", "--verbose", action="store_true", default=False,
-                      help="Spew out even more information than normal")
-    parser.add_option("-q", "--quiet", action="store_true", default=False,
-                      help="Restrict output to warnings and errors")
-    (options, args) = parser.parse_args()
+    parser = ArgumentParser(usage="%(prog)s [options] [APPID [APPID ...]]")
+    common.setup_global_opts(parser)
+    parser.add_argument("-f", "--format", action="store_true", default=False,
+                        help="Also warn about formatting issues, like rewritemeta -l")
+    parser.add_argument("appid", nargs='*', help="app-id in the form APPID")
+    options = parser.parse_args()
 
     config = common.read_config(options)
 
     # Get all apps...
-    allapps = metadata.read_metadata(xref=False)
-    apps = common.read_app_args(args, allapps, False)
-
-    filling_ucms = re.compile('^(Tags.*|RepoManifest.*)')
+    allapps = metadata.read_metadata(xref=True)
+    apps = common.read_app_args(options.appid, allapps, False)
 
     for appid, app in apps.iteritems():
-        if app['Disabled']:
+        if app.Disabled:
             continue
 
-        curid = appid
-        count['app_total'] += 1
-
-        # enabled_builds = 0
-        lowest_vercode = -1
-        curbuild = None
-        for build in app['builds']:
-            if not build['disable']:
-                # enabled_builds += 1
-                vercode = int(build['vercode'])
-                if lowest_vercode == -1 or vercode < lowest_vercode:
-                    lowest_vercode = vercode
-            if not curbuild or int(build['vercode']) > int(curbuild['vercode']):
-                curbuild = build
-
-        # Incorrect UCM
-        if (curbuild and curbuild['commit']
-                and app['Update Check Mode'] == 'RepoManifest'
-                and not curbuild['commit'].startswith('unknown')
-                and curbuild['vercode'] == app['Current Version Code']
-                and not curbuild['forcevercode']
-                and any(s in curbuild['commit'] for s in '.,_-/')):
-            warn("Last used commit '%s' looks like a tag, but Update Check Mode is '%s'" % (
-                curbuild['commit'], app['Update Check Mode']))
-
-        # Summary size limit
-        summ_chars = len(app['Summary'])
-        if summ_chars > config['char_limits']['Summary']:
-            warn("Summary of length %s is over the %i char limit" % (
-                summ_chars, config['char_limits']['Summary']))
-
-        # Redundant info
-        if app['Web Site'] and app['Source Code']:
-            if app['Web Site'].lower() == app['Source Code'].lower():
-                warn("Website '%s' is just the app's source code link" % app['Web Site'])
-
-        if filling_ucms.match(app['Update Check Mode']):
-            if all(app[f] == metadata.app_defaults[f] for f in [
-                    'Auto Name',
-                    'Current Version',
-                    'Current Version Code',
-                    ]):
-                warn("UCM is set but it looks like checkupdates hasn't been run yet")
-
-        if app['Update Check Name'] == appid:
-            warn("Update Check Name is set to the known app id - it can be removed")
-
-        cvc = int(app['Current Version Code'])
-        if cvc > 0 and cvc < lowest_vercode:
-            warn("Current Version Code is lower than any enabled build")
-
-        # Missing or incorrect categories
-        if not app['Categories']:
-            warn("Categories are not set")
-        for categ in app['Categories']:
-            if categ not in categories:
-                warn("Category '%s' is not valid" % categ)
-
-        if app['Name'] and app['Name'] == app['Auto Name']:
-            warn("Name '%s' is just the auto name" % app['Name'])
-
-        name = app['Name'] or app['Auto Name']
-        if app['Summary'] and name:
-            if app['Summary'].lower() == name.lower():
-                warn("Summary '%s' is just the app's name" % app['Summary'])
-
-        desc = app['Description']
-        if app['Summary'] and desc and len(desc) == 1:
-            if app['Summary'].lower() == desc[0].lower():
-                warn("Description '%s' is just the app's summary" % app['Summary'])
-
-        # Description size limit
-        desc_charcount = sum(len(l) for l in desc)
-        if desc_charcount > config['char_limits']['Description']:
-            warn("Description of length %s is over the %i char limit" % (
-                desc_charcount, config['char_limits']['Description']))
-
-        if (not desc[0] or not desc[-1]
-                or any(not desc[l - 1] and not desc[l] for l in range(1, len(desc)))):
-            warn("Description has an extra empty line")
-
-        # Check for lists using the wrong characters
-        validchars = ['*', '#']
-        lchar = ''
-        lcount = 0
-        for l in app['Description']:
-            if len(l) < 1:
-                continue
-
-            for um in desc_url.finditer(l):
-                url = um.group(1)
-                for m, r in http_warnings:
-                    if m.match(url):
-                        warn("URL '%s' in Description: %s" % (url, r))
-
-            c = l.decode('utf-8')[0]
-            if c == lchar:
-                lcount += 1
-                if lcount > 3 and lchar not in validchars:
-                    warn("Description has a list (%s) but it isn't bulleted (*) nor numbered (#)" % lchar)
-                    break
-            else:
-                lchar = c
-                lcount = 1
-
-        # Regex checks in all kinds of fields
-        for f in regex_warnings:
-            for m, r in regex_warnings[f]:
-                v = app[f]
-                if type(v) == str:
-                    if v is None:
-                        continue
-                    if m.match(v):
-                        warn("%s '%s': %s" % (f, v, r))
-                elif type(v) == list:
-                    for l in v:
-                        if m.match(l):
-                            warn("%s at line '%s': %s" % (f, l, r))
-
-        # Build warnings
-        for build in app['builds']:
-            if build['disable']:
-                continue
-            for s in ['master', 'origin', 'HEAD', 'default', 'trunk']:
-                if build['commit'] and build['commit'].startswith(s):
-                    warn("Branch '%s' used as commit in build '%s'" % (
-                        s, build['version']))
-                for srclib in build['srclibs']:
-                    ref = srclib.split('@')[1].split('/')[0]
-                    if ref.startswith(s):
-                        warn("Branch '%s' used as commit in srclib '%s'" % (
-                            s, srclib))
-
-        if not curid:
-            print
-
-    logging.info("Found a total of %i warnings in %i apps out of %i total." % (
-        count['warn'], count['app'], count['app_total']))
-
-    sys.exit(1 if count['warn'] > 0 else 0)
+        warns = []
+
+        for check_func in [
+                check_regexes,
+                check_ucm_tags,
+                check_char_limits,
+                check_old_links,
+                check_checkupdates_ran,
+                check_useless_fields,
+                check_empty_fields,
+                check_categories,
+                check_duplicates,
+                check_mediawiki_links,
+                check_bulleted_lists,
+                check_builds,
+                ]:
+            warns += check_func(app)
+
+        if options.format:
+            if not rewritemeta.proper_format(app):
+                warns.append("Run rewritemeta to fix formatting")
+
+        if warns:
+            anywarns = True
+            for warn in warns:
+                print("%s: %s" % (appid, warn))
+
+    if anywarns:
+        sys.exit(1)
+
 
 if __name__ == "__main__":
     main()