Merge pull request #3177 from migueldiascosta/review_pr_labels

advise PR labels in --review-pr and add support for --add-pr-labels

Author: boegel

easybuild/framework/easyconfig/tools.py

@@ -47,15 +47,18 @@
 from easybuild.base import fancylogger
 from easybuild.framework.easyconfig import EASYCONFIGS_PKG_SUBDIR
 from easybuild.framework.easyconfig.easyconfig import EASYCONFIGS_ARCHIVE_DIR, ActiveMNS, EasyConfig
-from easybuild.framework.easyconfig.easyconfig import create_paths, get_easyblock_class, process_easyconfig
+from easybuild.framework.easyconfig.easyconfig import create_paths, det_file_info, get_easyblock_class
+from easybuild.framework.easyconfig.easyconfig import process_easyconfig
 from easybuild.framework.easyconfig.format.yeb import quote_yaml_special_chars
 from easybuild.framework.easyconfig.style import cmdline_easyconfigs_style_check
 from easybuild.tools.build_log import EasyBuildError, print_msg, print_warning
 from easybuild.tools.config import build_option
 from easybuild.tools.environment import restore_env
 from easybuild.tools.filetools import find_easyconfigs, is_patch_file, locate_files
 from easybuild.tools.filetools import read_file, resolve_path, which, write_file
-from easybuild.tools.github import fetch_easyconfigs_from_pr, fetch_files_from_pr, download_repo
+from easybuild.tools.github import GITHUB_EASYCONFIGS_REPO
+from easybuild.tools.github import det_pr_labels, download_repo, fetch_easyconfigs_from_pr, fetch_pr_data
+from easybuild.tools.github import fetch_files_from_pr
 from easybuild.tools.multidiff import multidiff
 from easybuild.tools.py2vs3 import OrderedDict
 from easybuild.tools.toolchain.toolchain import is_system_toolchain
@@ -474,14 +477,19 @@ def find_related_easyconfigs(path, ec):
     return sorted(res)
 
 
-def review_pr(paths=None, pr=None, colored=True, branch='develop'):
+def review_pr(paths=None, pr=None, colored=True, branch='develop', testing=False):
     """
     Print multi-diff overview between specified easyconfigs or PR and specified branch.
     :param pr: pull request number in easybuild-easyconfigs repo to review
     :param paths: path tuples (path, generated) of easyconfigs to review
     :param colored: boolean indicating whether a colored multi-diff should be generated
     :param branch: easybuild-easyconfigs branch to compare with
+    :param testing: whether to ignore PR labels (used in test_review_pr)
     """
+    pr_target_repo = build_option('pr_target_repo') or GITHUB_EASYCONFIGS_REPO
+    if pr_target_repo != GITHUB_EASYCONFIGS_REPO:
+        raise EasyBuildError("Reviewing PRs for repositories other than easyconfigs hasn't been implemented yet")
+
     tmpdir = tempfile.mkdtemp()
 
     download_repo_path = download_repo(branch=branch, path=tmpdir)
@@ -508,6 +516,20 @@ def review_pr(paths=None, pr=None, colored=True, branch='develop'):
         else:
             lines.extend(['', "(no related easyconfigs found for %s)\n" % os.path.basename(ec['spec'])])
 
+    if pr:
+        file_info = det_file_info(pr_files, download_repo_path)
+
+        pr_target_account = build_option('pr_target_account')
+        github_user = build_option('github_user')
+        pr_data, _ = fetch_pr_data(pr, pr_target_account, pr_target_repo, github_user)
+        pr_labels = [label['name'] for label in pr_data['labels']] if not testing else []
+
+        expected_labels = det_pr_labels(file_info, pr_target_repo)
+        missing_labels = [label for label in expected_labels if label not in pr_labels]
+
+        if missing_labels:
+            lines.extend(['', "This PR should be labelled with %s" % ', '.join(["'%s'" % ml for ml in missing_labels])])
+
     return '\n'.join(lines)
 
 

