From 6a9877ac02154d2ff122532f9ec392eccbfe4040 Mon Sep 17 00:00:00 2001 From: Jan Keromnes Date: Fri, 13 Jul 2018 10:25:30 +0200 Subject: [PATCH] shipit_static_analysis: Dump analyzers raw outputs as Taskcluster artifacts for debugging. #1257 --- .../shipit_static_analysis/clang/format.py | 12 ++++++++-- .../shipit_static_analysis/clang/tidy.py | 12 +++++++++- .../shipit_static_analysis/config.py | 9 ++++++++ .../shipit_static_analysis/lint.py | 14 ++++++++++-- .../shipit_static_analysis/revisions.py | 22 +++++++++---------- .../shipit_static_analysis/workflow.py | 11 +++------- .../tests/test_clang.py | 16 ++++++++++++++ .../tests/test_revisions.py | 4 ++-- 8 files changed, 73 insertions(+), 27 deletions(-) diff --git a/src/shipit_static_analysis/shipit_static_analysis/clang/format.py b/src/shipit_static_analysis/shipit_static_analysis/clang/format.py index 10eb740973..bca36c06cb 100644 --- a/src/shipit_static_analysis/shipit_static_analysis/clang/format.py +++ b/src/shipit_static_analysis/shipit_static_analysis/clang/format.py @@ -94,11 +94,19 @@ def run_clang_format(self, filename, revision): logger.info('Running clang-format', cmd=' '.join(cmd)) # Run command - clang_output = subprocess.check_output(cmd, cwd=settings.repo_dir) + clang_output = subprocess.check_output(cmd, cwd=settings.repo_dir).decode('utf-8') + + # Dump raw clang-format output as a Taskcluster artifact (for debugging) + clang_output_path = os.path.join( + settings.taskcluster_results_dir, + '{}-clang-format.txt'.format(repr(revision)), + ) + with open(clang_output_path, 'w') as f: + f.write(clang_output) # Compare output with original file src_lines = [x.rstrip('\n') for x in open(full_path).readlines()] - clang_lines = clang_output.decode('utf-8').split('\n') + clang_lines = clang_output.split('\n') # Build issues from diff of diff ! diff = difflib.SequenceMatcher( diff --git a/src/shipit_static_analysis/shipit_static_analysis/clang/tidy.py b/src/shipit_static_analysis/shipit_static_analysis/clang/tidy.py index 98e789b413..2b588f901e 100755 --- a/src/shipit_static_analysis/shipit_static_analysis/clang/tidy.py +++ b/src/shipit_static_analysis/shipit_static_analysis/clang/tidy.py @@ -111,7 +111,17 @@ def run(self, revision): logger.error('Mach static analysis failed: {}'.format(e.output)) raise - issues = self.parse_issues(clang_output.decode('utf-8'), revision) + clang_output = clang_output.decode('utf-8') + + # Dump raw clang-tidy output as a Taskcluster artifact (for debugging) + clang_output_path = os.path.join( + settings.taskcluster_results_dir, + '{}-clang-tidy.txt'.format(repr(revision)), + ) + with open(clang_output_path, 'w') as f: + f.write(clang_output) + + issues = self.parse_issues(clang_output, revision) # Report stats for these issues stats.report_issues('clang-tidy', issues) diff --git a/src/shipit_static_analysis/shipit_static_analysis/config.py b/src/shipit_static_analysis/shipit_static_analysis/config.py index 31fda399f3..02e6e142f9 100644 --- a/src/shipit_static_analysis/shipit_static_analysis/config.py +++ b/src/shipit_static_analysis/shipit_static_analysis/config.py @@ -7,6 +7,7 @@ import enum import os +import tempfile import requests import yaml @@ -43,6 +44,7 @@ def __init__(self): self.cache_root = None self.repo_dir = None self.repo_shared_dir = None + self.taskcluster_results_dir = None def setup(self, app_channel, cache_root, publication): self.app_channel = app_channel @@ -63,6 +65,13 @@ def setup(self, app_channel, cache_root, publication): self.repo_dir = os.path.join(self.cache_root, 'sa-central') self.repo_shared_dir = os.path.join(self.cache_root, 'sa-central-shared') + if 'TASK_ID' in os.environ and 'RUN_ID' in os.environ: + self.taskcluster_results_dir = '/tmp/results' + else: + self.taskcluster_results_dir = tempfile.mkdtemp() + if not os.path.isdir(self.taskcluster_results_dir): + os.makedirs(self.taskcluster_results_dir) + def __getattr__(self, key): if key not in self.config: raise AttributeError diff --git a/src/shipit_static_analysis/shipit_static_analysis/lint.py b/src/shipit_static_analysis/shipit_static_analysis/lint.py index 96179142b6..0129ffe90f 100644 --- a/src/shipit_static_analysis/shipit_static_analysis/lint.py +++ b/src/shipit_static_analysis/shipit_static_analysis/lint.py @@ -192,17 +192,27 @@ def find_issues(self, path, revision): path ] returncode, output, error = run(' '.join(command), cwd=settings.repo_dir) + output = output.decode('utf-8') + + # Dump raw mozlint output as a Taskcluster artifact (for debugging) + output_path = os.path.join( + settings.taskcluster_results_dir, + '{}-mozlint.txt'.format(repr(revision)), + ) + with open(output_path, 'a') as f: + f.write(output) + if returncode == 0: logger.debug('No Mozlint errors', path=path) return - assert 'error: problem with lint setup' not in output.decode('utf-8'), \ + assert 'error: problem with lint setup' not in output, \ 'Mach lint setup failed' # Load output as json # Only consider last line, as ./mach lint may output # linter setup output on stdout :/ try: - lines = list(filter(None, output.decode('utf-8').split('\n'))) + lines = list(filter(None, output.split('\n'))) payload = json.loads(lines[-1]) except json.decoder.JSONDecodeError: logger.warn('Invalid json output', path=path, lines=lines) diff --git a/src/shipit_static_analysis/shipit_static_analysis/revisions.py b/src/shipit_static_analysis/shipit_static_analysis/revisions.py index d91f868682..7355f91c34 100644 --- a/src/shipit_static_analysis/shipit_static_analysis/revisions.py +++ b/src/shipit_static_analysis/shipit_static_analysis/revisions.py @@ -116,14 +116,12 @@ def namespaces(self): 'phabricator.diffphid.{}'.format(self.diff_phid), ] + def __repr__(self): + return self.diff_phid + def __str__(self): return 'Phabricator #{} - {}'.format(self.diff_id, self.diff_phid) - def build_diff_name(self): - return '{}-clang-format.diff'.format( - self.diff_phid, - ) - @property def url(self): return 'https://{}/D{}'.format(self.api.hostname, self.id) @@ -172,6 +170,13 @@ def __init__(self, review_request_id, mercurial, diffset_revision): self.review_request_id = int(review_request_id) self.diffset_revision = int(diffset_revision) + def __repr__(self): + return '{}-{}-{}'.format( + self.mercurial[:8], + self.review_request_id, + self.diffset_revision, + ) + def __str__(self): return 'MozReview #{} - {}'.format(self.review_request_id, self.diffset_revision) @@ -187,13 +192,6 @@ def namespaces(self): 'mozreview.rev.{}'.format(self.mercurial), ] - def build_diff_name(self): - return '{}-{}-{}-clang-format.diff'.format( - self.mercurial[:8], - self.review_request_id, - self.diffset_revision, - ) - def load(self, repo): ''' Load required revision from mercurial remote repo diff --git a/src/shipit_static_analysis/shipit_static_analysis/workflow.py b/src/shipit_static_analysis/shipit_static_analysis/workflow.py index bd10bf90e2..694bfc02bc 100644 --- a/src/shipit_static_analysis/shipit_static_analysis/workflow.py +++ b/src/shipit_static_analysis/shipit_static_analysis/workflow.py @@ -8,7 +8,6 @@ import itertools import os import subprocess -import tempfile from datetime import datetime from datetime import timedelta @@ -54,15 +53,11 @@ def __init__(self, reporters, analyzers, index_service): if 'TASK_ID' in os.environ and 'RUN_ID' in os.environ: self.taskcluster_task_id = os.environ['TASK_ID'] self.taskcluster_run_id = os.environ['RUN_ID'] - self.taskcluster_results_dir = '/tmp/results' self.on_taskcluster = True else: self.taskcluster_task_id = 'local instance' self.taskcluster_run_id = 0 - self.taskcluster_results_dir = tempfile.mkdtemp() self.on_taskcluster = False - if not os.path.isdir(self.taskcluster_results_dir): - os.makedirs(self.taskcluster_results_dir) # Load reporters to use self.reporters = reporters @@ -70,7 +65,7 @@ def __init__(self, reporters, analyzers, index_service): logger.warn('No reporters configured, this analysis will not be published') # Always add debug reporter and Diff reporter - self.reporters['debug'] = DebugReporter(output_dir=self.taskcluster_results_dir) + self.reporters['debug'] = DebugReporter(output_dir=settings.taskcluster_results_dir) # Use TC index service client self.index_service = index_service @@ -295,8 +290,8 @@ def build_improvement_patch(self, revision, issues): return # Write diff in results directory - diff_name = revision.build_diff_name() - diff_path = os.path.join(self.taskcluster_results_dir, diff_name) + diff_name = '{}-clang-format.diff'.format(repr(revision)) + diff_path = os.path.join(settings.taskcluster_results_dir, diff_name) with open(diff_path, 'w') as f: length = f.write(diff.decode('utf-8')) logger.info('Improvement diff dumped', path=diff_path, length=length) diff --git a/src/shipit_static_analysis/tests/test_clang.py b/src/shipit_static_analysis/tests/test_clang.py index f564306ad2..b0f976e608 100644 --- a/src/shipit_static_analysis/tests/test_clang.py +++ b/src/shipit_static_analysis/tests/test_clang.py @@ -70,6 +70,7 @@ def test_clang_format(mock_config, mock_repository, mock_stats, mock_clang, mock Test clang-format runner ''' from shipit_static_analysis.clang.format import ClangFormat, ClangFormatIssue + from shipit_static_analysis.config import settings # Write badly formatted c file bad_file = os.path.join(mock_config.repo_dir, 'bad.cpp') @@ -100,6 +101,13 @@ def test_clang_format(mock_config, mock_repository, mock_stats, mock_clang, mock mock_workflow.build_improvement_patch(mock_revision, issues) assert open(bad_file).read() == BAD_CPP_VALID + # Ensure the raw output dump exists + clang_output_path = os.path.join( + settings.taskcluster_results_dir, + '{}-clang-format.txt'.format(repr(mock_revision)), + ) + assert os.path.isfile(clang_output_path) + # Test stats mock_stats.flush() metrics = mock_stats.get_metrics('issues.clang-format') @@ -120,6 +128,7 @@ def test_clang_tidy(mock_repository, mock_config, mock_clang, mock_stats, mock_r Test clang-tidy runner ''' from shipit_static_analysis.clang.tidy import ClangTidy, ClangTidyIssue + from shipit_static_analysis.config import settings # Init clang tidy runner ct = ClangTidy() @@ -143,6 +152,13 @@ def test_clang_tidy(mock_repository, mock_config, mock_clang, mock_stats, mock_r assert issues[1].check == 'modernize-use-nullptr' assert issues[1].line == 8 + # Ensure the raw output dump exists + clang_output_path = os.path.join( + settings.taskcluster_results_dir, + '{}-clang-tidy.txt'.format(repr(mock_revision)), + ) + assert os.path.isfile(clang_output_path) + # Test stats mock_stats.flush() metrics = mock_stats.get_metrics('issues.clang-tidy') diff --git a/src/shipit_static_analysis/tests/test_revisions.py b/src/shipit_static_analysis/tests/test_revisions.py index cad9861061..a81ebb697c 100644 --- a/src/shipit_static_analysis/tests/test_revisions.py +++ b/src/shipit_static_analysis/tests/test_revisions.py @@ -19,7 +19,7 @@ def test_mozreview(): assert r.review_request_id == 164530 assert r.diffset_revision == 7 assert r.url == 'https://reviewboard.mozilla.org/r/164530/' - assert r.build_diff_name() == '308c22e7-164530-7-clang-format.diff' + assert repr(r) == '308c22e7-164530-7' @responses.activate @@ -40,7 +40,7 @@ def test_phabricator(mock_phabricator, mock_repository, mock_config): assert r.diff_id == 42 assert r.diff_phid == 'PHID-DIFF-testABcd12' assert r.url == 'https://phabricator.test/D51' - assert r.build_diff_name() == 'PHID-DIFF-testABcd12-clang-format.diff' + assert repr(r) == 'PHID-DIFF-testABcd12' assert r.id == 51 # revision # Check test.txt content