From ea3873db02b53a0fe8764449f01ac880e2bad2cf Mon Sep 17 00:00:00 2001 From: Miguel Dias Costa Date: Thu, 11 Mar 2021 16:33:10 +0800 Subject: [PATCH 1/5] add support for multiple PRs in --from-pr --- easybuild/framework/easyconfig/tools.py | 20 +++++++++----- easybuild/main.py | 5 ++-- easybuild/tools/filetools.py | 14 ++++++---- easybuild/tools/options.py | 13 ++++++--- easybuild/tools/testing.py | 33 +++++++++++++++-------- test/framework/options.py | 35 +++++++++++++++++++++++++ 6 files changed, 93 insertions(+), 27 deletions(-) diff --git a/easybuild/framework/easyconfig/tools.py b/easybuild/framework/easyconfig/tools.py index e05af337ca..fc9c086ee4 100644 --- a/easybuild/framework/easyconfig/tools.py +++ b/easybuild/framework/easyconfig/tools.py @@ -321,7 +321,7 @@ def alt_easyconfig_paths(tmpdir, tweaked_ecs=False, from_pr=False): # path where files touched in PR will be downloaded to pr_path = None if from_pr: - pr_path = os.path.join(tmpdir, "files_pr%s" % from_pr) + pr_path = os.path.join(tmpdir, "files_pr%s" % '_'.join(str(pr) for pr in from_pr)) return tweaked_ecs_paths, pr_path @@ -332,14 +332,20 @@ def det_easyconfig_paths(orig_paths): :param orig_paths: list of original easyconfig paths :return: list of paths to easyconfig files """ - from_pr = build_option('from_pr') + try: + from_pr_list = map(int, build_option('from_pr')) + except ValueError: + raise EasyBuildError("Argument to --from-pr must be a comma separated list of PR #s.") + robot_path = build_option('robot_path') # list of specified easyconfig files ec_files = orig_paths[:] - if from_pr is not None: - pr_files = fetch_easyconfigs_from_pr(from_pr) + if from_pr_list is not None: + pr_files = [] + for pr in from_pr_list: + pr_files.extend(fetch_easyconfigs_from_pr(pr)) if ec_files: # replace paths for specified easyconfigs that are touched in PR @@ -746,8 +752,10 @@ def det_copy_ec_specs(orig_paths, from_pr): # to avoid potential trouble with already existing files in the working tmpdir # (note: we use a fixed subdirectory in the working tmpdir here rather than a unique random subdirectory, # to ensure that the caching for fetch_files_from_pr works across calls for the same PR) - tmpdir = os.path.join(tempfile.gettempdir(), 'fetch_files_from_pr_%s' % from_pr) - pr_paths = fetch_files_from_pr(pr=from_pr, path=tmpdir) + tmpdir = os.path.join(tempfile.gettempdir(), 'fetch_files_from_pr_%s' % '_'.join(str(pr) for pr in from_pr)) + pr_paths = [] + for pr in from_pr: + pr_paths.extend(fetch_files_from_pr(pr=pr, path=tmpdir)) # assume that files need to be copied to current working directory for now target_path = os.getcwd() diff --git a/easybuild/main.py b/easybuild/main.py index 3c7429ea78..af4b2d1ab0 100644 --- a/easybuild/main.py +++ b/easybuild/main.py @@ -207,7 +207,8 @@ def main(args=None, logfile=None, do_build=None, testing=False, modtool=None): options, orig_paths = eb_go.options, eb_go.args global _log - (build_specs, _log, logfile, robot_path, search_query, eb_tmpdir, try_to_generate, tweaked_ecs_paths) = cfg_settings + (build_specs, _log, logfile, robot_path, search_query, eb_tmpdir, try_to_generate, + from_pr_list, tweaked_ecs_paths) = cfg_settings # load hook implementations (if any) hooks = load_hooks(options.hooks) @@ -318,7 +319,7 @@ def main(args=None, logfile=None, do_build=None, testing=False, modtool=None): if options.copy_ec: # figure out list of files to copy + target location (taking into account --from-pr) - orig_paths, target_path = det_copy_ec_specs(orig_paths, options.from_pr) + orig_paths, target_path = det_copy_ec_specs(orig_paths, from_pr_list) categorized_paths = categorize_files_by_type(orig_paths) diff --git a/easybuild/tools/filetools.py b/easybuild/tools/filetools.py index 2ac341a622..99c63ad2db 100644 --- a/easybuild/tools/filetools.py +++ b/easybuild/tools/filetools.py @@ -291,11 +291,15 @@ def symlink(source_path, symlink_path, use_abspath_source=True): if use_abspath_source: source_path = os.path.abspath(source_path) - try: - os.symlink(source_path, symlink_path) - _log.info("Symlinked %s to %s", source_path, symlink_path) - except OSError as err: - raise EasyBuildError("Symlinking %s to %s failed: %s", source_path, symlink_path, err) + if os.path.exists(symlink_path): + _log.info("Skipping symlinking %s to %s, link already exists", source_path, symlink_path) + # TODO: check if the symlink_path points to source_path + else: + try: + os.symlink(source_path, symlink_path) + _log.info("Symlinked %s to %s", source_path, symlink_path) + except OSError as err: + raise EasyBuildError("Symlinking %s to %s failed: %s", source_path, symlink_path, err) def remove_file(path): diff --git a/easybuild/tools/options.py b/easybuild/tools/options.py index 605ed46ab9..18dcb82fd0 100644 --- a/easybuild/tools/options.py +++ b/easybuild/tools/options.py @@ -638,7 +638,7 @@ def github_options(self): 'check-style': ("Run a style check on the given easyconfigs", None, 'store_true', False), 'cleanup-easyconfigs': ("Clean up easyconfig files for pull request", None, 'store_true', True), 'dump-test-report': ("Dump test report to specified path", None, 'store_or_None', 'test_report.md'), - 'from-pr': ("Obtain easyconfigs from specified PR", int, 'store', None, {'metavar': 'PR#'}), + 'from-pr': ("Obtain easyconfigs from specified PR", 'strlist', 'store', [], {'metavar': 'PR#'}), 'git-working-dirs-path': ("Path to Git working directories for EasyBuild repositories", str, 'store', None), 'github-user': ("GitHub username", str, 'store', None), 'github-org': ("GitHub organization", str, 'store', None), @@ -1442,10 +1442,16 @@ def set_up_configuration(args=None, logfile=None, testing=False, silent=False): # software name/version, toolchain name/version, extra patches, ... (try_to_generate, build_specs) = process_software_build_specs(options) + # map --from-pr strlist to list of ints + try: + from_pr_list = map(int, eb_go.options.from_pr) + except ValueError: + raise EasyBuildError("Argument to --from-pr must be a comma separated list of PR #s.") + # determine robot path # --try-X, --dep-graph, --search use robot path for searching, so enable it with path of installed easyconfigs tweaked_ecs = try_to_generate and build_specs - tweaked_ecs_paths, pr_path = alt_easyconfig_paths(tmpdir, tweaked_ecs=tweaked_ecs, from_pr=options.from_pr) + tweaked_ecs_paths, pr_path = alt_easyconfig_paths(tmpdir, tweaked_ecs=tweaked_ecs, from_pr=from_pr_list) auto_robot = try_to_generate or options.check_conflicts or options.dep_graph or search_query robot_path = det_robot_path(options.robot_paths, tweaked_ecs_paths, pr_path, auto_robot=auto_robot) log.debug("Full robot path: %s" % robot_path) @@ -1517,7 +1523,8 @@ def set_up_configuration(args=None, logfile=None, testing=False, silent=False): sys.path.remove(fake_vsc_path) sys.path.insert(0, new_fake_vsc_path) - return eb_go, (build_specs, log, logfile, robot_path, search_query, tmpdir, try_to_generate, tweaked_ecs_paths) + return eb_go, (build_specs, log, logfile, robot_path, search_query, tmpdir, try_to_generate, + from_pr_list, tweaked_ecs_paths) def process_software_build_specs(options): diff --git a/easybuild/tools/testing.py b/easybuild/tools/testing.py index cf93034570..1acd98d38d 100644 --- a/easybuild/tools/testing.py +++ b/easybuild/tools/testing.py @@ -138,7 +138,7 @@ def session_state(): } -def create_test_report(msg, ecs_with_res, init_session_state, pr_nr=None, gist_log=False): +def create_test_report(msg, ecs_with_res, init_session_state, pr_nrs=None, gist_log=False): """Create test report for easyconfigs PR, in Markdown format.""" github_user = build_option('github_user') @@ -149,9 +149,10 @@ def create_test_report(msg, ecs_with_res, init_session_state, pr_nr=None, gist_l # create a gist with a full test report test_report = [] - if pr_nr is not None: + if pr_nrs is not None: + pr_urls = ["https://github.com/%s/%s/pull/%s" % (pr_target_account, pr_target_repo, pr_nr) for pr_nr in pr_nrs] test_report.extend([ - "Test report for https://github.com/%s/%s/pull/%s" % (pr_target_account, pr_target_repo, pr_nr), + "Test report for %s" % ', '.join(pr_urls), "", ]) test_report.extend([ @@ -182,8 +183,8 @@ def create_test_report(msg, ecs_with_res, init_session_state, pr_nr=None, gist_l logtxt = read_file(ec_res['log_file']) partial_log_txt = '\n'.join(logtxt.split('\n')[-500:]) descr = "(partial) EasyBuild log for failed build of %s" % ec['spec'] - if pr_nr is not None: - descr += " (PR #%s)" % pr_nr + if pr_nrs is not None: + descr += " (PR #%s)" % ', #'.join(pr_nrs) fn = '%s_partial.log' % os.path.basename(ec['spec'])[:-3] gist_url = create_gist(partial_log_txt, fn, descr=descr, github_user=github_user) test_log = "(partial log available at %s)" % gist_url @@ -317,19 +318,29 @@ def overall_test_report(ecs_with_res, orig_cnt, success, msg, init_session_state :param init_session_state: initial session state info to include in test report """ dump_path = build_option('dump_test_report') - pr_nr = build_option('from_pr') - eb_pr_nrs = build_option('include_easyblocks_from_pr') + + try: + pr_nrs = map(int, build_option('from_pr')) + except ValueError: + raise EasyBuildError("Argument to --from-pr must be a comma separated list of PR #s.") + + try: + eb_pr_nrs = map(int, build_option('include_easyblocks_from_pr')) + except ValueError: + raise EasyBuildError("Argument to --include-easyblocks-from-pr must be a comma separated list of PR #s.") + upload = build_option('upload_test_report') if upload: msg = msg + " (%d easyconfigs in total)" % orig_cnt - test_report = create_test_report(msg, ecs_with_res, init_session_state, pr_nr=pr_nr, gist_log=True) - if pr_nr: + test_report = create_test_report(msg, ecs_with_res, init_session_state, pr_nrs=pr_nrs, gist_log=True) + if pr_nrs: # upload test report to gist and issue a comment in the PR to notify - txt = post_pr_test_report(pr_nr, GITHUB_EASYCONFIGS_REPO, test_report, msg, init_session_state, success) + for pr_nr in pr_nrs: + txt = post_pr_test_report(pr_nr, GITHUB_EASYCONFIGS_REPO, test_report, msg, init_session_state, success) elif eb_pr_nrs: # upload test report to gist and issue a comment in the easyblocks PR to notify - for eb_pr_nr in map(int, eb_pr_nrs): + for eb_pr_nr in eb_pr_nrs: txt = post_pr_test_report(eb_pr_nr, GITHUB_EASYBLOCKS_REPO, test_report, msg, init_session_state, success) else: diff --git a/test/framework/options.py b/test/framework/options.py index 3a8539014c..171cb430e0 100644 --- a/test/framework/options.py +++ b/test/framework/options.py @@ -1731,6 +1731,41 @@ def test_from_pr(self): print("Ignoring URLError '%s' in test_from_pr" % err) shutil.rmtree(tmpdir) + # test with multiple prs + tmpdir = tempfile.mkdtemp() + args = [ + # PRs for ReFrame 3.4.1 and 3.5.0 + '--from-pr=12150,12366', + '--dry-run', + # an argument must be specified to --robot, since easybuild-easyconfigs may not be installed + '--robot=%s' % os.path.join(os.path.dirname(__file__), 'easyconfigs'), + '--unittest-file=%s' % self.logfile, + '--github-user=%s' % GITHUB_TEST_ACCOUNT, # a GitHub token should be available for this user + '--tmpdir=%s' % tmpdir, + ] + try: + outtxt = self.eb_main(args, logfile=dummylogfn, raise_error=True) + modules = [ + (tmpdir, 'ReFrame/3.4.1'), + (tmpdir, 'ReFrame/3.5.0'), + ] + for path_prefix, module in modules: + ec_fn = "%s.eb" % '-'.join(module.split('/')) + path = '.*%s' % os.path.dirname(path_prefix) + regex = re.compile(r"^ \* \[.\] %s.*%s \(module: %s\)$" % (path, ec_fn, module), re.M) + self.assertTrue(regex.search(outtxt), "Found pattern %s in %s" % (regex.pattern, outtxt)) + + # make sure that *only* these modules are listed, no others + regex = re.compile(r"^ \* \[.\] .*/(?P.*) \(module: (?P.*)\)$", re.M) + self.assertTrue(sorted(regex.findall(outtxt)), sorted(modules)) + + pr_tmpdir = os.path.join(tmpdir, r'eb-\S{6,8}', 'files_pr12150_12366') + regex = re.compile("Appended list of robot search paths with %s:" % pr_tmpdir, re.M) + self.assertTrue(regex.search(outtxt), "Found pattern %s in %s" % (regex.pattern, outtxt)) + except URLError as err: + print("Ignoring URLError '%s' in test_from_pr" % err) + shutil.rmtree(tmpdir) + def test_from_pr_token_log(self): """Check that --from-pr doesn't leak GitHub token in log.""" if self.github_token is None: From 9a7f0e4886feb95f76754eb60aca845dc02f3187 Mon Sep 17 00:00:00 2001 From: Miguel Dias Costa Date: Fri, 12 Mar 2021 14:04:06 +0800 Subject: [PATCH 2/5] fix tests after adding support for multiple PRs in --from-pr --- easybuild/framework/easyconfig/tools.py | 3 ++ test/framework/options.py | 1 + .../easyblocks/generic/pythonbundle.py | 40 +++++++++++++++++++ 3 files changed, 44 insertions(+) create mode 100644 test/framework/sandbox/easybuild/easyblocks/generic/pythonbundle.py diff --git a/easybuild/framework/easyconfig/tools.py b/easybuild/framework/easyconfig/tools.py index fc9c086ee4..14913baff2 100644 --- a/easybuild/framework/easyconfig/tools.py +++ b/easybuild/framework/easyconfig/tools.py @@ -731,6 +731,9 @@ def avail_easyblocks(): def det_copy_ec_specs(orig_paths, from_pr): """Determine list of paths + target directory for --copy-ec.""" + if from_pr is not None and not isinstance(from_pr, list): + from_pr = [from_pr] + target_path, paths = None, [] # if only one argument is specified, use current directory as target directory diff --git a/test/framework/options.py b/test/framework/options.py index 1b363b3857..f4f290f340 100644 --- a/test/framework/options.py +++ b/test/framework/options.py @@ -821,6 +821,7 @@ def test_list_easyblocks(self): r'\| \| \|-- EB_toytoy', r'\| \|-- Toy_Extension', r'\|-- ModuleRC', + r'\|-- PythonBundle', r'\|-- Toolchain', r'Extension', r'\|-- ExtensionEasyBlock', diff --git a/test/framework/sandbox/easybuild/easyblocks/generic/pythonbundle.py b/test/framework/sandbox/easybuild/easyblocks/generic/pythonbundle.py new file mode 100644 index 0000000000..7146b8ecd3 --- /dev/null +++ b/test/framework/sandbox/easybuild/easyblocks/generic/pythonbundle.py @@ -0,0 +1,40 @@ +## +# 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 . +## +""" +Dummy easyblock for Makecp. + +@author: Miguel Dias Costa (National University of Singapore) +""" +from easybuild.framework.easyblock import EasyBlock + + +class PythonBundle(EasyBlock): + """Dummy support for bundle of modules.""" + + @staticmethod + def extra_options(extra_vars=None): + if extra_vars is None: + extra_vars = {} + return EasyBlock.extra_options(extra_vars) From 8092e69828fd2340cbdf82d76f94a5e283efdbb7 Mon Sep 17 00:00:00 2001 From: Miguel Dias Costa Date: Tue, 16 Mar 2021 14:53:48 +0800 Subject: [PATCH 3/5] use list comprehension instead of map when parsing from_pr strlist options --- easybuild/framework/easyconfig/tools.py | 2 +- easybuild/tools/options.py | 4 ++-- easybuild/tools/testing.py | 6 +++--- 3 files changed, 6 insertions(+), 6 deletions(-) diff --git a/easybuild/framework/easyconfig/tools.py b/easybuild/framework/easyconfig/tools.py index 14913baff2..20aa2e9430 100644 --- a/easybuild/framework/easyconfig/tools.py +++ b/easybuild/framework/easyconfig/tools.py @@ -333,7 +333,7 @@ def det_easyconfig_paths(orig_paths): :return: list of paths to easyconfig files """ try: - from_pr_list = map(int, build_option('from_pr')) + from_pr_list = [int(pr_nr) for pr_nr in build_option('from_pr')] except ValueError: raise EasyBuildError("Argument to --from-pr must be a comma separated list of PR #s.") diff --git a/easybuild/tools/options.py b/easybuild/tools/options.py index 18dcb82fd0..2d496cbf49 100644 --- a/easybuild/tools/options.py +++ b/easybuild/tools/options.py @@ -1444,7 +1444,7 @@ def set_up_configuration(args=None, logfile=None, testing=False, silent=False): # map --from-pr strlist to list of ints try: - from_pr_list = map(int, eb_go.options.from_pr) + from_pr_list = [int(pr_nr) for pr_nr in eb_go.options.from_pr] except ValueError: raise EasyBuildError("Argument to --from-pr must be a comma separated list of PR #s.") @@ -1480,7 +1480,7 @@ def set_up_configuration(args=None, logfile=None, testing=False, silent=False): # done here instead of in _postprocess_include because github integration requires build_options to be initialized if eb_go.options.include_easyblocks_from_pr: try: - easyblock_prs = map(int, eb_go.options.include_easyblocks_from_pr) + easyblock_prs = [int(pr_nr) for pr_nr in eb_go.options.include_easyblocks_from_pr] except ValueError: raise EasyBuildError("Argument to --include-easyblocks-from-pr must be a comma separated list of PR #s.") diff --git a/easybuild/tools/testing.py b/easybuild/tools/testing.py index 1acd98d38d..7bf38b4c61 100644 --- a/easybuild/tools/testing.py +++ b/easybuild/tools/testing.py @@ -282,7 +282,7 @@ def post_pr_test_report(pr_nr, repo_type, test_report, msg, init_session_state, if build_option('include_easyblocks_from_pr'): if repo_type == GITHUB_EASYCONFIGS_REPO: - easyblocks_pr_nrs = map(int, build_option('include_easyblocks_from_pr')) + easyblocks_pr_nrs = [int(pr_nr) for pr_nr in build_option('include_easyblocks_from_pr')] comment_lines.append("Using easyblocks from PR(s) %s" % ", ".join(["https://github.com/%s/%s/pull/%s" % (pr_target_account, GITHUB_EASYBLOCKS_REPO, easyblocks_pr_nr) @@ -320,12 +320,12 @@ def overall_test_report(ecs_with_res, orig_cnt, success, msg, init_session_state dump_path = build_option('dump_test_report') try: - pr_nrs = map(int, build_option('from_pr')) + pr_nrs = [int(pr_nr) for pr_nr in build_option('from_pr')] except ValueError: raise EasyBuildError("Argument to --from-pr must be a comma separated list of PR #s.") try: - eb_pr_nrs = map(int, build_option('include_easyblocks_from_pr')) + eb_pr_nrs = [int(pr_nr) for pr_nr in build_option('include_easyblocks_from_pr')] except ValueError: raise EasyBuildError("Argument to --include-easyblocks-from-pr must be a comma separated list of PR #s.") From 64f96e1b35d6e5822b9f0e2160e1d6405f5d833e Mon Sep 17 00:00:00 2001 From: Miguel Dias Costa Date: Tue, 25 May 2021 16:34:06 +0800 Subject: [PATCH 4/5] also check if existing symlink is correct before skipping --- easybuild/tools/filetools.py | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/easybuild/tools/filetools.py b/easybuild/tools/filetools.py index 05b06061e8..d09750c2d9 100644 --- a/easybuild/tools/filetools.py +++ b/easybuild/tools/filetools.py @@ -298,9 +298,8 @@ def symlink(source_path, symlink_path, use_abspath_source=True): if use_abspath_source: source_path = os.path.abspath(source_path) - if os.path.exists(symlink_path): + if os.path.exists(symlink_path) and os.path.abspath(source_path) == os.path.abspath(os.readlink(symlink_path)): _log.info("Skipping symlinking %s to %s, link already exists", source_path, symlink_path) - # TODO: check if the symlink_path points to source_path else: try: os.symlink(source_path, symlink_path) From d3d8e4c4a35a239a7b2223ba9cfd7e15bee932a8 Mon Sep 17 00:00:00 2001 From: Miguel Dias Costa Date: Tue, 25 May 2021 20:29:30 +0800 Subject: [PATCH 5/5] raise error when symlink already exists but points to a different path, and add test for it --- easybuild/tools/filetools.py | 7 ++++++- test/framework/filetools.py | 11 +++++++++++ 2 files changed, 17 insertions(+), 1 deletion(-) diff --git a/easybuild/tools/filetools.py b/easybuild/tools/filetools.py index d09750c2d9..4ad50ed0be 100644 --- a/easybuild/tools/filetools.py +++ b/easybuild/tools/filetools.py @@ -298,7 +298,12 @@ def symlink(source_path, symlink_path, use_abspath_source=True): if use_abspath_source: source_path = os.path.abspath(source_path) - if os.path.exists(symlink_path) and os.path.abspath(source_path) == os.path.abspath(os.readlink(symlink_path)): + if os.path.exists(symlink_path): + abs_source_path = os.path.abspath(source_path) + symlink_target_path = os.path.abspath(os.readlink(symlink_path)) + if abs_source_path != symlink_target_path: + raise EasyBuildError("Trying to symlink %s to %s, but the symlink already exists and points to %s.", + source_path, symlink_path, symlink_target_path) _log.info("Skipping symlinking %s to %s, link already exists", source_path, symlink_path) else: try: diff --git a/test/framework/filetools.py b/test/framework/filetools.py index 55fa363858..3effdeaa52 100644 --- a/test/framework/filetools.py +++ b/test/framework/filetools.py @@ -574,6 +574,17 @@ def test_symlink_resolve_path(self): self.assertTrue(os.path.samefile(os.path.join(self.test_prefix, 'test', 'test.txt'), link)) + # test symlink when it already exists and points to the same path + ft.symlink(test_file, link) + + # test symlink when it already exists but points to a different path + test_file2 = os.path.join(link_dir, 'test2.txt') + ft.write_file(test_file, "test123") + self.assertErrorRegex(EasyBuildError, + "Trying to symlink %s to %s, but the symlink already exists and points to %s." % + (test_file2, link, test_file), + ft.symlink, test_file2, link) + # test resolve_path self.assertEqual(test_dir, ft.resolve_path(link_dir)) self.assertEqual(os.path.join(os.path.realpath(self.test_prefix), 'test', 'test.txt'), ft.resolve_path(link))