easybuild/main.py

@@ -61,7 +61,7 @@
 from easybuild.tools.filetools import adjust_permissions, cleanup, copy_files, dump_index, load_index
 from easybuild.tools.filetools import locate_files, read_file, register_lock_cleanup_signal_handlers, write_file
 from easybuild.tools.github import check_github, close_pr, find_easybuild_easyconfig
-from easybuild.tools.github import install_github_token, list_prs, merge_pr, new_branch_github, new_pr
+from easybuild.tools.github import add_pr_labels, install_github_token, list_prs, merge_pr, new_branch_github, new_pr
 from easybuild.tools.github import new_pr_from_branch
 from easybuild.tools.github import sync_branch_with_develop, sync_pr_with_develop, update_branch, update_pr
 from easybuild.tools.hooks import START, END, load_hooks, run_hook
@@ -260,7 +260,10 @@ def main(args=None, logfile=None, do_build=None, testing=False, modtool=None):
         merge_pr(options.merge_pr)
 
     elif options.review_pr:
-        print(review_pr(pr=options.review_pr, colored=use_color(options.color)))
+        print(review_pr(pr=options.review_pr, colored=use_color(options.color), testing=testing))
+
+    elif options.add_pr_labels:
+        add_pr_labels(options.add_pr_labels)
 
     elif options.list_installed_software:
         detailed = options.list_installed_software == 'detailed'
@@ -277,6 +280,7 @@ def main(args=None, logfile=None, do_build=None, testing=False, modtool=None):
 
     # non-verbose cleanup after handling GitHub integration stuff or printing terse info
     early_stop_options = [
+        options.add_pr_labels,
         options.check_github,
         options.create_index,
         options.install_github_token,

easybuild/tools/github.py

@@ -1359,6 +1359,95 @@ def merge_url(gh):
         print_warning("Review indicates this PR should not be merged (use -f/--force to do so anyway)")
 
 
+def det_pr_labels(file_info, pr_target_repo):
+    """
+    Determine labels for a pull request based on provided information on files changed by that pull request.
+    """
+    labels = []
+    if pr_target_repo == GITHUB_EASYCONFIGS_REPO:
+        if any(file_info['new_folder']):
+            labels.append('new')
+        if any(file_info['new_file_in_existing_folder']):
+            labels.append('update')
+    elif pr_target_repo == GITHUB_EASYBLOCKS_REPO:
+        if any(file_info['new']):
+            labels.append('new')
+    return labels
+
+
+def post_pr_labels(pr, labels):
+    """
+    Update PR labels
+    """
+    pr_target_account = build_option('pr_target_account')
+    pr_target_repo = build_option('pr_target_repo') or GITHUB_EASYCONFIGS_REPO
+
+    # fetch GitHub token if available
+    github_user = build_option('github_user')
+    if github_user is None:
+        _log.info("GitHub user not specified, not adding labels to PR# %s" % pr)
+        return False
+
+    github_token = fetch_github_token(github_user)
+    if github_token is None:
+        _log.info("GitHub token for user '%s' not found, not adding labels to PR# %s" % (github_user, pr))
+        return False
+
+    dry_run = build_option('dry_run') or build_option('extended_dry_run')
+
+    if not dry_run:
+        g = RestClient(GITHUB_API_URL, username=github_user, token=github_token)
+
+        pr_url = g.repos[pr_target_account][pr_target_repo].issues[pr]
+        try:
+            status, data = pr_url.labels.post(body=labels)
+            if status == HTTP_STATUS_OK:
+                print_msg("Added labels %s to PR#%s" % (', '.join(labels), pr), log=_log, prefix=False)
+                return True
+        except HTTPError as err:
+            _log.info("Failed to add labels to PR# %s: %s." % (pr, err))
+            return False
+    else:
+        return True
+
+
+def add_pr_labels(pr, branch='develop'):
+    """
+    Try to determine and add labels to PR.
+    :param pr: pull request number in easybuild-easyconfigs repo
+    :param branch: easybuild-easyconfigs branch to compare with
+    """
+    pr_target_repo = build_option('pr_target_repo') or GITHUB_EASYCONFIGS_REPO
+    if pr_target_repo != GITHUB_EASYCONFIGS_REPO:
+        raise EasyBuildError("Adding labels to PRs for repositories other than easyconfigs hasn't been implemented yet")
+
+    tmpdir = tempfile.mkdtemp()
+
+    download_repo_path = download_repo(branch=branch, path=tmpdir)
+
+    pr_files = [path for path in fetch_easyconfigs_from_pr(pr) if path.endswith('.eb')]
+
+    file_info = det_file_info(pr_files, download_repo_path)
+
+    pr_target_account = build_option('pr_target_account')
+    github_user = build_option('github_user')
+    pr_data, _ = fetch_pr_data(pr, pr_target_account, pr_target_repo, github_user)
+    pr_labels = [label['name'] for label in pr_data['labels']]
+
+    expected_labels = det_pr_labels(file_info, pr_target_repo)
+    missing_labels = [label for label in expected_labels if label not in pr_labels]
+
+    dry_run = build_option('dry_run') or build_option('extended_dry_run')
+
+    if missing_labels:
+        missing_labels_txt = ', '.join(["'%s'" % ml for ml in missing_labels])
+        print_msg("PR #%s should be labelled %s" % (pr, missing_labels_txt), log=_log, prefix=False)
+        if not dry_run and not post_pr_labels(pr, missing_labels):
+            print_msg("Could not add labels %s to PR #%s" % (missing_labels_txt, pr), log=_log, prefix=False)
+    else:
+        print_msg("Could not determine any missing labels for PR #%s" % pr, log=_log, prefix=False)
+
+
 @only_if_module_is_available('git', pkgname='GitPython')
 def new_branch_github(paths, ecs, commit_msg=None):
     """
@@ -1486,14 +1575,9 @@ def new_pr_from_branch(branch_name, title=None, descr=None, pr_target_repo=None,
 
         file_info = det_file_info(ec_paths, target_dir)
 
-    labels = []
-    if pr_target_repo == GITHUB_EASYCONFIGS_REPO:
-        # label easyconfigs for new software and/or new easyconfigs for existing software
-        if any(file_info['new_folder']):
-            labels.append('new')
-        if any(file_info['new_file_in_existing_folder']):
-            labels.append('update')
+    labels = det_pr_labels(file_info, pr_target_repo)
 
+    if pr_target_repo == GITHUB_EASYCONFIGS_REPO:
         # only use most common toolchain(s) in toolchain label of PR title
         toolchains = ['%(name)s/%(version)s' % ec['toolchain'] for ec in file_info['ecs']]
         toolchains_counted = sorted([(toolchains.count(tc), tc) for tc in nub(toolchains)])
@@ -1503,9 +1587,6 @@ def new_pr_from_branch(branch_name, title=None, descr=None, pr_target_repo=None,
         classes = [ec['moduleclass'] for ec in file_info['ecs']]
         classes_counted = sorted([(classes.count(c), c) for c in nub(classes)])
         class_label = ','.join([tc for (cnt, tc) in classes_counted if cnt == classes_counted[-1][0]])
-    elif pr_target_repo == GITHUB_EASYBLOCKS_REPO:
-        if any(file_info['new']):
-            labels.append('new')
 
     if title is None:
         if pr_target_repo == GITHUB_EASYCONFIGS_REPO:
@@ -1581,15 +1662,9 @@ def new_pr_from_branch(branch_name, title=None, descr=None, pr_target_repo=None,
         print_msg("Opened pull request: %s" % data['html_url'], log=_log, prefix=False)
 
         if labels:
-            # post labels
             pr = data['html_url'].split('/')[-1]
-            pr_url = g.repos[pr_target_account][pr_target_repo].issues[pr]
-            try:
-                status, data = pr_url.labels.post(body=labels)
-                if status == HTTP_STATUS_OK:
-                    print_msg("Added labels %s to PR#%s" % (', '.join(labels), pr), log=_log, prefix=False)
-            except HTTPError as err:
-                _log.info("Failed to add labels to PR# %s: %s." % (pr, err))
+            if not post_pr_labels(pr, labels):
+                print_msg("This PR should be labelled %s" % ', '.join(labels), log=_log, prefix=False)
 
 
 def new_pr(paths, ecs, title=None, descr=None, commit_msg=None):

easybuild/tools/options.py

@@ -631,6 +631,7 @@ def github_options(self):
         descr = ("GitHub integration options", "Integration with GitHub")
 
         opts = OrderedDict({
+            'add-pr-labels': ("Try to add labels to PR based on files changed", int, 'store', None, {'metavar': 'PR#'}),
             'check-github': ("Check status of GitHub integration, and report back", None, 'store_true', False),
             'check-contrib': ("Runs checks to see whether the given easyconfigs are ready to be contributed back",
                               None, 'store_true', False),

test/framework/github.py

@@ -42,7 +42,7 @@
 from easybuild.tools.config import build_option, module_classes
 from easybuild.tools.configobj import ConfigObj
 from easybuild.tools.filetools import read_file, write_file
-from easybuild.tools.github import VALID_CLOSE_PR_REASONS
+from easybuild.tools.github import GITHUB_EASYCONFIGS_REPO, GITHUB_EASYBLOCKS_REPO, VALID_CLOSE_PR_REASONS
 from easybuild.tools.testing import post_pr_test_report, session_state
 from easybuild.tools.py2vs3 import HTTPError, URLError, ascii_letters
 import easybuild.tools.github as gh
@@ -120,6 +120,48 @@ def test_read(self):
         except (IOError, OSError):
             pass
 
+    def test_add_pr_labels(self):
+        """Test add_pr_labels function."""
+        if self.skip_github_tests:
+            print("Skipping test_add_pr_labels, no GitHub token available?")
+            return
+
+        build_options = {
+            'pr_target_account': GITHUB_USER,
+            'pr_target_repo': GITHUB_EASYBLOCKS_REPO,
+            'github_user':  GITHUB_TEST_ACCOUNT,
+            'dry_run': True,
+        }
+        init_config(build_options=build_options)
+
+        self.mock_stdout(True)
+        error_pattern = "Adding labels to PRs for repositories other than easyconfigs hasn't been implemented yet"
+        self.assertErrorRegex(EasyBuildError, error_pattern, gh.add_pr_labels, 1)
+        self.mock_stdout(False)
+
+        build_options['pr_target_repo'] = GITHUB_EASYCONFIGS_REPO
+        init_config(build_options=build_options)
+
+        # PR #11262 includes easyconfigs that use 'dummy' toolchain,
+        # so we need to allow triggering deprecated behaviour
+        self.allow_deprecated_behaviour()
+
+        self.mock_stdout(True)
+        self.mock_stderr(True)
+        gh.add_pr_labels(11262)
+        stdout = self.get_stdout()
+        self.mock_stdout(False)
+        self.mock_stderr(False)
+        self.assertTrue("Could not determine any missing labels for PR #11262" in stdout)
+
+        self.mock_stdout(True)
+        self.mock_stderr(True)
+        gh.add_pr_labels(8006)  # closed, unmerged, unlabeled PR
+        stdout = self.get_stdout()
+        self.mock_stdout(False)
+        self.mock_stderr(False)
+        self.assertTrue("PR #8006 should be labelled 'update'" in stdout)
+
     def test_fetch_pr_data(self):
         """Test fetch_pr_data function."""
         if self.skip_github_tests:
@@ -681,6 +723,25 @@ def run_check(expected_result=False):
         expected_warning = ''
         self.assertEqual(run_check(True), '')
 
+    def test_det_pr_labels(self):
+        """Test for det_pr_labels function."""
+
+        file_info = {'new_folder': [False], 'new_file_in_existing_folder': [True]}
+        res = gh.det_pr_labels(file_info, GITHUB_EASYCONFIGS_REPO)
+        self.assertEqual(res, ['update'])
+
+        file_info = {'new_folder': [True], 'new_file_in_existing_folder': [False]}
+        res = gh.det_pr_labels(file_info, GITHUB_EASYCONFIGS_REPO)
+        self.assertEqual(res, ['new'])
+
+        file_info = {'new_folder': [True, False], 'new_file_in_existing_folder': [False, True]}
+        res = gh.det_pr_labels(file_info, GITHUB_EASYCONFIGS_REPO)
+        self.assertTrue(sorted(res), ['new', 'update'])
+
+        file_info = {'new': [True]}
+        res = gh.det_pr_labels(file_info, GITHUB_EASYBLOCKS_REPO)
+        self.assertEqual(res, ['new'])
+
     def test_det_patch_specs(self):
         """Test for det_patch_specs function."""
 

test/framework/options.py

@@ -754,6 +754,8 @@ def test_list_easyblocks(self):
                 r'EasyBlock',
                 r'\|-- bar',
                 r'\|-- ConfigureMake',
+                r'\|   \|-- MakeCp',
+                r'\|-- EB_EasyBuildMeta',
                 r'\|-- EB_FFTW',
                 r'\|-- EB_foo',
                 r'\|   \|-- EB_foofoo',
@@ -3582,6 +3584,21 @@ def test_review_pr(self):
         regex = re.compile(r"^Comparing gzip-1.10-\S* with gzip-1.10-")
         self.assertTrue(regex.search(txt), "Pattern '%s' not found in: %s" % (regex.pattern, txt))
 
+        self.mock_stdout(True)
+        self.mock_stderr(True)
+        # closed PR for gzip 1.2.8 easyconfig,
+        # see https://github.com/easybuilders/easybuild-easyconfigs/pull/5365
+        args = [
+            '--color=never',
+            '--github-user=%s' % GITHUB_TEST_ACCOUNT,
+            '--review-pr=5365',
+        ]
+        self.eb_main(args, raise_error=True, testing=True)
+        txt = self.get_stdout()
+        self.mock_stdout(False)
+        self.mock_stderr(False)
+        self.assertTrue("This PR should be labelled with 'update'" in txt)
+
     def test_set_tmpdir(self):
         """Test set_tmpdir config function."""
         self.purge_environment()

test/framework/sandbox/easybuild/easyblocks/e/__init__.py

@@ -0,0 +1,34 @@
+##
+# Copyright 2009-2020 Ghent University
+#
+# This file is part of EasyBuild,
+# originally created by the HPC team of Ghent University (http://ugent.be/hpc/en),
+# with support of Ghent University (http://ugent.be/hpc),
+# the Flemish Supercomputer Centre (VSC) (https://www.vscentrum.be),
+# Flemish Research Foundation (FWO) (http://www.fwo.be/en)
+# and the Department of Economy, Science and Innovation (EWI) (http://www.ewi-vlaanderen.be/en).
+#
+# https://github.com/easybuilders/easybuild
+#
+# EasyBuild is free software: you can redistribute it and/or modify
+# it under the terms of the GNU General Public License as published by
+# the Free Software Foundation v2.
+#
+# EasyBuild is distributed in the hope that it will be useful,
+# but WITHOUT ANY WARRANTY; without even the implied warranty of
+# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+# GNU General Public License for more details.
+#
+# You should have received a copy of the GNU General Public License
+# along with EasyBuild.  If not, see <http://www.gnu.org/licenses/>.
+##
+"""
+Dummy easyblock for EasyBuildMeta
+
+@author: Miguel Dias Costa (National University of Singapore)
+"""
+from easybuild.framework.easyblock import EasyBlock
+
+
+class EB_EasyBuildMeta(EasyBlock):
+    pass