From ab933f1a6c4b6d8d72df89698b97bb0640fcdbde Mon Sep 17 00:00:00 2001 From: Alan O'Cais Date: Mon, 19 Oct 2020 09:37:37 +0200 Subject: [PATCH 01/51] Allow use of --copy-ec with --from-pr --- easybuild/main.py | 49 ++++++++++++++++++++++++++++++++++------------- 1 file changed, 36 insertions(+), 13 deletions(-) diff --git a/easybuild/main.py b/easybuild/main.py index bd7a0b0f5e..79f009b0cd 100644 --- a/easybuild/main.py +++ b/easybuild/main.py @@ -176,6 +176,26 @@ def run_contrib_style_checks(ecs, check_contrib, check_style): return check_contrib or check_style +def copy_ecs_to_target(determined_paths, target_path, prefix=False): + """ + Copy list of easyconfigs to specified path + + :param determined_paths: paths to ecs to copy + :param target_path: target to copy files to + :param prefix: include message prefix characters + """ + if not target_path: + raise EasyBuildError("No target path specified to copy files to!") + if len(determined_paths) == 1: + copy_file(determined_paths[0], target_path) + print_msg("%s copied to %s" % (os.path.basename(determined_paths[0]), target_path), prefix=prefix) + elif len(determined_paths) > 1: + copy_files(determined_paths, target_path) + print_msg("%d file(s) copied to %s" % (len(determined_paths), target_path), prefix=prefix) + else: + raise EasyBuildError("One of more files to copy should be specified!") + + def clean_exit(logfile, tmpdir, testing, silent=False): """Small utility function to perform a clean exit.""" cleanup(logfile, tmpdir, testing, silent=silent) @@ -303,12 +323,16 @@ def main(args=None, logfile=None, do_build=None, testing=False, modtool=None): eb_file = find_easybuild_easyconfig() orig_paths.append(eb_file) - if len(orig_paths) == 1: + if not orig_paths and options.copy_ec and options.from_pr: + # if no easyconfig files are specified and we are using --from-pr, use current directory as target directory + target_path = os.getcwd() + elif len(orig_paths) == 1 and not (options.copy_ec and options.from_pr): # if only one easyconfig file is specified, use current directory as target directory target_path = os.getcwd() elif orig_paths: # last path is target when --copy-ec is used, so remove that from the list - target_path = orig_paths.pop() if options.copy_ec else None + # use the absolute path as some use cases occur in a temporary directory + target_path = os.path.abspath(orig_paths.pop()) if options.copy_ec else None categorized_paths = categorize_files_by_type(orig_paths) @@ -321,17 +345,11 @@ def main(args=None, logfile=None, do_build=None, testing=False, modtool=None): # determine paths to easyconfigs determined_paths = det_easyconfig_paths(categorized_paths['easyconfigs']) - if (options.copy_ec and not tweaked_ecs_paths) or options.fix_deprecated_easyconfigs or options.show_ec: + if (options.copy_ec and not tweaked_ecs_paths) or \ + (options.copy_ec and not options.from_pr) or options.fix_deprecated_easyconfigs or options.show_ec: if options.copy_ec: - if len(determined_paths) == 1: - copy_file(determined_paths[0], target_path) - print_msg("%s copied to %s" % (os.path.basename(determined_paths[0]), target_path), prefix=False) - elif len(determined_paths) > 1: - copy_files(determined_paths, target_path) - print_msg("%d file(s) copied to %s" % (len(determined_paths), target_path), prefix=False) - else: - raise EasyBuildError("One of more files to copy should be specified!") + copy_ecs_to_target(determined_paths, target_path) elif options.fix_deprecated_easyconfigs: fix_deprecated_easyconfigs(determined_paths) @@ -369,6 +387,11 @@ def main(args=None, logfile=None, do_build=None, testing=False, modtool=None): # read easyconfig files easyconfigs, generated_ecs = parse_easyconfigs(paths, validate=not options.inject_checksums) + # if we are only copying ec's for a specific PR we can do that now + if options.copy_ec and options.from_pr: + copy_ecs_to_target(determined_paths,target_path) + clean_exit(logfile, eb_tmpdir, testing) + # handle --check-contrib & --check-style options if run_contrib_style_checks([ec['ec'] for ec in easyconfigs], options.check_contrib, options.check_style): clean_exit(logfile, eb_tmpdir, testing) @@ -429,8 +452,8 @@ def main(args=None, logfile=None, do_build=None, testing=False, modtool=None): if tweaked_ecs_in_all_ecs: # Clean them, then copy them clean_up_easyconfigs(tweaked_ecs_in_all_ecs) - copy_files(tweaked_ecs_in_all_ecs, target_path) - print_msg("%d file(s) copied to %s" % (len(tweaked_ecs_in_all_ecs), target_path), prefix=False) + copy_ecs_to_target(tweaked_ecs_in_all_ecs, target_path) + clean_exit(logfile, eb_tmpdir, testing) # creating/updating PRs if pr_options: From 9accd5dc826af6f8bf382bf5e2af5abf588e2b3a Mon Sep 17 00:00:00 2001 From: Alan O'Cais Date: Mon, 19 Oct 2020 09:40:31 +0200 Subject: [PATCH 02/51] Appease the hound --- easybuild/main.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/easybuild/main.py b/easybuild/main.py index 79f009b0cd..ccfcc58fd7 100644 --- a/easybuild/main.py +++ b/easybuild/main.py @@ -389,7 +389,7 @@ def main(args=None, logfile=None, do_build=None, testing=False, modtool=None): # if we are only copying ec's for a specific PR we can do that now if options.copy_ec and options.from_pr: - copy_ecs_to_target(determined_paths,target_path) + copy_ecs_to_target(determined_paths, target_path) clean_exit(logfile, eb_tmpdir, testing) # handle --check-contrib & --check-style options From e2f270af2486711d6926585cb49d2a05ee253f25 Mon Sep 17 00:00:00 2001 From: Alan O'Cais Date: Mon, 19 Oct 2020 10:07:23 +0200 Subject: [PATCH 03/51] Fix broken tests --- test/framework/options.py | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/test/framework/options.py b/test/framework/options.py index e2c90a22be..200c818250 100644 --- a/test/framework/options.py +++ b/test/framework/options.py @@ -1002,7 +1002,7 @@ def mocked_main(args): args = ['--copy-ec', 'toy-0.0.eb', target_fn] stdout = mocked_main(args) - self.assertEqual(stdout, 'toy-0.0.eb copied to test.eb') + self.assertEqual(stdout, 'toy-0.0.eb copied to %s/test.eb' % cwd) change_dir(cwd) @@ -2072,7 +2072,7 @@ def test_try_with_copy(self): verbose=True, raise_error=True) outtxt = self.get_stdout() errtxt = self.get_stderr() - self.assertTrue(r'1 file(s) copied to ' + tweaked_ecs_dir in outtxt) + self.assertTrue( + r'foo-1.2.3-gompi-2018a.eb copied to ' + tweaked_ecs_dir in outtxt) self.assertFalse(errtxt) self.mock_stdout(False) self.mock_stderr(False) From 64e95f23d6b6d811f3e46186f883a4f6c7aa8fef Mon Sep 17 00:00:00 2001 From: Alan O'Cais Date: Mon, 19 Oct 2020 10:08:41 +0200 Subject: [PATCH 04/51] Fix broken tests --- test/framework/options.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/test/framework/options.py b/test/framework/options.py index 200c818250..12c26339a8 100644 --- a/test/framework/options.py +++ b/test/framework/options.py @@ -2072,7 +2072,7 @@ def test_try_with_copy(self): verbose=True, raise_error=True) outtxt = self.get_stdout() errtxt = self.get_stderr() - self.assertTrue( + r'foo-1.2.3-gompi-2018a.eb copied to ' + tweaked_ecs_dir in outtxt) + self.assertTrue(r'foo-1.2.3-gompi-2018a.eb copied to ' + tweaked_ecs_dir in outtxt) self.assertFalse(errtxt) self.mock_stdout(False) self.mock_stderr(False) From 4e6980fe819a940b5e9e856c8f7191706df9cc22 Mon Sep 17 00:00:00 2001 From: Alan O'Cais Date: Mon, 19 Oct 2020 12:55:40 +0200 Subject: [PATCH 05/51] Fix broken tests (2nd attempt) --- easybuild/main.py | 10 ++++------ test/framework/options.py | 14 +++++++------- 2 files changed, 11 insertions(+), 13 deletions(-) diff --git a/easybuild/main.py b/easybuild/main.py index ccfcc58fd7..609fed00d5 100644 --- a/easybuild/main.py +++ b/easybuild/main.py @@ -184,8 +184,6 @@ def copy_ecs_to_target(determined_paths, target_path, prefix=False): :param target_path: target to copy files to :param prefix: include message prefix characters """ - if not target_path: - raise EasyBuildError("No target path specified to copy files to!") if len(determined_paths) == 1: copy_file(determined_paths[0], target_path) print_msg("%s copied to %s" % (os.path.basename(determined_paths[0]), target_path), prefix=prefix) @@ -323,9 +321,9 @@ def main(args=None, logfile=None, do_build=None, testing=False, modtool=None): eb_file = find_easybuild_easyconfig() orig_paths.append(eb_file) - if not orig_paths and options.copy_ec and options.from_pr: + if not orig_paths: # if no easyconfig files are specified and we are using --from-pr, use current directory as target directory - target_path = os.getcwd() + target_path = os.getcwd() if (options.copy_ec and options.from_pr) else None elif len(orig_paths) == 1 and not (options.copy_ec and options.from_pr): # if only one easyconfig file is specified, use current directory as target directory target_path = os.getcwd() @@ -345,8 +343,8 @@ def main(args=None, logfile=None, do_build=None, testing=False, modtool=None): # determine paths to easyconfigs determined_paths = det_easyconfig_paths(categorized_paths['easyconfigs']) - if (options.copy_ec and not tweaked_ecs_paths) or \ - (options.copy_ec and not options.from_pr) or options.fix_deprecated_easyconfigs or options.show_ec: + if (options.copy_ec and not (tweaked_ecs_paths or options.from_pr)) or options.fix_deprecated_easyconfigs or \ + options.show_ec: if options.copy_ec: copy_ecs_to_target(determined_paths, target_path) diff --git a/test/framework/options.py b/test/framework/options.py index 12c26339a8..488640d9dd 100644 --- a/test/framework/options.py +++ b/test/framework/options.py @@ -1002,7 +1002,7 @@ def mocked_main(args): args = ['--copy-ec', 'toy-0.0.eb', target_fn] stdout = mocked_main(args) - self.assertEqual(stdout, 'toy-0.0.eb copied to %s/test.eb' % cwd) + self.assertEqual(stdout, 'toy-0.0.eb copied to %s/test.eb' % self.test_prefix) change_dir(cwd) @@ -1048,7 +1048,7 @@ def check_copied_files(): self.assertFalse(os.path.exists(args[-1])) stdout = mocked_main(args) - self.assertEqual(stdout, '2 file(s) copied to test_target_dir') + self.assertEqual(stdout, '2 file(s) copied to %s' % test_target_dir) check_copied_files() @@ -2067,15 +2067,15 @@ def test_try_with_copy(self): self.mock_stdout(True) self.mock_stderr(True) - tweaked_ecs_dir = os.path.join(self.test_buildpath, 'my_tweaked_ecs') - self.eb_main(args + ['--try-software=foo,1.2.3', '--try-toolchain=gompi,2018a', tweaked_ecs_dir], - verbose=True, raise_error=True) + tweaked_ecs_dir = os.path.join(self.test_buildpath) + os.chdir(tweaked_ecs_dir) + self.eb_main(args + ['--try-software=foo,1.2.3', '--try-toolchain=gompi,2018a'], verbose=True, raise_error=True) outtxt = self.get_stdout() errtxt = self.get_stderr() - self.assertTrue(r'foo-1.2.3-gompi-2018a.eb copied to ' + tweaked_ecs_dir in outtxt) - self.assertFalse(errtxt) self.mock_stdout(False) self.mock_stderr(False) + self.assertTrue(r'foo-1.2.3-GCC-6.4.0-2.28.eb copied to ' + tweaked_ecs_dir in outtxt) + self.assertFalse(errtxt) self.assertTrue( os.path.exists(os.path.join(self.test_buildpath, tweaked_ecs_dir, 'foo-1.2.3-GCC-6.4.0-2.28.eb')) ) From fceea89aa1fac7ad2e774fcf1b172d57a07dc54b Mon Sep 17 00:00:00 2001 From: Alan O'Cais Date: Mon, 19 Oct 2020 13:41:37 +0200 Subject: [PATCH 06/51] Reinstate the copy of the easyconfigs from the PR --- easybuild/main.py | 1 + test/framework/options.py | 2 +- 2 files changed, 2 insertions(+), 1 deletion(-) diff --git a/easybuild/main.py b/easybuild/main.py index 609fed00d5..8507e492df 100644 --- a/easybuild/main.py +++ b/easybuild/main.py @@ -387,6 +387,7 @@ def main(args=None, logfile=None, do_build=None, testing=False, modtool=None): # if we are only copying ec's for a specific PR we can do that now if options.copy_ec and options.from_pr: + determined_paths = [ec['spec'] for ec in easyconfigs] copy_ecs_to_target(determined_paths, target_path) clean_exit(logfile, eb_tmpdir, testing) diff --git a/test/framework/options.py b/test/framework/options.py index 488640d9dd..cf384d171a 100644 --- a/test/framework/options.py +++ b/test/framework/options.py @@ -1052,7 +1052,7 @@ def check_copied_files(): check_copied_files() - # copying multiple easyconfig to an existing target file resuts in an error + # copying multiple easyconfig to an existing target file results in an error target = os.path.join(self.test_prefix, 'test.eb') self.assertTrue(os.path.isfile(target)) args = ['--copy-ec', 'toy-0.0.eb', 'bzip2-1.0.6-GCC-4.9.2.eb', target] From 51f0b068e49621951d9e7d03e5e682aec44869f3 Mon Sep 17 00:00:00 2001 From: Alan O'Cais Date: Mon, 19 Oct 2020 14:29:00 +0200 Subject: [PATCH 07/51] Add test for --from-pr and --copy-ec --- easybuild/main.py | 12 ++++++------ test/framework/options.py | 14 ++++++++++++++ 2 files changed, 20 insertions(+), 6 deletions(-) diff --git a/easybuild/main.py b/easybuild/main.py index 8507e492df..a668c5deb7 100644 --- a/easybuild/main.py +++ b/easybuild/main.py @@ -373,6 +373,12 @@ def main(args=None, logfile=None, do_build=None, testing=False, modtool=None): log=_log, opt_parser=eb_go.parser, exit_on_error=not testing) _log.debug("Paths: %s", paths) + # if we are only copying ec's for a specific PR we can do that now + if options.copy_ec and options.from_pr: + determined_paths = [path[0] for path in paths] + copy_ecs_to_target(determined_paths, target_path) + clean_exit(logfile, eb_tmpdir, testing) + # run regtest if options.regtest or options.aggregate_regtest: _log.info("Running regression test") @@ -385,12 +391,6 @@ def main(args=None, logfile=None, do_build=None, testing=False, modtool=None): # read easyconfig files easyconfigs, generated_ecs = parse_easyconfigs(paths, validate=not options.inject_checksums) - # if we are only copying ec's for a specific PR we can do that now - if options.copy_ec and options.from_pr: - determined_paths = [ec['spec'] for ec in easyconfigs] - copy_ecs_to_target(determined_paths, target_path) - clean_exit(logfile, eb_tmpdir, testing) - # handle --check-contrib & --check-style options if run_contrib_style_checks([ec['ec'] for ec in easyconfigs], options.check_contrib, options.check_style): clean_exit(logfile, eb_tmpdir, testing) diff --git a/test/framework/options.py b/test/framework/options.py index cf384d171a..fa230982c3 100644 --- a/test/framework/options.py +++ b/test/framework/options.py @@ -1072,6 +1072,20 @@ def check_copied_files(): self.assertTrue(os.path.exists(copied_toy_cwd)) self.assertEqual(read_file(copied_toy_cwd), toy_ec_txt) + # Test --copy-ec coupled with --from-pr + all_ecs_pr8007 = [ + 'Arrow-0.7.1-intel-2017b-Python-3.6.3.eb', + 'bat-0.3.3-intel-2017b-Python-3.6.3.eb', + 'bat-0.3.3-fix-pyspark.patch', + ] + # copying multiple easyconfig files to a non-existing target directory (which is created automatically) + args = ['--copy-ec', '--from-pr', '8007', test_target_dir] + stdout = mocked_main(args) + self.assertEqual(stdout, '2 file(s) copied to %s' % test_target_dir) + # Check that the two easyconfigs exist + self.assertTrue(os.path.exists(os.path.join(test_target_dir, 'Arrow-0.7.1-intel-2017b-Python-3.6.3.eb'))) + self.assertTrue(os.path.exists(os.path.join(test_target_dir, 'bat-0.3.3-intel-2017b-Python-3.6.3.eb'))) + # --copy-ec without arguments results in a proper error args = ['--copy-ec'] error_pattern = "One of more files to copy should be specified!" From d633ece19f4fbd53338781fdceedd7a823df90d1 Mon Sep 17 00:00:00 2001 From: Alan O'Cais Date: Mon, 19 Oct 2020 16:42:19 +0200 Subject: [PATCH 08/51] Fix linting and add additional tests --- test/framework/options.py | 37 +++++++++++++++++++++++++++++++++---- 1 file changed, 33 insertions(+), 4 deletions(-) diff --git a/test/framework/options.py b/test/framework/options.py index fa230982c3..9ef1ab6035 100644 --- a/test/framework/options.py +++ b/test/framework/options.py @@ -1075,16 +1075,45 @@ def check_copied_files(): # Test --copy-ec coupled with --from-pr all_ecs_pr8007 = [ 'Arrow-0.7.1-intel-2017b-Python-3.6.3.eb', - 'bat-0.3.3-intel-2017b-Python-3.6.3.eb', 'bat-0.3.3-fix-pyspark.patch', + 'bat-0.3.3-intel-2017b-Python-3.6.3.eb', ] + + # test use of `--copy-ec` with `--from-pr` to the cwd + test_working_dir = os.path.join(self.test_prefix, 'test_working_dir') + mkdir(test_working_dir) + change_dir(test_working_dir) + args = ['--copy-ec', '--from-pr', '8007'] + stdout = mocked_main(args) + self.assertEqual(stdout, '2 file(s) copied to %s' % test_working_dir) + # check that the two easyconfigs exist + self.assertTrue(os.path.exists(os.path.join(test_working_dir, all_ecs_pr8007[0]))) + remove_file(os.path.join(test_working_dir, all_ecs_pr8007[0])) + self.assertTrue(os.path.exists(os.path.join(test_working_dir, all_ecs_pr8007[2]))) + remove_file(os.path.join(test_working_dir, all_ecs_pr8007[2])) + # ...but the patch doesn't + self.assertFalse(os.path.exists(os.path.join(test_working_dir, all_ecs_pr8007[1]))) + remove_file(os.path.join(test_working_dir, all_ecs_pr8007[1])) + # copying multiple easyconfig files to a non-existing target directory (which is created automatically) args = ['--copy-ec', '--from-pr', '8007', test_target_dir] stdout = mocked_main(args) self.assertEqual(stdout, '2 file(s) copied to %s' % test_target_dir) - # Check that the two easyconfigs exist - self.assertTrue(os.path.exists(os.path.join(test_target_dir, 'Arrow-0.7.1-intel-2017b-Python-3.6.3.eb'))) - self.assertTrue(os.path.exists(os.path.join(test_target_dir, 'bat-0.3.3-intel-2017b-Python-3.6.3.eb'))) + # check that the two easyconfigs exist + self.assertTrue(os.path.exists(os.path.join(test_target_dir, all_ecs_pr8007[0]))) + self.assertTrue(os.path.exists(os.path.join(test_target_dir, all_ecs_pr8007[2]))) + # ...but the patch doesn't + self.assertFalse(os.path.exists(os.path.join(test_target_dir, all_ecs_pr8007[1]))) + remove_dir(test_target_dir) + + # test with only one ec in the PR (final argument is taken as a filename) + test_ec = os.path.join(self.test_prefix, 'test.eb') + args = ['--copy-ec', '--from-pr', '11521', test_ec] + ec_pr11521 = "ExifTool-12.00-GCCcore-9.3.0.eb" + stdout = mocked_main(args) + self.assertEqual(stdout, '%s copied to %s' % (ec_pr11521, test_ec)) + self.assertTrue(os.path.exists(test_ec)) + remove_file(test_ec) # --copy-ec without arguments results in a proper error args = ['--copy-ec'] From f8db57dca8c7ba2899ad26265cbcfdac577a0c98 Mon Sep 17 00:00:00 2001 From: Alan O'Cais Date: Mon, 19 Oct 2020 18:31:28 +0200 Subject: [PATCH 09/51] Address most comments in the review --- easybuild/framework/easyconfig/tools.py | 18 +++++++++++ easybuild/main.py | 43 ++++++++----------------- 2 files changed, 31 insertions(+), 30 deletions(-) diff --git a/easybuild/framework/easyconfig/tools.py b/easybuild/framework/easyconfig/tools.py index 2a3260ae80..51537b4b0d 100644 --- a/easybuild/framework/easyconfig/tools.py +++ b/easybuild/framework/easyconfig/tools.py @@ -728,3 +728,21 @@ def avail_easyblocks(): easyblock_mod_name, easyblocks[easyblock_mod_name]['loc'], path) return easyblocks + + +def copy_ecs_to_target(determined_paths, target_path, prefix=False): + """ + Copy list of easyconfigs to specified path + + :param determined_paths: paths to ecs to copy + :param target_path: target to copy files to + :param prefix: include message prefix characters + """ + if len(determined_paths) == 1: + copy_file(determined_paths[0], target_path) + print_msg("%s copied to %s" % (os.path.basename(determined_paths[0]), target_path), prefix=prefix) + elif len(determined_paths) > 1: + copy_files(determined_paths, target_path) + print_msg("%d file(s) copied to %s" % (len(determined_paths), target_path), prefix=prefix) + else: + raise EasyBuildError("One of more files to copy should be specified!") \ No newline at end of file diff --git a/easybuild/main.py b/easybuild/main.py index a668c5deb7..823346b093 100644 --- a/easybuild/main.py +++ b/easybuild/main.py @@ -50,7 +50,7 @@ from easybuild.framework.easyconfig.easyconfig import clean_up_easyconfigs from easybuild.framework.easyconfig.easyconfig import fix_deprecated_easyconfigs, verify_easyconfig_filename from easybuild.framework.easyconfig.style import cmdline_easyconfigs_style_check -from easybuild.framework.easyconfig.tools import categorize_files_by_type, dep_graph +from easybuild.framework.easyconfig.tools import categorize_files_by_type, copy_ecs_to_target, dep_graph from easybuild.framework.easyconfig.tools import det_easyconfig_paths, dump_env_script, get_paths_for from easybuild.framework.easyconfig.tools import parse_easyconfigs, review_pr, run_contrib_checks, skip_available from easybuild.framework.easyconfig.tweak import obtain_ec_for, tweak @@ -176,24 +176,6 @@ def run_contrib_style_checks(ecs, check_contrib, check_style): return check_contrib or check_style -def copy_ecs_to_target(determined_paths, target_path, prefix=False): - """ - Copy list of easyconfigs to specified path - - :param determined_paths: paths to ecs to copy - :param target_path: target to copy files to - :param prefix: include message prefix characters - """ - if len(determined_paths) == 1: - copy_file(determined_paths[0], target_path) - print_msg("%s copied to %s" % (os.path.basename(determined_paths[0]), target_path), prefix=prefix) - elif len(determined_paths) > 1: - copy_files(determined_paths, target_path) - print_msg("%d file(s) copied to %s" % (len(determined_paths), target_path), prefix=prefix) - else: - raise EasyBuildError("One of more files to copy should be specified!") - - def clean_exit(logfile, tmpdir, testing, silent=False): """Small utility function to perform a clean exit.""" cleanup(logfile, tmpdir, testing, silent=silent) @@ -321,16 +303,17 @@ def main(args=None, logfile=None, do_build=None, testing=False, modtool=None): eb_file = find_easybuild_easyconfig() orig_paths.append(eb_file) - if not orig_paths: - # if no easyconfig files are specified and we are using --from-pr, use current directory as target directory - target_path = os.getcwd() if (options.copy_ec and options.from_pr) else None - elif len(orig_paths) == 1 and not (options.copy_ec and options.from_pr): - # if only one easyconfig file is specified, use current directory as target directory + # if only one easyconfig is specified, or if none are specified and we are using --from-pr, + # use current directory as target directory + if len(orig_paths) == 1 and not (options.copy_ec and options.from_pr): target_path = os.getcwd() elif orig_paths: # last path is target when --copy-ec is used, so remove that from the list - # use the absolute path as some use cases occur in a temporary directory + # use the absolute path as the --from-pr case drops us into a temporary directory target_path = os.path.abspath(orig_paths.pop()) if options.copy_ec else None + else: + # if no easyconfig files are specified and we are using --from-pr, use current directory as target directory + target_path = os.getcwd() if (options.copy_ec and options.from_pr) else None categorized_paths = categorize_files_by_type(orig_paths) @@ -343,8 +326,9 @@ def main(args=None, logfile=None, do_build=None, testing=False, modtool=None): # determine paths to easyconfigs determined_paths = det_easyconfig_paths(categorized_paths['easyconfigs']) - if (options.copy_ec and not (tweaked_ecs_paths or options.from_pr)) or options.fix_deprecated_easyconfigs or \ - options.show_ec: + # only copy easyconfigs here if we're not using --try-* or --from-pr (that's are handled below) + copy_ec = options.copy_ec and not (tweaked_ecs_paths or options.from_pr) + if copy_ec or options.fix_deprecated_easyconfigs or options.show_ec: if options.copy_ec: copy_ecs_to_target(determined_paths, target_path) @@ -373,10 +357,9 @@ def main(args=None, logfile=None, do_build=None, testing=False, modtool=None): log=_log, opt_parser=eb_go.parser, exit_on_error=not testing) _log.debug("Paths: %s", paths) - # if we are only copying ec's for a specific PR we can do that now + # if we are only copying ec's from a specific PR we can do that now if options.copy_ec and options.from_pr: - determined_paths = [path[0] for path in paths] - copy_ecs_to_target(determined_paths, target_path) + copy_ecs_to_target([x for (x, _) in paths], target_path) clean_exit(logfile, eb_tmpdir, testing) # run regtest From b3615f41f6a14eb2daf7f911a5915204db427216 Mon Sep 17 00:00:00 2001 From: Alan O'Cais Date: Mon, 19 Oct 2020 18:35:48 +0200 Subject: [PATCH 10/51] Fix linting --- easybuild/framework/easyconfig/tools.py | 1 + easybuild/main.py | 2 +- 2 files changed, 2 insertions(+), 1 deletion(-) diff --git a/easybuild/framework/easyconfig/tools.py b/easybuild/framework/easyconfig/tools.py index 51537b4b0d..b8bb7197f3 100644 --- a/easybuild/framework/easyconfig/tools.py +++ b/easybuild/framework/easyconfig/tools.py @@ -53,6 +53,7 @@ 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 copy_file, copy_files from easybuild.tools.filetools import find_easyconfigs, is_patch_file, read_file, resolve_path, which, write_file from easybuild.tools.github import fetch_easyconfigs_from_pr, download_repo from easybuild.tools.multidiff import multidiff diff --git a/easybuild/main.py b/easybuild/main.py index 823346b093..eea5c7d0d4 100644 --- a/easybuild/main.py +++ b/easybuild/main.py @@ -57,7 +57,7 @@ from easybuild.tools.config import find_last_log, get_repository, get_repositorypath, build_option from easybuild.tools.containers.common import containerize from easybuild.tools.docs import list_software -from easybuild.tools.filetools import adjust_permissions, cleanup, copy_file, copy_files, dump_index, load_index +from easybuild.tools.filetools import adjust_permissions, cleanup, dump_index, load_index from easybuild.tools.filetools import read_file, register_lock_cleanup_signal_handlers, write_file from easybuild.tools.github import check_github, close_pr, new_branch_github, find_easybuild_easyconfig from easybuild.tools.github import install_github_token, list_prs, new_pr, new_pr_from_branch, merge_pr From 2408626be636079f629c2127d47622594785a145 Mon Sep 17 00:00:00 2001 From: Alan O'Cais Date: Mon, 19 Oct 2020 18:36:46 +0200 Subject: [PATCH 11/51] Fix linting --- easybuild/framework/easyconfig/tools.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/easybuild/framework/easyconfig/tools.py b/easybuild/framework/easyconfig/tools.py index b8bb7197f3..d0b353de94 100644 --- a/easybuild/framework/easyconfig/tools.py +++ b/easybuild/framework/easyconfig/tools.py @@ -746,4 +746,4 @@ def copy_ecs_to_target(determined_paths, target_path, prefix=False): copy_files(determined_paths, target_path) print_msg("%d file(s) copied to %s" % (len(determined_paths), target_path), prefix=prefix) else: - raise EasyBuildError("One of more files to copy should be specified!") \ No newline at end of file + raise EasyBuildError("One of more files to copy should be specified!") From 34cd4a5f451f02cd6da2b7629888ec7dffb0d78f Mon Sep 17 00:00:00 2001 From: Alan O'Cais Date: Tue, 20 Oct 2020 11:08:06 +0200 Subject: [PATCH 12/51] Rework the approach and add additional tests --- easybuild/framework/easyconfig/tools.py | 5 +-- easybuild/main.py | 40 +++++++++++++++------- test/framework/options.py | 44 +++++++++++++++---------- 3 files changed, 58 insertions(+), 31 deletions(-) diff --git a/easybuild/framework/easyconfig/tools.py b/easybuild/framework/easyconfig/tools.py index d0b353de94..ea216f4288 100644 --- a/easybuild/framework/easyconfig/tools.py +++ b/easybuild/framework/easyconfig/tools.py @@ -731,15 +731,16 @@ def avail_easyblocks(): return easyblocks -def copy_ecs_to_target(determined_paths, target_path, prefix=False): +def copy_ecs_to_target(determined_paths, target_path, prefix=False, target_is_dir=False): """ Copy list of easyconfigs to specified path :param determined_paths: paths to ecs to copy :param target_path: target to copy files to :param prefix: include message prefix characters + :param target_is_dir: target is always a directory """ - if len(determined_paths) == 1: + if len(determined_paths) == 1 and not target_is_dir: copy_file(determined_paths[0], target_path) print_msg("%s copied to %s" % (os.path.basename(determined_paths[0]), target_path), prefix=prefix) elif len(determined_paths) > 1: diff --git a/easybuild/main.py b/easybuild/main.py index eea5c7d0d4..a19643f056 100644 --- a/easybuild/main.py +++ b/easybuild/main.py @@ -39,6 +39,7 @@ import os import stat import sys +import tempfile import traceback # IMPORTANT this has to be the first easybuild import as it customises the logging @@ -59,8 +60,9 @@ from easybuild.tools.docs import list_software from easybuild.tools.filetools import adjust_permissions, cleanup, dump_index, load_index from easybuild.tools.filetools import read_file, register_lock_cleanup_signal_handlers, write_file -from easybuild.tools.github import check_github, close_pr, new_branch_github, find_easybuild_easyconfig -from easybuild.tools.github import install_github_token, list_prs, new_pr, new_pr_from_branch, merge_pr +from easybuild.tools.github import check_github, close_pr, fetch_files_from_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 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 from easybuild.tools.modules import modules_tool @@ -310,7 +312,7 @@ def main(args=None, logfile=None, do_build=None, testing=False, modtool=None): elif orig_paths: # last path is target when --copy-ec is used, so remove that from the list # use the absolute path as the --from-pr case drops us into a temporary directory - target_path = os.path.abspath(orig_paths.pop()) if options.copy_ec else None + target_path = orig_paths.pop() if options.copy_ec else None else: # if no easyconfig files are specified and we are using --from-pr, use current directory as target directory target_path = os.getcwd() if (options.copy_ec and options.from_pr) else None @@ -326,9 +328,28 @@ def main(args=None, logfile=None, do_build=None, testing=False, modtool=None): # determine paths to easyconfigs determined_paths = det_easyconfig_paths(categorized_paths['easyconfigs']) - # only copy easyconfigs here if we're not using --try-* or --from-pr (that's are handled below) - copy_ec = options.copy_ec and not (tweaked_ecs_paths or options.from_pr) + # only copy easyconfigs here if we're not using --try-* (that's are handled below) + copy_ec = options.copy_ec and not tweaked_ecs_paths if copy_ec or options.fix_deprecated_easyconfigs or options.show_ec: + if options.from_pr: + # if we are using from pr we also want any (related) files that are not easyconfigs + pr_paths = fetch_files_from_pr(pr=options.from_pr, path=tempfile.mktemp()) + for pr_path in pr_paths: + # we assumed that the last argument from the command line was the target_path but if it appears in the + # PR file list then it was most likely intended to use the CWD and (also) copy that particular file + if target_path == os.path.basename(pr_path): + determined_paths.append(pr_path) + target_path = os.getcwd() + other_pr_paths = [] + for ec_path in determined_paths: + for pr_path in pr_paths: + if os.path.basename(ec_path) == os.path.basename(pr_path): + # Search for any associated patches (they would have the same dirname) + for patch_path in pr_paths: + if pr_path != patch_path and os.path.dirname(pr_path) == os.path.dirname(patch_path): + other_pr_paths.append(patch_path) + if other_pr_paths: + determined_paths += other_pr_paths if options.copy_ec: copy_ecs_to_target(determined_paths, target_path) @@ -357,16 +378,11 @@ def main(args=None, logfile=None, do_build=None, testing=False, modtool=None): log=_log, opt_parser=eb_go.parser, exit_on_error=not testing) _log.debug("Paths: %s", paths) - # if we are only copying ec's from a specific PR we can do that now - if options.copy_ec and options.from_pr: - copy_ecs_to_target([x for (x, _) in paths], target_path) - clean_exit(logfile, eb_tmpdir, testing) - # run regtest if options.regtest or options.aggregate_regtest: _log.info("Running regression test") # fallback: easybuild-easyconfigs install path - regtest_ok = regtest([path[0] for path in paths] or easyconfigs_pkg_paths, modtool) + regtest_ok = regtest([x for (x, _) in paths] or easyconfigs_pkg_paths, modtool) if not regtest_ok: _log.info("Regression test failed (partially)!") sys.exit(31) # exit -> 3x1t -> 31 @@ -434,7 +450,7 @@ def main(args=None, logfile=None, do_build=None, testing=False, modtool=None): if tweaked_ecs_in_all_ecs: # Clean them, then copy them clean_up_easyconfigs(tweaked_ecs_in_all_ecs) - copy_ecs_to_target(tweaked_ecs_in_all_ecs, target_path) + copy_ecs_to_target(tweaked_ecs_in_all_ecs, target_path, target_is_dir=True) clean_exit(logfile, eb_tmpdir, testing) # creating/updating PRs diff --git a/test/framework/options.py b/test/framework/options.py index 9ef1ab6035..b4cf7907b4 100644 --- a/test/framework/options.py +++ b/test/framework/options.py @@ -975,7 +975,7 @@ def mocked_main(args): stderr, stdout = self.get_stderr(), self.get_stdout() self.mock_stderr(False) self.mock_stdout(False) - self.assertEqual(stderr, '') + # self.assertEqual(stderr, '') return stdout.strip() topdir = os.path.dirname(os.path.abspath(__file__)) @@ -1002,7 +1002,7 @@ def mocked_main(args): args = ['--copy-ec', 'toy-0.0.eb', target_fn] stdout = mocked_main(args) - self.assertEqual(stdout, 'toy-0.0.eb copied to %s/test.eb' % self.test_prefix) + self.assertEqual(stdout, 'toy-0.0.eb copied to test.eb') change_dir(cwd) @@ -1048,7 +1048,7 @@ def check_copied_files(): self.assertFalse(os.path.exists(args[-1])) stdout = mocked_main(args) - self.assertEqual(stdout, '2 file(s) copied to %s' % test_target_dir) + self.assertEqual(stdout, '2 file(s) copied to test_target_dir') check_copied_files() @@ -1085,27 +1085,37 @@ def check_copied_files(): change_dir(test_working_dir) args = ['--copy-ec', '--from-pr', '8007'] stdout = mocked_main(args) - self.assertEqual(stdout, '2 file(s) copied to %s' % test_working_dir) - # check that the two easyconfigs exist - self.assertTrue(os.path.exists(os.path.join(test_working_dir, all_ecs_pr8007[0]))) - remove_file(os.path.join(test_working_dir, all_ecs_pr8007[0])) - self.assertTrue(os.path.exists(os.path.join(test_working_dir, all_ecs_pr8007[2]))) - remove_file(os.path.join(test_working_dir, all_ecs_pr8007[2])) - # ...but the patch doesn't - self.assertFalse(os.path.exists(os.path.join(test_working_dir, all_ecs_pr8007[1]))) - remove_file(os.path.join(test_working_dir, all_ecs_pr8007[1])) + self.assertEqual(stdout, '3 file(s) copied to %s' % test_working_dir) + # check that the files exist + for pr_file in all_ecs_pr8007: + self.assertTrue(os.path.exists(os.path.join(test_working_dir, pr_file))) + remove_file(os.path.join(test_working_dir, pr_file)) # copying multiple easyconfig files to a non-existing target directory (which is created automatically) args = ['--copy-ec', '--from-pr', '8007', test_target_dir] stdout = mocked_main(args) + self.assertEqual(stdout, '3 file(s) copied to %s' % test_target_dir) + for pr_file in all_ecs_pr8007: + self.assertTrue(os.path.exists(os.path.join(test_target_dir, pr_file))) + remove_dir(test_target_dir) + + # test where we select a single file from a PR but also has a patch file + args = ['--copy-ec', '--from-pr', '8007', 'bat-0.3.3-intel-2017b-Python-3.6.3.eb', test_target_dir] + stdout = mocked_main(args) self.assertEqual(stdout, '2 file(s) copied to %s' % test_target_dir) - # check that the two easyconfigs exist - self.assertTrue(os.path.exists(os.path.join(test_target_dir, all_ecs_pr8007[0]))) - self.assertTrue(os.path.exists(os.path.join(test_target_dir, all_ecs_pr8007[2]))) - # ...but the patch doesn't - self.assertFalse(os.path.exists(os.path.join(test_target_dir, all_ecs_pr8007[1]))) + for pr_file in ['bat-0.3.3-fix-pyspark.patch', 'bat-0.3.3-intel-2017b-Python-3.6.3.eb']: + self.assertTrue(os.path.exists(os.path.join(test_target_dir, pr_file))) remove_dir(test_target_dir) + # test the same thing but where we don't provide a target directory + args = ['--copy-ec', '--from-pr', '8007', 'bat-0.3.3-intel-2017b-Python-3.6.3.eb'] + stdout = mocked_main(args) + self.assertEqual(stdout, '2 file(s) copied to %s' % test_working_dir) + for pr_file in ['bat-0.3.3-fix-pyspark.patch', 'bat-0.3.3-intel-2017b-Python-3.6.3.eb']: + path = os.path.join(test_working_dir, pr_file) + self.assertTrue(os.path.exists(path)) + remove_file(path) + # test with only one ec in the PR (final argument is taken as a filename) test_ec = os.path.join(self.test_prefix, 'test.eb') args = ['--copy-ec', '--from-pr', '11521', test_ec] From f821af19323e8ca33ec8cc1134d4389082ea86c5 Mon Sep 17 00:00:00 2001 From: Alan O'Cais Date: Tue, 20 Oct 2020 11:08:36 +0200 Subject: [PATCH 13/51] Rework the approach and add additional tests --- test/framework/options.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/test/framework/options.py b/test/framework/options.py index b4cf7907b4..ec8e41f162 100644 --- a/test/framework/options.py +++ b/test/framework/options.py @@ -975,7 +975,7 @@ def mocked_main(args): stderr, stdout = self.get_stderr(), self.get_stdout() self.mock_stderr(False) self.mock_stdout(False) - # self.assertEqual(stderr, '') + self.assertEqual(stderr, '') return stdout.strip() topdir = os.path.dirname(os.path.abspath(__file__)) From 33d8570b515c801e866644fa15f2c7b2b1f74a59 Mon Sep 17 00:00:00 2001 From: Alan O'Cais Date: Tue, 20 Oct 2020 11:11:41 +0200 Subject: [PATCH 14/51] Fix comment --- easybuild/main.py | 1 - 1 file changed, 1 deletion(-) diff --git a/easybuild/main.py b/easybuild/main.py index a19643f056..c616dd1573 100644 --- a/easybuild/main.py +++ b/easybuild/main.py @@ -311,7 +311,6 @@ def main(args=None, logfile=None, do_build=None, testing=False, modtool=None): target_path = os.getcwd() elif orig_paths: # last path is target when --copy-ec is used, so remove that from the list - # use the absolute path as the --from-pr case drops us into a temporary directory target_path = orig_paths.pop() if options.copy_ec else None else: # if no easyconfig files are specified and we are using --from-pr, use current directory as target directory From 3980dc9c01d3911ba75518089f9496a89681ad22 Mon Sep 17 00:00:00 2001 From: Alan O'Cais Date: Tue, 20 Oct 2020 11:19:07 +0200 Subject: [PATCH 15/51] Be a little more careful with patches --- easybuild/main.py | 9 +++++---- 1 file changed, 5 insertions(+), 4 deletions(-) diff --git a/easybuild/main.py b/easybuild/main.py index c616dd1573..df5396920b 100644 --- a/easybuild/main.py +++ b/easybuild/main.py @@ -331,7 +331,7 @@ def main(args=None, logfile=None, do_build=None, testing=False, modtool=None): copy_ec = options.copy_ec and not tweaked_ecs_paths if copy_ec or options.fix_deprecated_easyconfigs or options.show_ec: if options.from_pr: - # if we are using from pr we also want any (related) files that are not easyconfigs + # pull in the paths to all the changed files in the PR (need to do this in a new temp dir) pr_paths = fetch_files_from_pr(pr=options.from_pr, path=tempfile.mktemp()) for pr_path in pr_paths: # we assumed that the last argument from the command line was the target_path but if it appears in the @@ -346,9 +346,10 @@ def main(args=None, logfile=None, do_build=None, testing=False, modtool=None): # Search for any associated patches (they would have the same dirname) for patch_path in pr_paths: if pr_path != patch_path and os.path.dirname(pr_path) == os.path.dirname(patch_path): - other_pr_paths.append(patch_path) - if other_pr_paths: - determined_paths += other_pr_paths + # if it's an easyconfig, we already have it covered + if not patch_path.endswith('.eb'): + other_pr_paths.append(patch_path) + determined_paths += other_pr_paths if options.copy_ec: copy_ecs_to_target(determined_paths, target_path) From f7aa64619afd7bbefbd96e2d974b281b50128444 Mon Sep 17 00:00:00 2001 From: Alan O'Cais Date: Tue, 20 Oct 2020 11:21:45 +0200 Subject: [PATCH 16/51] Retain old behaviour when using try-* --- easybuild/main.py | 1 - 1 file changed, 1 deletion(-) diff --git a/easybuild/main.py b/easybuild/main.py index df5396920b..fd319107f9 100644 --- a/easybuild/main.py +++ b/easybuild/main.py @@ -451,7 +451,6 @@ def main(args=None, logfile=None, do_build=None, testing=False, modtool=None): # Clean them, then copy them clean_up_easyconfigs(tweaked_ecs_in_all_ecs) copy_ecs_to_target(tweaked_ecs_in_all_ecs, target_path, target_is_dir=True) - clean_exit(logfile, eb_tmpdir, testing) # creating/updating PRs if pr_options: From 11f5a48f754fab0d4107c010e56a912227c3c46b Mon Sep 17 00:00:00 2001 From: Alan O'Cais Date: Tue, 20 Oct 2020 11:27:16 +0200 Subject: [PATCH 17/51] Tidy up the new tests --- test/framework/options.py | 13 +++++++++---- 1 file changed, 9 insertions(+), 4 deletions(-) diff --git a/test/framework/options.py b/test/framework/options.py index ec8e41f162..6eefe26313 100644 --- a/test/framework/options.py +++ b/test/framework/options.py @@ -1073,6 +1073,14 @@ def check_copied_files(): self.assertEqual(read_file(copied_toy_cwd), toy_ec_txt) # Test --copy-ec coupled with --from-pr + + test_working_dir = os.path.join(self.test_prefix, 'test_working_dir') + mkdir(test_working_dir) + change_dir(test_working_dir) + test_target_dir = os.path.join(self.test_prefix, 'test_target_dir') + # Make sure the test target directory doesn't exist + remove_dir(test_target_dir) + all_ecs_pr8007 = [ 'Arrow-0.7.1-intel-2017b-Python-3.6.3.eb', 'bat-0.3.3-fix-pyspark.patch', @@ -1080,9 +1088,6 @@ def check_copied_files(): ] # test use of `--copy-ec` with `--from-pr` to the cwd - test_working_dir = os.path.join(self.test_prefix, 'test_working_dir') - mkdir(test_working_dir) - change_dir(test_working_dir) args = ['--copy-ec', '--from-pr', '8007'] stdout = mocked_main(args) self.assertEqual(stdout, '3 file(s) copied to %s' % test_working_dir) @@ -1125,7 +1130,7 @@ def check_copied_files(): self.assertTrue(os.path.exists(test_ec)) remove_file(test_ec) - # --copy-ec without arguments results in a proper error + # --copy-ec without arguments (and no --from-pr) results in a proper error args = ['--copy-ec'] error_pattern = "One of more files to copy should be specified!" self.assertErrorRegex(EasyBuildError, error_pattern, self.eb_main, args, raise_error=True) From 01412431cc0fd0c57cd0c69ce3940fbbf0313c4c Mon Sep 17 00:00:00 2001 From: Alan O'Cais Date: Tue, 20 Oct 2020 11:32:17 +0200 Subject: [PATCH 18/51] Keep our patch list unique --- easybuild/main.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/easybuild/main.py b/easybuild/main.py index fd319107f9..eaf46e3e6f 100644 --- a/easybuild/main.py +++ b/easybuild/main.py @@ -347,7 +347,7 @@ def main(args=None, logfile=None, do_build=None, testing=False, modtool=None): for patch_path in pr_paths: if pr_path != patch_path and os.path.dirname(pr_path) == os.path.dirname(patch_path): # if it's an easyconfig, we already have it covered - if not patch_path.endswith('.eb'): + if not patch_path.endswith('.eb') and patch_path not in other_pr_paths: other_pr_paths.append(patch_path) determined_paths += other_pr_paths From e61e72d9b9dc3baa582d67ffc8941811ee846cae Mon Sep 17 00:00:00 2001 From: Alan O'Cais Date: Tue, 20 Oct 2020 11:34:49 +0200 Subject: [PATCH 19/51] Fix copy_ecs_to_target for case where I want to copy a single file to a directory --- easybuild/framework/easyconfig/tools.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/easybuild/framework/easyconfig/tools.py b/easybuild/framework/easyconfig/tools.py index ea216f4288..128d5eddcc 100644 --- a/easybuild/framework/easyconfig/tools.py +++ b/easybuild/framework/easyconfig/tools.py @@ -743,7 +743,7 @@ def copy_ecs_to_target(determined_paths, target_path, prefix=False, target_is_di if len(determined_paths) == 1 and not target_is_dir: copy_file(determined_paths[0], target_path) print_msg("%s copied to %s" % (os.path.basename(determined_paths[0]), target_path), prefix=prefix) - elif len(determined_paths) > 1: + elif determined_paths: copy_files(determined_paths, target_path) print_msg("%d file(s) copied to %s" % (len(determined_paths), target_path), prefix=prefix) else: From 30145e48b3ae5351420813cc62bae4b911d00071 Mon Sep 17 00:00:00 2001 From: Alan O'Cais Date: Tue, 20 Oct 2020 12:03:48 +0200 Subject: [PATCH 20/51] Fix broken test --- test/framework/options.py | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/test/framework/options.py b/test/framework/options.py index 6eefe26313..69dd910c22 100644 --- a/test/framework/options.py +++ b/test/framework/options.py @@ -2125,15 +2125,15 @@ def test_try_with_copy(self): self.mock_stdout(True) self.mock_stderr(True) - tweaked_ecs_dir = os.path.join(self.test_buildpath) - os.chdir(tweaked_ecs_dir) - self.eb_main(args + ['--try-software=foo,1.2.3', '--try-toolchain=gompi,2018a'], verbose=True, raise_error=True) + tweaked_ecs_dir = os.path.join(self.test_buildpath, 'my_tweaked_ecs') + self.eb_main(args + ['--try-software=foo,1.2.3', '--try-toolchain=gompi,2018a', tweaked_ecs_dir], + verbose=True, raise_error=True) outtxt = self.get_stdout() errtxt = self.get_stderr() - self.mock_stdout(False) - self.mock_stderr(False) self.assertTrue(r'foo-1.2.3-GCC-6.4.0-2.28.eb copied to ' + tweaked_ecs_dir in outtxt) self.assertFalse(errtxt) + self.mock_stdout(False) + self.mock_stderr(False) self.assertTrue( os.path.exists(os.path.join(self.test_buildpath, tweaked_ecs_dir, 'foo-1.2.3-GCC-6.4.0-2.28.eb')) ) From 49b4f475f7546ab54781c5116547ce8b11fbae0c Mon Sep 17 00:00:00 2001 From: Alan O'Cais Date: Tue, 20 Oct 2020 12:04:49 +0200 Subject: [PATCH 21/51] Fix broken test --- test/framework/options.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/test/framework/options.py b/test/framework/options.py index 69dd910c22..c4a0c49203 100644 --- a/test/framework/options.py +++ b/test/framework/options.py @@ -2130,7 +2130,7 @@ def test_try_with_copy(self): verbose=True, raise_error=True) outtxt = self.get_stdout() errtxt = self.get_stderr() - self.assertTrue(r'foo-1.2.3-GCC-6.4.0-2.28.eb copied to ' + tweaked_ecs_dir in outtxt) + self.assertTrue(r'1 file(s) copied to ' + tweaked_ecs_dir in outtxt) self.assertFalse(errtxt) self.mock_stdout(False) self.mock_stderr(False) From 1a8eecf0625c42e871f440590b3da16edccc9917 Mon Sep 17 00:00:00 2001 From: Alan O'Cais Date: Tue, 20 Oct 2020 12:17:41 +0200 Subject: [PATCH 22/51] Fix broken test --- easybuild/main.py | 4 +++- test/framework/options.py | 2 +- 2 files changed, 4 insertions(+), 2 deletions(-) diff --git a/easybuild/main.py b/easybuild/main.py index eaf46e3e6f..8363a5a303 100644 --- a/easybuild/main.py +++ b/easybuild/main.py @@ -337,7 +337,9 @@ def main(args=None, logfile=None, do_build=None, testing=False, modtool=None): # we assumed that the last argument from the command line was the target_path but if it appears in the # PR file list then it was most likely intended to use the CWD and (also) copy that particular file if target_path == os.path.basename(pr_path): - determined_paths.append(pr_path) + if not orig_paths: + # It should have been the only easyconfig selected + determined_paths = [pr_path] target_path = os.getcwd() other_pr_paths = [] for ec_path in determined_paths: diff --git a/test/framework/options.py b/test/framework/options.py index c4a0c49203..9fb15b30b6 100644 --- a/test/framework/options.py +++ b/test/framework/options.py @@ -975,7 +975,7 @@ def mocked_main(args): stderr, stdout = self.get_stderr(), self.get_stdout() self.mock_stderr(False) self.mock_stdout(False) - self.assertEqual(stderr, '') + # self.assertEqual(stderr, '') return stdout.strip() topdir = os.path.dirname(os.path.abspath(__file__)) From 380387b4427efc4ea58435888724e88fbd900be0 Mon Sep 17 00:00:00 2001 From: Alan O'Cais Date: Tue, 20 Oct 2020 12:21:41 +0200 Subject: [PATCH 23/51] Fix broken test --- test/framework/options.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/test/framework/options.py b/test/framework/options.py index 9fb15b30b6..c4a0c49203 100644 --- a/test/framework/options.py +++ b/test/framework/options.py @@ -975,7 +975,7 @@ def mocked_main(args): stderr, stdout = self.get_stderr(), self.get_stdout() self.mock_stderr(False) self.mock_stdout(False) - # self.assertEqual(stderr, '') + self.assertEqual(stderr, '') return stdout.strip() topdir = os.path.dirname(os.path.abspath(__file__)) From eedbfd10af99b8789bda68c6b6eb7551e237337f Mon Sep 17 00:00:00 2001 From: Alan O'Cais Date: Tue, 20 Oct 2020 12:25:51 +0200 Subject: [PATCH 24/51] Fix broken test --- easybuild/main.py | 3 +++ 1 file changed, 3 insertions(+) diff --git a/easybuild/main.py b/easybuild/main.py index 8363a5a303..4aef13249f 100644 --- a/easybuild/main.py +++ b/easybuild/main.py @@ -340,6 +340,9 @@ def main(args=None, logfile=None, do_build=None, testing=False, modtool=None): if not orig_paths: # It should have been the only easyconfig selected determined_paths = [pr_path] + else: + if os.path.basename(pr_path) not in [os.path.basename(path) for path in determined_paths]: + determined_paths.append(pr_path) target_path = os.getcwd() other_pr_paths = [] for ec_path in determined_paths: From ae9e0ea8b5f4dd41f4d78267ab5dee03a402fde7 Mon Sep 17 00:00:00 2001 From: Alan O'Cais Date: Tue, 20 Oct 2020 12:40:56 +0200 Subject: [PATCH 25/51] Only grab the current working directory once at the beginning. --- easybuild/main.py | 7 ++++--- test/framework/options.py | 2 +- 2 files changed, 5 insertions(+), 4 deletions(-) diff --git a/easybuild/main.py b/easybuild/main.py index 4aef13249f..14d5bf7425 100644 --- a/easybuild/main.py +++ b/easybuild/main.py @@ -307,14 +307,15 @@ def main(args=None, logfile=None, do_build=None, testing=False, modtool=None): # if only one easyconfig is specified, or if none are specified and we are using --from-pr, # use current directory as target directory + cwd = os.getcwd() if len(orig_paths) == 1 and not (options.copy_ec and options.from_pr): - target_path = os.getcwd() + target_path = cwd elif orig_paths: # last path is target when --copy-ec is used, so remove that from the list target_path = orig_paths.pop() if options.copy_ec else None else: # if no easyconfig files are specified and we are using --from-pr, use current directory as target directory - target_path = os.getcwd() if (options.copy_ec and options.from_pr) else None + target_path = cwd if (options.copy_ec and options.from_pr) else None categorized_paths = categorize_files_by_type(orig_paths) @@ -343,7 +344,7 @@ def main(args=None, logfile=None, do_build=None, testing=False, modtool=None): else: if os.path.basename(pr_path) not in [os.path.basename(path) for path in determined_paths]: determined_paths.append(pr_path) - target_path = os.getcwd() + target_path = cwd other_pr_paths = [] for ec_path in determined_paths: for pr_path in pr_paths: diff --git a/test/framework/options.py b/test/framework/options.py index c4a0c49203..9fb15b30b6 100644 --- a/test/framework/options.py +++ b/test/framework/options.py @@ -975,7 +975,7 @@ def mocked_main(args): stderr, stdout = self.get_stderr(), self.get_stdout() self.mock_stderr(False) self.mock_stdout(False) - self.assertEqual(stderr, '') + # self.assertEqual(stderr, '') return stdout.strip() topdir = os.path.dirname(os.path.abspath(__file__)) From c5560c474b2031300ce2bdbc746fbe51d28360ec Mon Sep 17 00:00:00 2001 From: Alan O'Cais Date: Tue, 20 Oct 2020 12:51:40 +0200 Subject: [PATCH 26/51] Fix GitHub tools leaving you in temporary directory --- easybuild/main.py | 7 +++---- easybuild/tools/github.py | 4 ++++ 2 files changed, 7 insertions(+), 4 deletions(-) diff --git a/easybuild/main.py b/easybuild/main.py index 14d5bf7425..4aef13249f 100644 --- a/easybuild/main.py +++ b/easybuild/main.py @@ -307,15 +307,14 @@ def main(args=None, logfile=None, do_build=None, testing=False, modtool=None): # if only one easyconfig is specified, or if none are specified and we are using --from-pr, # use current directory as target directory - cwd = os.getcwd() if len(orig_paths) == 1 and not (options.copy_ec and options.from_pr): - target_path = cwd + target_path = os.getcwd() elif orig_paths: # last path is target when --copy-ec is used, so remove that from the list target_path = orig_paths.pop() if options.copy_ec else None else: # if no easyconfig files are specified and we are using --from-pr, use current directory as target directory - target_path = cwd if (options.copy_ec and options.from_pr) else None + target_path = os.getcwd() if (options.copy_ec and options.from_pr) else None categorized_paths = categorize_files_by_type(orig_paths) @@ -344,7 +343,7 @@ def main(args=None, logfile=None, do_build=None, testing=False, modtool=None): else: if os.path.basename(pr_path) not in [os.path.basename(path) for path in determined_paths]: determined_paths.append(pr_path) - target_path = cwd + target_path = os.getcwd() other_pr_paths = [] for ec_path in determined_paths: for pr_path in pr_paths: diff --git a/easybuild/tools/github.py b/easybuild/tools/github.py index d1d9df5290..928c33fbe7 100644 --- a/easybuild/tools/github.py +++ b/easybuild/tools/github.py @@ -364,6 +364,7 @@ def download_repo(repo=GITHUB_EASYCONFIGS_REPO, branch='master', account=GITHUB_ _log.debug("%s downloaded to %s, extracting now" % (base_name, path)) base_dir = extract_file(target_path, path, forced=True, change_into_dir=False) + working_directory = os.getcwd() change_dir(base_dir) extracted_path = os.path.join(base_dir, extracted_dir_name) @@ -373,6 +374,9 @@ def download_repo(repo=GITHUB_EASYCONFIGS_REPO, branch='master', account=GITHUB_ write_file(latest_sha_path, latest_commit_sha, forced=True) + # go back to previous working directory + change_dir(working_directory) + _log.debug("Repo %s at branch %s extracted into %s" % (repo, branch, extracted_path)) return extracted_path From 26d91152662a67ffe436b5558b3e5b8bf1f17ad1 Mon Sep 17 00:00:00 2001 From: Alan O'Cais Date: Tue, 20 Oct 2020 12:51:53 +0200 Subject: [PATCH 27/51] Fix GitHub tools leaving you in temporary directory --- test/framework/options.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/test/framework/options.py b/test/framework/options.py index 9fb15b30b6..c4a0c49203 100644 --- a/test/framework/options.py +++ b/test/framework/options.py @@ -975,7 +975,7 @@ def mocked_main(args): stderr, stdout = self.get_stderr(), self.get_stdout() self.mock_stderr(False) self.mock_stdout(False) - # self.assertEqual(stderr, '') + self.assertEqual(stderr, '') return stdout.strip() topdir = os.path.dirname(os.path.abspath(__file__)) From 7944c0a3e418b9a112b42390a1067a14c2569b11 Mon Sep 17 00:00:00 2001 From: Alan O'Cais Date: Tue, 20 Oct 2020 13:07:56 +0200 Subject: [PATCH 28/51] See if starting in the right directory makes a difference --- test/framework/options.py | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/test/framework/options.py b/test/framework/options.py index c4a0c49203..3bb150f21a 100644 --- a/test/framework/options.py +++ b/test/framework/options.py @@ -975,7 +975,7 @@ def mocked_main(args): stderr, stdout = self.get_stderr(), self.get_stdout() self.mock_stderr(False) self.mock_stdout(False) - self.assertEqual(stderr, '') + # self.assertEqual(stderr, '') return stdout.strip() topdir = os.path.dirname(os.path.abspath(__file__)) @@ -1113,6 +1113,7 @@ def check_copied_files(): remove_dir(test_target_dir) # test the same thing but where we don't provide a target directory + change_dir(test_working_dir) args = ['--copy-ec', '--from-pr', '8007', 'bat-0.3.3-intel-2017b-Python-3.6.3.eb'] stdout = mocked_main(args) self.assertEqual(stdout, '2 file(s) copied to %s' % test_working_dir) From 6e5be8deac0b636edbc9eccb88f11c62c90db05b Mon Sep 17 00:00:00 2001 From: Alan O'Cais Date: Tue, 20 Oct 2020 13:11:45 +0200 Subject: [PATCH 29/51] Final fix on broken test --- test/framework/options.py | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/test/framework/options.py b/test/framework/options.py index 3bb150f21a..daaff28863 100644 --- a/test/framework/options.py +++ b/test/framework/options.py @@ -975,7 +975,7 @@ def mocked_main(args): stderr, stdout = self.get_stderr(), self.get_stdout() self.mock_stderr(False) self.mock_stdout(False) - # self.assertEqual(stderr, '') + self.assertEqual(stderr, '') return stdout.strip() topdir = os.path.dirname(os.path.abspath(__file__)) @@ -1076,7 +1076,6 @@ def check_copied_files(): test_working_dir = os.path.join(self.test_prefix, 'test_working_dir') mkdir(test_working_dir) - change_dir(test_working_dir) test_target_dir = os.path.join(self.test_prefix, 'test_target_dir') # Make sure the test target directory doesn't exist remove_dir(test_target_dir) @@ -1088,6 +1087,7 @@ def check_copied_files(): ] # test use of `--copy-ec` with `--from-pr` to the cwd + change_dir(test_working_dir) args = ['--copy-ec', '--from-pr', '8007'] stdout = mocked_main(args) self.assertEqual(stdout, '3 file(s) copied to %s' % test_working_dir) From 1758308460ce7dfb50c19e6d45618121c8dc47d5 Mon Sep 17 00:00:00 2001 From: Kenneth Hoste Date: Sat, 24 Oct 2020 11:16:23 +0200 Subject: [PATCH 30/51] use decorator to cache result of fetch_files_from_pr --- easybuild/tools/github.py | 46 +++++++++++++++++++++++++++++++-------- 1 file changed, 37 insertions(+), 9 deletions(-) diff --git a/easybuild/tools/github.py b/easybuild/tools/github.py index 928c33fbe7..8d7dac059a 100644 --- a/easybuild/tools/github.py +++ b/easybuild/tools/github.py @@ -33,6 +33,7 @@ import copy import getpass import glob +import functools import os import random import re @@ -364,8 +365,7 @@ def download_repo(repo=GITHUB_EASYCONFIGS_REPO, branch='master', account=GITHUB_ _log.debug("%s downloaded to %s, extracting now" % (base_name, path)) base_dir = extract_file(target_path, path, forced=True, change_into_dir=False) - working_directory = os.getcwd() - change_dir(base_dir) + cwd = change_dir(base_dir) extracted_path = os.path.join(base_dir, extracted_dir_name) # check if extracted_path exists @@ -375,22 +375,40 @@ def download_repo(repo=GITHUB_EASYCONFIGS_REPO, branch='master', account=GITHUB_ write_file(latest_sha_path, latest_commit_sha, forced=True) # go back to previous working directory - change_dir(working_directory) + change_dir(cwd) _log.debug("Repo %s at branch %s extracted into %s" % (repo, branch, extracted_path)) return extracted_path -def fetch_easyblocks_from_pr(pr, path=None, github_user=None): - """Fetch patched easyconfig files for a particular PR.""" - return fetch_files_from_pr(pr, path, github_user, github_repo=GITHUB_EASYBLOCKS_REPO) +def pr_files_cache(func): + """ + Decorator to cache result of fetch_files_from_pr. + """ + cache = {} + @functools.wraps(func) + def cache_aware_func(pr, *args, **kwargs): + """Retrieve cached resul, or fetch files from PR & cache result.""" + # cache key is combination of all function arguments (incl. optional ones) + key = tuple([pr] + [kwargs[key] for key in sorted(kwargs.keys())]) -def fetch_easyconfigs_from_pr(pr, path=None, github_user=None): - """Fetch patched easyconfig files for a particular PR.""" - return fetch_files_from_pr(pr, path, github_user, github_repo=GITHUB_EASYCONFIGS_REPO) + if key in cache: + _log.debug("Using cached value for fetch_files_from_pr for PR #%s (%s)", pr, kwargs) + return cache[key] + else: + res = func(pr, *args, **kwargs) + cache[key] = res + return res + + # expose clear/update methods of cache to wrapped function + cache_aware_func.clear_cache = cache.clear + cache_aware_func.update_cache = cache.update + + return cache_aware_func +@pr_files_cache def fetch_files_from_pr(pr, path=None, github_user=None, github_repo=None): """Fetch patched files for a particular PR.""" @@ -500,6 +518,16 @@ def fetch_files_from_pr(pr, path=None, github_user=None, github_repo=None): return files +def fetch_easyblocks_from_pr(pr, path=None, github_user=None): + """Fetch patched easyconfig files for a particular PR.""" + return fetch_files_from_pr(pr, path, github_user, github_repo=GITHUB_EASYBLOCKS_REPO) + + +def fetch_easyconfigs_from_pr(pr, path=None, github_user=None): + """Fetch patched easyconfig files for a particular PR.""" + return fetch_files_from_pr(pr, path, github_user, github_repo=GITHUB_EASYCONFIGS_REPO) + + def create_gist(txt, fn, descr=None, github_user=None, github_token=None): """Create a gist with the provided text.""" From 7bef831e27c473a3d098c93b9f7c68b6d00cefd3 Mon Sep 17 00:00:00 2001 From: Kenneth Hoste Date: Sat, 24 Oct 2020 12:06:28 +0200 Subject: [PATCH 31/51] add locate_files function to easybuild.tools.filetools --- easybuild/tools/filetools.py | 43 +++++++++++++++++++++ test/framework/filetools.py | 73 ++++++++++++++++++++++++++++++++++++ 2 files changed, 116 insertions(+) diff --git a/easybuild/tools/filetools.py b/easybuild/tools/filetools.py index 1c3dbf5f5b..1975ac780d 100644 --- a/easybuild/tools/filetools.py +++ b/easybuild/tools/filetools.py @@ -792,6 +792,49 @@ def find_easyconfigs(path, ignore_dirs=None): return files +def locate_files(files, paths, ignore_subdirs=None): + """ + Determine full path for list of files, in given list of paths (directories). + """ + # determine which easyconfigs files need to be found, if any + files_to_find = [] + for idx, ec_file in enumerate(files): + if ec_file == os.path.basename(ec_file) and not os.path.exists(ec_file): + files_to_find.append((idx, ec_file)) + _log.debug("List of files to find: %s" % files_to_find) + + # find missing easyconfigs by walking paths in robot search path + for path in paths: + _log.debug("Looking for missing files (%d left) in %s..." % (len(files_to_find), path)) + for (subpath, dirnames, filenames) in os.walk(path, topdown=True): + for idx, orig_path in files_to_find[:]: + if orig_path in filenames: + full_path = os.path.join(subpath, orig_path) + _log.info("Found %s in %s: %s" % (orig_path, path, full_path)) + files[idx] = full_path + # if file was found, stop looking for it (first hit wins) + files_to_find.remove((idx, orig_path)) + + # stop os.walk insanity as soon as we have all we need (os.walk loop) + if not files_to_find: + break + + # ignore specified subdirectories + if ignore_subdirs: + dirnames[:] = [d for d in dirnames if d not in ignore_subdirs] + + # stop os.walk insanity as soon as we have all we need (outer loop) + if not files_to_find: + break + + if files_to_find: + filenames = ', '.join([f for (_, f) in files_to_find]) + paths = ', '.join(paths) + raise EasyBuildError("One or more files not found: %s (search paths: %s)", filenames, paths) + + return [os.path.abspath(f) for f in files] + + def find_glob_pattern(glob_pattern, fail_on_no_match=True): """Find unique file/dir matching glob_pattern (raises error if more than one match is found)""" if build_option('extended_dry_run'): diff --git a/test/framework/filetools.py b/test/framework/filetools.py index 8c5a572543..8081aef00c 100644 --- a/test/framework/filetools.py +++ b/test/framework/filetools.py @@ -2680,6 +2680,79 @@ def test_locks(self): self.assertFalse(os.path.exists(lock_path)) self.assertEqual(os.listdir(locks_dir), []) + def test_locate_files(self): + """Test locate_files function.""" + + # create some files to find + one = os.path.join(self.test_prefix, '1.txt') + ft.write_file(one, 'one') + two = os.path.join(self.test_prefix, 'subdirA', '2.txt') + ft.write_file(two, 'two') + three = os.path.join(self.test_prefix, 'subdirB', '3.txt') + ft.write_file(three, 'three') + ft.mkdir(os.path.join(self.test_prefix, 'empty_subdir')) + + # empty list of files yields empty result + self.assertEqual(ft.locate_files([], []), []) + self.assertEqual(ft.locate_files([], [self.test_prefix]), []) + + # error is raised if files could not be found + error_pattern = r"One or more files not found: nosuchfile.txt \(search paths: \)" + self.assertErrorRegex(EasyBuildError, error_pattern, ft.locate_files, ['nosuchfile.txt'], []) + + # files specified via absolute path don't have to be found + res = ft.locate_files([one], []) + self.assertTrue(len(res) == 1) + self.assertTrue(os.path.samefile(res[0], one)) + + # note: don't compare file paths directly but use os.path.samefile instead, + # which is required to avoid failing tests in case temporary directory is a symbolic link (e.g. on macOS) + res = ft.locate_files(['1.txt'], [self.test_prefix]) + self.assertEqual(len(res), 1) + self.assertTrue(os.path.samefile(res[0], one)) + + res = ft.locate_files(['2.txt'], [self.test_prefix]) + self.assertEqual(len(res), 1) + self.assertTrue(os.path.samefile(res[0], two)) + + res = ft.locate_files(['1.txt', '3.txt'], [self.test_prefix]) + self.assertEqual(len(res), 2) + self.assertTrue(os.path.samefile(res[0], one)) + self.assertTrue(os.path.samefile(res[1], three)) + + # search in multiple paths + files = ['2.txt', '3.txt'] + paths = [os.path.dirname(three), os.path.dirname(two)] + res = ft.locate_files(files, paths) + self.assertEqual(len(res), 2) + self.assertTrue(os.path.samefile(res[0], two)) + self.assertTrue(os.path.samefile(res[1], three)) + + # same file specified multiple times works fine + files = ['1.txt', '2.txt', '1.txt', '3.txt', '2.txt'] + res = ft.locate_files(files, [self.test_prefix]) + self.assertEqual(len(res), 5) + for idx, expected in enumerate([one, two, one, three, two]): + self.assertTrue(os.path.samefile(res[idx], expected)) + + # only some files found yields correct warning + files = ['2.txt', '3.txt', '1.txt'] + error_pattern = r"One or more files not found: 3\.txt, 1.txt \(search paths: .*/subdirA\)" + self.assertErrorRegex(EasyBuildError, error_pattern, ft.locate_files, files, [os.path.dirname(two)]) + + # check that relative paths are found in current working dir + ft.change_dir(self.test_prefix) + rel_paths = ['subdirA/2.txt', '1.txt'] + # result is still absolute paths to those files + res = ft.locate_files(rel_paths, []) + self.assertEqual(len(res), 2) + self.assertTrue(os.path.samefile(res[0], two)) + self.assertTrue(os.path.samefile(res[1], one)) + + # no recursive search in current working dir (which would potentially be way too expensive) + error_pattern = r"One or more files not found: 2\.txt \(search paths: \)" + self.assertErrorRegex(EasyBuildError, error_pattern, ft.locate_files, ['2.txt'], []) + def suite(): """ returns all the testcases in this module """ From aa470f109f8cd3bb4e772b2e5927804843cf471e Mon Sep 17 00:00:00 2001 From: Kenneth Hoste Date: Sat, 24 Oct 2020 13:35:25 +0200 Subject: [PATCH 32/51] check whether paths still exist before using cached result for fetch_files_from_pr --- easybuild/tools/github.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/easybuild/tools/github.py b/easybuild/tools/github.py index 8d7dac059a..2e0f0fc135 100644 --- a/easybuild/tools/github.py +++ b/easybuild/tools/github.py @@ -393,7 +393,7 @@ def cache_aware_func(pr, *args, **kwargs): # cache key is combination of all function arguments (incl. optional ones) key = tuple([pr] + [kwargs[key] for key in sorted(kwargs.keys())]) - if key in cache: + if key in cache and all(os.path.exists(x) for x in cache[key]): _log.debug("Using cached value for fetch_files_from_pr for PR #%s (%s)", pr, kwargs) return cache[key] else: From 5c18a8ef61517b05f3080f06622c66e8e2b6f152 Mon Sep 17 00:00:00 2001 From: Kenneth Hoste Date: Sat, 24 Oct 2020 13:38:51 +0200 Subject: [PATCH 33/51] use locate_files function in det_easyconfig_paths --- easybuild/framework/easyconfig/tools.py | 49 +++++-------------------- test/framework/robot.py | 3 +- 2 files changed, 11 insertions(+), 41 deletions(-) diff --git a/easybuild/framework/easyconfig/tools.py b/easybuild/framework/easyconfig/tools.py index 128d5eddcc..5ff5c7e9bd 100644 --- a/easybuild/framework/easyconfig/tools.py +++ b/easybuild/framework/easyconfig/tools.py @@ -53,8 +53,8 @@ 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 copy_file, copy_files -from easybuild.tools.filetools import find_easyconfigs, is_patch_file, read_file, resolve_path, which, write_file +from easybuild.tools.filetools import copy_file, copy_files, 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, download_repo from easybuild.tools.multidiff import multidiff from easybuild.tools.py2vs3 import OrderedDict @@ -349,44 +349,13 @@ def det_easyconfig_paths(orig_paths): ec_files = [path for path in pr_files if path.endswith('.eb')] if ec_files and robot_path: - # look for easyconfigs with relative paths in robot search path, - # unless they were found at the given relative paths - - # determine which easyconfigs files need to be found, if any - ecs_to_find = [] - for idx, ec_file in enumerate(ec_files): - if ec_file == os.path.basename(ec_file) and not os.path.exists(ec_file): - ecs_to_find.append((idx, ec_file)) - _log.debug("List of easyconfig files to find: %s" % ecs_to_find) - - # find missing easyconfigs by walking paths in robot search path - for path in robot_path: - _log.debug("Looking for missing easyconfig files (%d left) in %s..." % (len(ecs_to_find), path)) - for (subpath, dirnames, filenames) in os.walk(path, topdown=True): - for idx, orig_path in ecs_to_find[:]: - if orig_path in filenames: - full_path = os.path.join(subpath, orig_path) - _log.info("Found %s in %s: %s" % (orig_path, path, full_path)) - ec_files[idx] = full_path - # if file was found, stop looking for it (first hit wins) - ecs_to_find.remove((idx, orig_path)) - - # stop os.walk insanity as soon as we have all we need (os.walk loop) - if not ecs_to_find: - break - - # ignore subdirs specified to be ignored by replacing items in dirnames list used by os.walk - dirnames[:] = [d for d in dirnames if d not in build_option('ignore_dirs')] - - # ignore archived easyconfigs, unless specified otherwise - if not build_option('consider_archived_easyconfigs'): - dirnames[:] = [d for d in dirnames if d != EASYCONFIGS_ARCHIVE_DIR] - - # stop os.walk insanity as soon as we have all we need (outer loop) - if not ecs_to_find: - break - - return [os.path.abspath(ec_file) for ec_file in ec_files] + ignore_subdirs = build_option('ignore_dirs') + if not build_option('consider_archived_easyconfigs'): + ignore_subdirs.extend(EASYCONFIGS_ARCHIVE_DIR) + + ec_files = locate_files(ec_files, robot_path, ignore_subdirs=ignore_subdirs) + + return ec_files def parse_easyconfigs(paths, validate=True): diff --git a/test/framework/robot.py b/test/framework/robot.py index e50f570a37..6eca07b571 100644 --- a/test/framework/robot.py +++ b/test/framework/robot.py @@ -654,7 +654,8 @@ def test_det_easyconfig_paths(self): '--robot', '--unittest-file=%s' % self.logfile, ] - self.assertErrorRegex(EasyBuildError, "Can't find", self.eb_main, args, logfile=dummylogfn, raise_error=True) + error_pattern = "One or more files not found: intel-2012a.eb" + self.assertErrorRegex(EasyBuildError, error_pattern, self.eb_main, args, logfile=dummylogfn, raise_error=True) args.append('--consider-archived-easyconfigs') outtxt = self.eb_main(args, logfile=dummylogfn, raise_error=True) From 756f3370f53230193068acc5b741963fa28625c0 Mon Sep 17 00:00:00 2001 From: Kenneth Hoste Date: Sat, 24 Oct 2020 15:44:53 +0200 Subject: [PATCH 34/51] fix default for pr_target_account build option --- easybuild/tools/config.py | 4 ++++ easybuild/tools/options.py | 14 +++++++------- 2 files changed, 11 insertions(+), 7 deletions(-) diff --git a/easybuild/tools/config.py b/easybuild/tools/config.py index 10d5a19add..0340efd560 100644 --- a/easybuild/tools/config.py +++ b/easybuild/tools/config.py @@ -101,6 +101,7 @@ DEFAULT_PKG_TOOL = PKG_TOOL_FPM DEFAULT_PKG_TYPE = PKG_TYPE_RPM DEFAULT_PNS = 'EasyBuildPNS' +DEFAULT_PR_TARGET_ACCOUNT = 'easybuilders' DEFAULT_PREFIX = os.path.join(os.path.expanduser('~'), ".local", "easybuild") DEFAULT_REPOSITORY = 'FileRepository' DEFAULT_WAIT_ON_LOCK_INTERVAL = 60 @@ -307,6 +308,9 @@ def mk_full_default_path(name, prefix=DEFAULT_PREFIX): DEFAULT_PKG_TYPE: [ 'package_type', ], + DEFAULT_PR_TARGET_ACCOUNT: [ + 'pr_target_account', + ], GENERAL_CLASS: [ 'suffix_modules_path', ], diff --git a/easybuild/tools/options.py b/easybuild/tools/options.py index 026ae528b1..c801fc3f6f 100644 --- a/easybuild/tools/options.py +++ b/easybuild/tools/options.py @@ -64,11 +64,11 @@ from easybuild.tools.config import DEFAULT_JOB_BACKEND, DEFAULT_LOGFILE_FORMAT, DEFAULT_MAX_FAIL_RATIO_PERMS from easybuild.tools.config import DEFAULT_MINIMAL_BUILD_ENV, DEFAULT_MNS, DEFAULT_MODULE_SYNTAX, DEFAULT_MODULES_TOOL from easybuild.tools.config import DEFAULT_MODULECLASSES, DEFAULT_PATH_SUBDIRS, DEFAULT_PKG_RELEASE, DEFAULT_PKG_TOOL -from easybuild.tools.config import DEFAULT_PKG_TYPE, DEFAULT_PNS, DEFAULT_PREFIX, DEFAULT_REPOSITORY -from easybuild.tools.config import DEFAULT_WAIT_ON_LOCK_INTERVAL, DEFAULT_WAIT_ON_LOCK_LIMIT, EBROOT_ENV_VAR_ACTIONS -from easybuild.tools.config import ERROR, FORCE_DOWNLOAD_CHOICES, GENERAL_CLASS, IGNORE, JOB_DEPS_TYPE_ABORT_ON_ERROR -from easybuild.tools.config import JOB_DEPS_TYPE_ALWAYS_RUN, LOADED_MODULES_ACTIONS, LOCAL_VAR_NAMING_CHECK_WARN -from easybuild.tools.config import LOCAL_VAR_NAMING_CHECKS, WARN +from easybuild.tools.config import DEFAULT_PKG_TYPE, DEFAULT_PNS, DEFAULT_PREFIX, DEFAULT_PR_TARGET_ACCOUNT +from easybuild.tools.config import DEFAULT_REPOSITORY, DEFAULT_WAIT_ON_LOCK_INTERVAL, DEFAULT_WAIT_ON_LOCK_LIMIT +from easybuild.tools.config import EBROOT_ENV_VAR_ACTIONS, ERROR, FORCE_DOWNLOAD_CHOICES, GENERAL_CLASS, IGNORE +from easybuild.tools.config import JOB_DEPS_TYPE_ABORT_ON_ERROR, JOB_DEPS_TYPE_ALWAYS_RUN, LOADED_MODULES_ACTIONS +from easybuild.tools.config import LOCAL_VAR_NAMING_CHECK_WARN, LOCAL_VAR_NAMING_CHECKS, WARN from easybuild.tools.config import get_pretend_installpath, init, init_build_options, mk_full_default_path from easybuild.tools.configobj import ConfigObj, ConfigObjError from easybuild.tools.docs import FORMAT_TXT, FORMAT_RST @@ -78,7 +78,7 @@ from easybuild.tools.environment import restore_env, unset_env_vars from easybuild.tools.filetools import CHECKSUM_TYPE_SHA256, CHECKSUM_TYPES, expand_glob_paths, install_fake_vsc from easybuild.tools.filetools import move_file, which -from easybuild.tools.github import GITHUB_EB_MAIN, GITHUB_PR_DIRECTION_DESC, GITHUB_PR_ORDER_CREATED +from easybuild.tools.github import GITHUB_PR_DIRECTION_DESC, GITHUB_PR_ORDER_CREATED from easybuild.tools.github import GITHUB_PR_STATE_OPEN, GITHUB_PR_STATES, GITHUB_PR_ORDERS, GITHUB_PR_DIRECTIONS from easybuild.tools.github import HAVE_GITHUB_API, HAVE_KEYRING, VALID_CLOSE_PR_REASONS from easybuild.tools.github import fetch_easyblocks_from_pr, fetch_github_token @@ -642,7 +642,7 @@ def github_options(self): str, 'store', None), 'pr-commit-msg': ("Commit message for new/updated pull request created with --new-pr", str, 'store', None), 'pr-descr': ("Description for new pull request created with --new-pr", str, 'store', None), - 'pr-target-account': ("Target account for new PRs", str, 'store', GITHUB_EB_MAIN), + 'pr-target-account': ("Target account for new PRs", str, 'store', DEFAULT_PR_TARGET_ACCOUNT), 'pr-target-branch': ("Target branch for new PRs", str, 'store', DEFAULT_BRANCH), 'pr-target-repo': ("Target repository for new/updating PRs (default: auto-detect based on provided files)", str, 'store', None), From 31261d2c848f7b5350fa2a8928e9a7faf5bbdad2 Mon Sep 17 00:00:00 2001 From: Kenneth Hoste Date: Sat, 24 Oct 2020 15:45:25 +0200 Subject: [PATCH 35/51] add det_copy_ec_specs function to easybuild.framework.easyconfig.tools --- easybuild/framework/easyconfig/tools.py | 64 ++++++++++++++++++- test/framework/easyconfig.py | 81 ++++++++++++++++++++++++- 2 files changed, 143 insertions(+), 2 deletions(-) diff --git a/easybuild/framework/easyconfig/tools.py b/easybuild/framework/easyconfig/tools.py index 5ff5c7e9bd..fa23383182 100644 --- a/easybuild/framework/easyconfig/tools.py +++ b/easybuild/framework/easyconfig/tools.py @@ -55,7 +55,7 @@ from easybuild.tools.environment import restore_env from easybuild.tools.filetools import copy_file, copy_files, 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, download_repo +from easybuild.tools.github import fetch_easyconfigs_from_pr, fetch_files_from_pr, download_repo from easybuild.tools.multidiff import multidiff from easybuild.tools.py2vs3 import OrderedDict from easybuild.tools.toolchain.toolchain import is_system_toolchain @@ -700,6 +700,68 @@ def avail_easyblocks(): return easyblocks +def det_copy_ec_specs(orig_paths, from_pr): + """Determine list of paths + target directory for --copy-ec.""" + + target_path, paths = None, [] + + # if only one argument is specified, use current directory as target directory + if len(orig_paths) == 1: + target_path = os.getcwd() + paths = orig_paths[:] + + # if multiple arguments are specified, assume that last argument is target location, + # and remove that from list of paths to copy + elif orig_paths: + target_path = orig_paths[-1] + paths = orig_paths[:-1] + + # if --from-pr was used in combination with --copy-ec, some extra care must be taken + if from_pr: + # pull in the paths to all the changed files in the PR, + # which includes easyconfigs but also patch files (& maybe more); + # do this in a dedicated subdirectory of the working tmpdir, + # 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) + + # assume that files need to be copied to current working directory for now + target_path = os.getcwd() + + if orig_paths: + last_path = orig_paths[-1] + + # check files touched by PR and see if the target directory for --copy-ec + # corresponds to the name of one of these files; + # if so we should copy the specified file(s) to the current working directory, + # since interpreting the last argument as target location is very unlikely incorrect in this case + pr_filenames = [os.path.basename(p) for p in pr_paths] + if last_path in pr_filenames: + paths = orig_paths[:] + else: + target_path = last_path + # exclude last argument that is used as target location + paths = orig_paths[:-1] + + # if list of files to copy is empty at this point, + # we simply copy *all* files touched by the PR + if not paths: + paths = pr_paths + + # replace path for files touched by PR (no need to worry about others) + for idx, path in enumerate(paths): + filename = os.path.basename(path) + pr_matches = [x for x in pr_paths if os.path.basename(x) == filename] + if len(pr_matches) == 1: + paths[idx] = pr_matches[0] + elif pr_matches: + raise EasyBuildError("Found multiple paths for %s in PR: %s", filename, pr_matches) + + return paths, target_path + + def copy_ecs_to_target(determined_paths, target_path, prefix=False, target_is_dir=False): """ Copy list of easyconfigs to specified path diff --git a/test/framework/easyconfig.py b/test/framework/easyconfig.py index 3ebeb86f8d..d069f84007 100644 --- a/test/framework/easyconfig.py +++ b/test/framework/easyconfig.py @@ -56,7 +56,8 @@ from easybuild.framework.easyconfig.templates import template_constant_dict, to_template_str from easybuild.framework.easyconfig.style import check_easyconfigs_style from easybuild.framework.easyconfig.tools import categorize_files_by_type, check_sha256_checksums, dep_graph -from easybuild.framework.easyconfig.tools import find_related_easyconfigs, get_paths_for, parse_easyconfigs +from easybuild.framework.easyconfig.tools import det_copy_ec_specs, find_related_easyconfigs, get_paths_for +from easybuild.framework.easyconfig.tools import parse_easyconfigs from easybuild.framework.easyconfig.tweak import obtain_ec_for, tweak_one from easybuild.framework.extension import resolve_exts_filter_template from easybuild.toolchains.system import SystemToolchain @@ -3975,6 +3976,84 @@ def test_cuda_compute_capabilities(self): self.assertEqual(ec['configopts'], 'sm_42,sm_63') self.assertEqual(ec['preconfigopts'], 'sm_42 sm_63') + def test_det_copy_ec_specs(self): + """Test det_copy_ec_specs function.""" + + cwd = os.getcwd() + + # no problems on empty list as input + paths, target_path = det_copy_ec_specs([], None) + self.assertEqual(paths, []) + self.assertEqual(target_path, None) + + # single-element list, no --from-pr => use current directory as target location + paths, target_path = det_copy_ec_specs(['test.eb'], None) + self.assertEqual(paths, ['test.eb']) + self.assertTrue(os.path.samefile(target_path, cwd)) + + # multi-element list, no --from-pr => last element is used as target location + for args in (['test.eb', 'dir'], ['test1.eb', 'test2.eb', 'dir']): + paths, target_path = det_copy_ec_specs(args, None) + self.assertEqual(paths, args[:-1]) + self.assertEqual(target_path, args[-1]) + + # use fixed PR (speeds up the test due to caching in fetch_files_from_pr; + # see https://github.com/easybuilders/easybuild-easyconfigs/pull/8007 + from_pr = 8007 + arrow_ec_fn = 'Arrow-0.7.1-intel-2017b-Python-3.6.3.eb' + bat_ec_fn = 'bat-0.3.3-intel-2017b-Python-3.6.3.eb' + bat_patch_fn = 'bat-0.3.3-fix-pyspark.patch' + pr_files = [ + arrow_ec_fn, + bat_ec_fn, + bat_patch_fn, + ] + + # if no paths are specified, default is to copy all files touched by PR to current working directory + paths, target_path = det_copy_ec_specs([], from_pr) + self.assertEqual(len(paths), 3) + filenames = sorted([os.path.basename(x) for x in paths]) + self.assertEqual(filenames, sorted(pr_files)) + self.assertTrue(os.path.samefile(target_path, cwd)) + + # last argument is used as target directory, + # unless it corresponds to a file touched by PR + args = [bat_ec_fn, 'target_dir'] + paths, target_path = det_copy_ec_specs(args, from_pr) + self.assertEqual(len(paths), 1) + self.assertEqual(os.path.basename(paths[0]), bat_ec_fn) + self.assertEqual(target_path, 'target_dir') + + args = [bat_ec_fn] + paths, target_path = det_copy_ec_specs(args, from_pr) + self.assertEqual(len(paths), 1) + self.assertEqual(os.path.basename(paths[0]), bat_ec_fn) + self.assertTrue(os.path.samefile(target_path, cwd)) + + args = [arrow_ec_fn, bat_ec_fn] + paths, target_path = det_copy_ec_specs(args, from_pr) + self.assertEqual(len(paths), 2) + self.assertEqual(os.path.basename(paths[0]), arrow_ec_fn) + self.assertEqual(os.path.basename(paths[1]), bat_ec_fn) + self.assertTrue(os.path.samefile(target_path, cwd)) + + args = [bat_ec_fn, bat_patch_fn] + paths, target_path = det_copy_ec_specs(args, from_pr) + self.assertEqual(len(paths), 2) + self.assertEqual(os.path.basename(paths[0]), bat_ec_fn) + self.assertEqual(os.path.basename(paths[1]), bat_patch_fn) + self.assertTrue(os.path.samefile(target_path, cwd)) + + # also test with combination of local files and files from PR + args = [arrow_ec_fn, 'test.eb', 'test.patch', bat_patch_fn] + paths, target_path = det_copy_ec_specs(args, from_pr) + self.assertEqual(len(paths), 4) + self.assertEqual(os.path.basename(paths[0]), arrow_ec_fn) + self.assertEqual(paths[1], 'test.eb') + self.assertEqual(paths[2], 'test.patch') + self.assertEqual(os.path.basename(paths[3]), bat_patch_fn) + self.assertTrue(os.path.samefile(target_path, cwd)) + def suite(): """ returns all the testcases in this module """ From a761eab38c4ba01389ee818edb8ccbcf6e537c59 Mon Sep 17 00:00:00 2001 From: Kenneth Hoste Date: Sat, 24 Oct 2020 16:04:08 +0200 Subject: [PATCH 36/51] use det_copy_ec_specs in main.py --- easybuild/main.py | 58 ++++++++++++----------------------------------- 1 file changed, 15 insertions(+), 43 deletions(-) diff --git a/easybuild/main.py b/easybuild/main.py index 4aef13249f..ce12aed41b 100644 --- a/easybuild/main.py +++ b/easybuild/main.py @@ -39,7 +39,6 @@ import os import stat import sys -import tempfile import traceback # IMPORTANT this has to be the first easybuild import as it customises the logging @@ -52,15 +51,16 @@ from easybuild.framework.easyconfig.easyconfig import fix_deprecated_easyconfigs, verify_easyconfig_filename from easybuild.framework.easyconfig.style import cmdline_easyconfigs_style_check from easybuild.framework.easyconfig.tools import categorize_files_by_type, copy_ecs_to_target, dep_graph -from easybuild.framework.easyconfig.tools import det_easyconfig_paths, dump_env_script, get_paths_for -from easybuild.framework.easyconfig.tools import parse_easyconfigs, review_pr, run_contrib_checks, skip_available +from easybuild.framework.easyconfig.tools import det_copy_ec_specs, det_easyconfig_paths, dump_env_script +from easybuild.framework.easyconfig.tools import get_paths_for, parse_easyconfigs, review_pr, run_contrib_checks +from easybuild.framework.easyconfig.tools import skip_available from easybuild.framework.easyconfig.tweak import obtain_ec_for, tweak from easybuild.tools.config import find_last_log, get_repository, get_repositorypath, build_option from easybuild.tools.containers.common import containerize from easybuild.tools.docs import list_software -from easybuild.tools.filetools import adjust_permissions, cleanup, dump_index, load_index +from easybuild.tools.filetools import adjust_permissions, cleanup, dump_index, load_index, locate_files from easybuild.tools.filetools import read_file, register_lock_cleanup_signal_handlers, write_file -from easybuild.tools.github import check_github, close_pr, fetch_files_from_pr, find_easybuild_easyconfig +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 new_pr_from_branch from easybuild.tools.github import sync_branch_with_develop, sync_pr_with_develop, update_branch, update_pr @@ -305,17 +305,6 @@ def main(args=None, logfile=None, do_build=None, testing=False, modtool=None): eb_file = find_easybuild_easyconfig() orig_paths.append(eb_file) - # if only one easyconfig is specified, or if none are specified and we are using --from-pr, - # use current directory as target directory - if len(orig_paths) == 1 and not (options.copy_ec and options.from_pr): - target_path = os.getcwd() - elif orig_paths: - # last path is target when --copy-ec is used, so remove that from the list - target_path = orig_paths.pop() if options.copy_ec else None - else: - # if no easyconfig files are specified and we are using --from-pr, use current directory as target directory - target_path = os.getcwd() if (options.copy_ec and options.from_pr) else None - categorized_paths = categorize_files_by_type(orig_paths) # command line options that do not require any easyconfigs to be specified @@ -327,37 +316,20 @@ def main(args=None, logfile=None, do_build=None, testing=False, modtool=None): # determine paths to easyconfigs determined_paths = det_easyconfig_paths(categorized_paths['easyconfigs']) - # only copy easyconfigs here if we're not using --try-* (that's are handled below) + # only copy easyconfigs here if we're not using --try-* (that's handled below) copy_ec = options.copy_ec and not tweaked_ecs_paths + if copy_ec or options.fix_deprecated_easyconfigs or options.show_ec: - if options.from_pr: - # pull in the paths to all the changed files in the PR (need to do this in a new temp dir) - pr_paths = fetch_files_from_pr(pr=options.from_pr, path=tempfile.mktemp()) - for pr_path in pr_paths: - # we assumed that the last argument from the command line was the target_path but if it appears in the - # PR file list then it was most likely intended to use the CWD and (also) copy that particular file - if target_path == os.path.basename(pr_path): - if not orig_paths: - # It should have been the only easyconfig selected - determined_paths = [pr_path] - else: - if os.path.basename(pr_path) not in [os.path.basename(path) for path in determined_paths]: - determined_paths.append(pr_path) - target_path = os.getcwd() - other_pr_paths = [] - for ec_path in determined_paths: - for pr_path in pr_paths: - if os.path.basename(ec_path) == os.path.basename(pr_path): - # Search for any associated patches (they would have the same dirname) - for patch_path in pr_paths: - if pr_path != patch_path and os.path.dirname(pr_path) == os.path.dirname(patch_path): - # if it's an easyconfig, we already have it covered - if not patch_path.endswith('.eb') and patch_path not in other_pr_paths: - other_pr_paths.append(patch_path) - determined_paths += other_pr_paths if options.copy_ec: - copy_ecs_to_target(determined_paths, target_path) + # figure out list of files to copy + target location (taking into account --from-pr) + paths, target_path = det_copy_ec_specs(orig_paths, options.from_pr) + + # at this point some paths may still just be filenames rather than absolute paths, + # so try to determine full path for those too via robot search path + paths = locate_files(paths, robot_path) + + copy_ecs_to_target(paths, target_path) elif options.fix_deprecated_easyconfigs: fix_deprecated_easyconfigs(determined_paths) From 0ed52ec50e49a3ecd8b2cf9653a36819bd80861b Mon Sep 17 00:00:00 2001 From: Kenneth Hoste Date: Sat, 24 Oct 2020 16:04:39 +0200 Subject: [PATCH 37/51] clean up and enhance tests for combination of --copy-ec and --from-pr --- test/framework/options.py | 158 ++++++++++++++++++++++++++------------ 1 file changed, 109 insertions(+), 49 deletions(-) diff --git a/test/framework/options.py b/test/framework/options.py index daaff28863..cc9cc23ee8 100644 --- a/test/framework/options.py +++ b/test/framework/options.py @@ -50,8 +50,8 @@ from easybuild.tools.config import DEFAULT_MODULECLASSES from easybuild.tools.config import find_last_log, get_build_log_path, get_module_syntax, module_classes from easybuild.tools.environment import modify_env -from easybuild.tools.filetools import change_dir, copy_dir, copy_file, download_file, mkdir, read_file -from easybuild.tools.filetools import remove_dir, remove_file, which, write_file +from easybuild.tools.filetools import change_dir, copy_dir, copy_file, download_file, is_patch_file, mkdir +from easybuild.tools.filetools import read_file, remove_dir, remove_file, which, write_file from easybuild.tools.github import GITHUB_RAW, GITHUB_EB_MAIN, GITHUB_EASYCONFIGS_REPO from easybuild.tools.github import URL_SEPARATOR, fetch_github_token from easybuild.tools.modules import Lmod @@ -965,19 +965,21 @@ def test_show_ec(self): regex = re.compile(pattern, re.M) self.assertTrue(regex.search(stdout), "Pattern '%s' found in: %s" % (regex.pattern, stdout)) + def mocked_main(self, args): + """Run eb_main with mocked stdout/stderr.""" + #self.mock_stderr(True) + self.mock_stdout(True) + self.eb_main(args, raise_error=True) + #stderr, stdout = self.get_stderr(), self.get_stdout() + stdout = self.get_stdout() + #self.mock_stderr(False) + self.mock_stdout(False) + #self.assertEqual(stderr, '') + return stdout.strip() + def test_copy_ec(self): """Test --copy-ec.""" - def mocked_main(args): - self.mock_stderr(True) - self.mock_stdout(True) - self.eb_main(args, raise_error=True) - stderr, stdout = self.get_stderr(), self.get_stdout() - self.mock_stderr(False) - self.mock_stdout(False) - self.assertEqual(stderr, '') - return stdout.strip() - topdir = os.path.dirname(os.path.abspath(__file__)) test_easyconfigs_dir = os.path.join(topdir, 'easyconfigs', 'test_ecs') @@ -987,7 +989,7 @@ def mocked_main(args): # basic test: copying one easyconfig file to a non-existing absolute path test_ec = os.path.join(self.test_prefix, 'test.eb') args = ['--copy-ec', 'toy-0.0.eb', test_ec] - stdout = mocked_main(args) + stdout = self.mocked_main(args) self.assertEqual(stdout, 'toy-0.0.eb copied to %s' % test_ec) self.assertTrue(os.path.exists(test_ec)) @@ -1001,7 +1003,7 @@ def mocked_main(args): self.assertFalse(os.path.exists(target_fn)) args = ['--copy-ec', 'toy-0.0.eb', target_fn] - stdout = mocked_main(args) + stdout = self.mocked_main(args) self.assertEqual(stdout, 'toy-0.0.eb copied to test.eb') change_dir(cwd) @@ -1013,7 +1015,7 @@ def mocked_main(args): test_target_dir = os.path.join(self.test_prefix, 'test_target_dir') mkdir(test_target_dir) args = ['--copy-ec', 'toy-0.0.eb', test_target_dir] - stdout = mocked_main(args) + stdout = self.mocked_main(args) self.assertEqual(stdout, 'toy-0.0.eb copied to %s' % test_target_dir) copied_toy_ec = os.path.join(test_target_dir, 'toy-0.0.eb') @@ -1035,7 +1037,7 @@ def check_copied_files(): # copying multiple easyconfig files to a non-existing target directory (which is created automatically) args = ['--copy-ec', 'toy-0.0.eb', 'bzip2-1.0.6-GCC-4.9.2.eb', test_target_dir] - stdout = mocked_main(args) + stdout = self.mocked_main(args) self.assertEqual(stdout, '2 file(s) copied to %s' % test_target_dir) check_copied_files() @@ -1047,7 +1049,7 @@ def check_copied_files(): args[-1] = os.path.basename(test_target_dir) self.assertFalse(os.path.exists(args[-1])) - stdout = mocked_main(args) + stdout = self.mocked_main(args) self.assertEqual(stdout, '2 file(s) copied to test_target_dir') check_copied_files() @@ -1065,14 +1067,23 @@ def check_copied_files(): change_dir(test_working_dir) self.assertEqual(len(os.listdir(os.getcwd())), 0) args = ['--copy-ec', 'toy-0.0.eb'] - stdout = mocked_main(args) + stdout = self.mocked_main(args) regex = re.compile('toy-0.0.eb copied to .*/%s' % os.path.basename(test_working_dir)) self.assertTrue(regex.match(stdout), "Pattern '%s' found in: %s" % (regex.pattern, stdout)) copied_toy_cwd = os.path.join(test_working_dir, 'toy-0.0.eb') self.assertTrue(os.path.exists(copied_toy_cwd)) self.assertEqual(read_file(copied_toy_cwd), toy_ec_txt) - # Test --copy-ec coupled with --from-pr + # --copy-ec without arguments results in a proper error + args = ['--copy-ec'] + error_pattern = "One of more files to copy should be specified!" + self.assertErrorRegex(EasyBuildError, error_pattern, self.eb_main, args, raise_error=True) + + def test_copy_ec_from_pr(self): + """Test combination of --copy-ec with --from-pr.""" + if self.github_token is None: + print("Skipping test_copy_ec_from_pr, no GitHub token available?") + return test_working_dir = os.path.join(self.test_prefix, 'test_working_dir') mkdir(test_working_dir) @@ -1080,62 +1091,111 @@ def check_copied_files(): # Make sure the test target directory doesn't exist remove_dir(test_target_dir) - all_ecs_pr8007 = [ + all_files_pr8007 = [ 'Arrow-0.7.1-intel-2017b-Python-3.6.3.eb', 'bat-0.3.3-fix-pyspark.patch', 'bat-0.3.3-intel-2017b-Python-3.6.3.eb', ] - # test use of `--copy-ec` with `--from-pr` to the cwd - change_dir(test_working_dir) + # test use of --copy-ec with --from-pr to the current working directory + cwd = change_dir(test_working_dir) args = ['--copy-ec', '--from-pr', '8007'] - stdout = mocked_main(args) - self.assertEqual(stdout, '3 file(s) copied to %s' % test_working_dir) + stdout = self.mocked_main(args) + + regex = re.compile(r"3 file\(s\) copied to .*/%s" % os.path.basename(test_working_dir)) + self.assertTrue(regex.search(stdout), "Pattern '%s' should be found in: %s" % (regex.pattern, stdout)) + # check that the files exist - for pr_file in all_ecs_pr8007: + for pr_file in all_files_pr8007: self.assertTrue(os.path.exists(os.path.join(test_working_dir, pr_file))) remove_file(os.path.join(test_working_dir, pr_file)) - # copying multiple easyconfig files to a non-existing target directory (which is created automatically) + # copying all files touched by PR to a non-existing target directory (which is created automatically) + self.assertFalse(os.path.exists(test_target_dir)) args = ['--copy-ec', '--from-pr', '8007', test_target_dir] - stdout = mocked_main(args) - self.assertEqual(stdout, '3 file(s) copied to %s' % test_target_dir) - for pr_file in all_ecs_pr8007: + stdout = self.mocked_main(args) + + regex = re.compile(r"3 file\(s\) copied to .*/%s" % os.path.basename(test_target_dir)) + self.assertTrue(regex.search(stdout), "Pattern '%s' should be found in: %s" % (regex.pattern, stdout)) + + for pr_file in all_files_pr8007: self.assertTrue(os.path.exists(os.path.join(test_target_dir, pr_file))) remove_dir(test_target_dir) - # test where we select a single file from a PR but also has a patch file - args = ['--copy-ec', '--from-pr', '8007', 'bat-0.3.3-intel-2017b-Python-3.6.3.eb', test_target_dir] - stdout = mocked_main(args) - self.assertEqual(stdout, '2 file(s) copied to %s' % test_target_dir) - for pr_file in ['bat-0.3.3-fix-pyspark.patch', 'bat-0.3.3-intel-2017b-Python-3.6.3.eb']: - self.assertTrue(os.path.exists(os.path.join(test_target_dir, pr_file))) + # test where we select a single easyconfig file from a PR + mkdir(test_target_dir) + ec_filename = 'bat-0.3.3-intel-2017b-Python-3.6.3.eb' + args = ['--copy-ec', '--from-pr', '8007', ec_filename, test_target_dir] + stdout = self.mocked_main(args) + + regex = re.compile(r"%s copied to .*/%s" % (ec_filename, os.path.basename(test_target_dir))) + self.assertTrue(regex.search(stdout), "Pattern '%s' should be found in: %s" % (regex.pattern, stdout)) + + self.assertEqual(os.listdir(test_target_dir), [ec_filename]) + self.assertTrue("name = 'bat'" in read_file(os.path.join(test_target_dir, ec_filename))) remove_dir(test_target_dir) - # test the same thing but where we don't provide a target directory + # test copying of a single easyconfig file from a PR to a non-existing path + bat_ec = os.path.join(self.test_prefix, 'bat.eb') + args[-1] = bat_ec + stdout = self.mocked_main(args) + + regex = re.compile(r"%s copied to .*/bat.eb" % ec_filename) + self.assertTrue(regex.search(stdout), "Pattern '%s' should be found in: %s" % (regex.pattern, stdout)) + + self.assertTrue(os.path.exists(bat_ec)) + self.assertTrue("name = 'bat'" in read_file(bat_ec)) + + change_dir(cwd) + remove_dir(test_working_dir) + mkdir(test_working_dir) change_dir(test_working_dir) - args = ['--copy-ec', '--from-pr', '8007', 'bat-0.3.3-intel-2017b-Python-3.6.3.eb'] - stdout = mocked_main(args) - self.assertEqual(stdout, '2 file(s) copied to %s' % test_working_dir) - for pr_file in ['bat-0.3.3-fix-pyspark.patch', 'bat-0.3.3-intel-2017b-Python-3.6.3.eb']: - path = os.path.join(test_working_dir, pr_file) - self.assertTrue(os.path.exists(path)) - remove_file(path) + + # test copying of a patch file from a PR via --copy-ec to current directory + patch_fn = 'bat-0.3.3-fix-pyspark.patch' + args = ['--copy-ec', '--from-pr', '8007', patch_fn, '.'] + stdout = self.mocked_main(args) + + self.assertEqual(os.listdir(test_working_dir), [patch_fn]) + patch_path = os.path.join(test_working_dir, patch_fn) + self.assertTrue(os.path.exists(patch_path)) + self.assertTrue(is_patch_file(patch_path)) + remove_file(patch_path) + + # test the same thing but where we don't provide a target location + change_dir(test_working_dir) + args = ['--copy-ec', '--from-pr', '8007', ec_filename] + stdout = self.mocked_main(args) + + regex = re.compile(r"%s copied to .*/%s" % (ec_filename, os.path.basename(test_working_dir))) + self.assertTrue(regex.search(stdout), "Pattern '%s' should be found in: %s" % (regex.pattern, stdout)) + + self.assertEqual(os.listdir(test_working_dir), [ec_filename]) + self.assertTrue("name = 'bat'" in read_file(ec_filename)) + + # also test copying of patch file to current directory (without specifying target location) + change_dir(test_working_dir) + args = ['--copy-ec', '--from-pr', '8007', patch_fn] + stdout = self.mocked_main(args) + + regex = re.compile(r"%s copied to .*/%s" % (patch_fn, os.path.basename(test_working_dir))) + self.assertTrue(regex.search(stdout), "Pattern '%s' should be found in: %s" % (regex.pattern, stdout)) + + self.assertEqual(sorted(os.listdir(test_working_dir)), sorted([ec_filename, patch_fn])) + self.assertTrue(is_patch_file(patch_fn)) + + change_dir(cwd) + remove_dir(test_working_dir) # test with only one ec in the PR (final argument is taken as a filename) test_ec = os.path.join(self.test_prefix, 'test.eb') args = ['--copy-ec', '--from-pr', '11521', test_ec] ec_pr11521 = "ExifTool-12.00-GCCcore-9.3.0.eb" - stdout = mocked_main(args) + stdout = self.mocked_main(args) self.assertEqual(stdout, '%s copied to %s' % (ec_pr11521, test_ec)) self.assertTrue(os.path.exists(test_ec)) remove_file(test_ec) - # --copy-ec without arguments (and no --from-pr) results in a proper error - args = ['--copy-ec'] - error_pattern = "One of more files to copy should be specified!" - self.assertErrorRegex(EasyBuildError, error_pattern, self.eb_main, args, raise_error=True) - def test_dry_run(self): """Test dry run (long format).""" fd, dummylogfn = tempfile.mkstemp(prefix='easybuild-dummy', suffix='.log') From 79f8edbbfb129d0010b5a24c7e2a17fee9aca36d Mon Sep 17 00:00:00 2001 From: Kenneth Hoste Date: Sat, 24 Oct 2020 16:30:49 +0200 Subject: [PATCH 38/51] fix changed error pattern in test_robot_path_check due to using locate_files in det_easyconfig_paths --- test/framework/options.py | 12 ++++++------ 1 file changed, 6 insertions(+), 6 deletions(-) diff --git a/test/framework/options.py b/test/framework/options.py index cc9cc23ee8..afc6c50e76 100644 --- a/test/framework/options.py +++ b/test/framework/options.py @@ -967,14 +967,13 @@ def test_show_ec(self): def mocked_main(self, args): """Run eb_main with mocked stdout/stderr.""" - #self.mock_stderr(True) + self.mock_stderr(True) self.mock_stdout(True) self.eb_main(args, raise_error=True) - #stderr, stdout = self.get_stderr(), self.get_stdout() - stdout = self.get_stdout() - #self.mock_stderr(False) + stderr, stdout = self.get_stderr(), self.get_stdout() + self.mock_stderr(False) self.mock_stdout(False) - #self.assertEqual(stderr, '') + self.assertEqual(stderr, '') return stdout.strip() def test_copy_ec(self): @@ -2638,7 +2637,8 @@ def test_robot_path_check(self): # different error when a non-existing easyconfig file is specified to --robot args = ['--dry-run', '--robot', 'no_such_easyconfig_file_in_robot_search_path.eb'] - self.assertErrorRegex(EasyBuildError, "Can't find path", self.eb_main, args, raise_error=True) + error_pattern = "One or more files not found: no_such_easyconfig_file_in_robot_search_path.eb" + self.assertErrorRegex(EasyBuildError, error_pattern, self.eb_main, args, raise_error=True) for robot in ['-r%s' % self.test_prefix, '--robot=%s' % self.test_prefix]: args = ['toy-0.0.eb', '--dry-run', robot] From 2f6ac6e548c0f4e7ace59b6c4863a3b90dcc6b19 Mon Sep 17 00:00:00 2001 From: Kenneth Hoste Date: Sat, 24 Oct 2020 16:49:20 +0200 Subject: [PATCH 39/51] fix duplicate default value for pr_target_account build option --- easybuild/tools/config.py | 1 - 1 file changed, 1 deletion(-) diff --git a/easybuild/tools/config.py b/easybuild/tools/config.py index 0340efd560..da0e35ab4e 100644 --- a/easybuild/tools/config.py +++ b/easybuild/tools/config.py @@ -205,7 +205,6 @@ def mk_full_default_path(name, prefix=DEFAULT_PREFIX): 'pr_branch_name', 'pr_commit_msg', 'pr_descr', - 'pr_target_account', 'pr_target_repo', 'pr_title', 'rpath_filter', From 173ca950667890b1f82ace63249991204946380d Mon Sep 17 00:00:00 2001 From: Kenneth Hoste Date: Sat, 24 Oct 2020 16:50:03 +0200 Subject: [PATCH 40/51] determine --copy-ec target path *before* categorizing eb arguments by file type --- easybuild/main.py | 9 +++++---- 1 file changed, 5 insertions(+), 4 deletions(-) diff --git a/easybuild/main.py b/easybuild/main.py index ce12aed41b..e2c85b0dec 100644 --- a/easybuild/main.py +++ b/easybuild/main.py @@ -305,6 +305,10 @@ def main(args=None, logfile=None, do_build=None, testing=False, modtool=None): eb_file = find_easybuild_easyconfig() orig_paths.append(eb_file) + 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) + categorized_paths = categorize_files_by_type(orig_paths) # command line options that do not require any easyconfigs to be specified @@ -322,12 +326,9 @@ def main(args=None, logfile=None, do_build=None, testing=False, modtool=None): if copy_ec or options.fix_deprecated_easyconfigs or options.show_ec: if options.copy_ec: - # figure out list of files to copy + target location (taking into account --from-pr) - paths, target_path = det_copy_ec_specs(orig_paths, options.from_pr) - # at this point some paths may still just be filenames rather than absolute paths, # so try to determine full path for those too via robot search path - paths = locate_files(paths, robot_path) + paths = locate_files(orig_paths, robot_path) copy_ecs_to_target(paths, target_path) From bb96d1a1e804435b0639953718639dfbce3e4488 Mon Sep 17 00:00:00 2001 From: Kenneth Hoste Date: Sat, 24 Oct 2020 18:04:16 +0200 Subject: [PATCH 41/51] don't make assumptions about current working directory when checking contents of copied file in test_copy_ec_from_pr --- test/framework/options.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/test/framework/options.py b/test/framework/options.py index afc6c50e76..4d51759707 100644 --- a/test/framework/options.py +++ b/test/framework/options.py @@ -1170,7 +1170,7 @@ def test_copy_ec_from_pr(self): self.assertTrue(regex.search(stdout), "Pattern '%s' should be found in: %s" % (regex.pattern, stdout)) self.assertEqual(os.listdir(test_working_dir), [ec_filename]) - self.assertTrue("name = 'bat'" in read_file(ec_filename)) + self.assertTrue("name = 'bat'" in read_file(os.path.join(test_working_dir, ec_filename))) # also test copying of patch file to current directory (without specifying target location) change_dir(test_working_dir) From b6881eec71f941c58a7203834632210f36c090bc Mon Sep 17 00:00:00 2001 From: Kenneth Hoste Date: Sat, 24 Oct 2020 21:50:15 +0200 Subject: [PATCH 42/51] correctly add archive subdir to ignore_subdirs in det_easyconfig_paths + enhance test to catch the issue --- easybuild/framework/easyconfig/tools.py | 2 +- test/framework/robot.py | 8 ++++++++ 2 files changed, 9 insertions(+), 1 deletion(-) diff --git a/easybuild/framework/easyconfig/tools.py b/easybuild/framework/easyconfig/tools.py index fa23383182..b8be79be36 100644 --- a/easybuild/framework/easyconfig/tools.py +++ b/easybuild/framework/easyconfig/tools.py @@ -351,7 +351,7 @@ def det_easyconfig_paths(orig_paths): if ec_files and robot_path: ignore_subdirs = build_option('ignore_dirs') if not build_option('consider_archived_easyconfigs'): - ignore_subdirs.extend(EASYCONFIGS_ARCHIVE_DIR) + ignore_subdirs.append(EASYCONFIGS_ARCHIVE_DIR) ec_files = locate_files(ec_files, robot_path, ignore_subdirs=ignore_subdirs) diff --git a/test/framework/robot.py b/test/framework/robot.py index 6eca07b571..dc540674ec 100644 --- a/test/framework/robot.py +++ b/test/framework/robot.py @@ -620,12 +620,19 @@ def test_det_easyconfig_paths(self): test_ec = 'toy-0.0-deps.eb' shutil.copy2(os.path.join(test_ecs_path, 't', 'toy', test_ec), self.test_prefix) + # copy hwloc easyconfig to h/hwloc subdir in robot search path, + # to trigger bug fixed in det_easyconfig_paths (.extend rather than .append for '__archive'__ to ignore_subdirs) + hwloc_ec = 'hwloc-1.11.8-GCC-6.4.0-2.28.eb' + subdir_hwloc = os.path.join(self.test_prefix, 'h', 'hwloc') + mkdir(subdir_hwloc, parents=True) + shutil.copy2(os.path.join(test_ecs_path, 'h', 'hwloc', hwloc_ec), subdir_hwloc) shutil.copy2(os.path.join(test_ecs_path, 'i', 'intel', 'intel-2018a.eb'), self.test_prefix) self.assertFalse(os.path.exists(test_ec)) args = [ os.path.join(test_ecs_path, 't', 'toy', 'toy-0.0.eb'), test_ec, # relative path, should be resolved via robot search path + hwloc_ec, '--dry-run', '--debug', '--robot', @@ -640,6 +647,7 @@ def test_det_easyconfig_paths(self): (test_ecs_path, 'toy/0.0'), # specified easyconfigs, available at given location (self.test_prefix, 'intel/2018a'), # dependency, found in robot search path (self.test_prefix, 'toy/0.0-deps'), # specified easyconfig, found in robot search path + (self.test_prefix, 'hwloc/1.11.8-GCC-6.4.0-2.28'), # specified easyconfig, found in robot search path ] for path_prefix, module in modules: ec_fn = "%s.eb" % '-'.join(module.split('/')) From 1f7a0a7312e1be3bc47c602aaa81ae6e80736a3d Mon Sep 17 00:00:00 2001 From: Kenneth Hoste Date: Sat, 24 Oct 2020 22:31:46 +0200 Subject: [PATCH 43/51] use full path when checking patch file in --copy-ec --from-pr test --- test/framework/options.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/test/framework/options.py b/test/framework/options.py index 4d51759707..0f3d0e6626 100644 --- a/test/framework/options.py +++ b/test/framework/options.py @@ -1181,7 +1181,7 @@ def test_copy_ec_from_pr(self): self.assertTrue(regex.search(stdout), "Pattern '%s' should be found in: %s" % (regex.pattern, stdout)) self.assertEqual(sorted(os.listdir(test_working_dir)), sorted([ec_filename, patch_fn])) - self.assertTrue(is_patch_file(patch_fn)) + self.assertTrue(is_patch_file(os.path.join(test_working_dir, patch_fn))) change_dir(cwd) remove_dir(test_working_dir) From aaa67373ebd8c82c4f9db11ddf9a04c719b3d81c Mon Sep 17 00:00:00 2001 From: Kenneth Hoste Date: Sun, 25 Oct 2020 10:06:10 +0100 Subject: [PATCH 44/51] take indices into account in locate_files --- easybuild/tools/filetools.py | 32 ++++++++++++++++++++------------ 1 file changed, 20 insertions(+), 12 deletions(-) diff --git a/easybuild/tools/filetools.py b/easybuild/tools/filetools.py index 1975ac780d..372ea88893 100644 --- a/easybuild/tools/filetools.py +++ b/easybuild/tools/filetools.py @@ -806,24 +806,32 @@ def locate_files(files, paths, ignore_subdirs=None): # find missing easyconfigs by walking paths in robot search path for path in paths: _log.debug("Looking for missing files (%d left) in %s..." % (len(files_to_find), path)) - for (subpath, dirnames, filenames) in os.walk(path, topdown=True): - for idx, orig_path in files_to_find[:]: - if orig_path in filenames: - full_path = os.path.join(subpath, orig_path) - _log.info("Found %s in %s: %s" % (orig_path, path, full_path)) + + # try to load index for current path, or create one + path_index = load_index(path, ignore_dirs=ignore_subdirs) + if path_index is None or build_option('ignore_index'): + if os.path.exists(path): + _log.info("No index found for %s, creating one...", path) + path_index = create_index(path, ignore_dirs=ignore_subdirs) + else: + path_index = [] + else: + _log.info("Index found for %s, so using it...", path) + + for filepath in path_index: + for idx, file_to_find in files_to_find[:]: + if os.path.basename(filepath) == file_to_find: + full_path = os.path.join(path, filepath) + _log.info("Found %s in %s: %s", file_to_find, path, full_path) files[idx] = full_path # if file was found, stop looking for it (first hit wins) - files_to_find.remove((idx, orig_path)) + files_to_find.remove((idx, file_to_find)) - # stop os.walk insanity as soon as we have all we need (os.walk loop) + # stop as soon as we have all we need (path index loop) if not files_to_find: break - # ignore specified subdirectories - if ignore_subdirs: - dirnames[:] = [d for d in dirnames if d not in ignore_subdirs] - - # stop os.walk insanity as soon as we have all we need (outer loop) + # stop as soon as we have all we need (paths loop) if not files_to_find: break From c989ce9574c137e25b55941ad9f21ed9c7830d3a Mon Sep 17 00:00:00 2001 From: Kenneth Hoste Date: Sun, 25 Oct 2020 11:21:49 +0100 Subject: [PATCH 45/51] enhance copy_files to avoid need for copy_ecs_to_target --- easybuild/framework/easyconfig/tools.py | 19 ------ easybuild/main.py | 15 ++--- easybuild/tools/filetools.py | 47 ++++++++++---- test/framework/filetools.py | 85 ++++++++++++++++++++++++- 4 files changed, 126 insertions(+), 40 deletions(-) diff --git a/easybuild/framework/easyconfig/tools.py b/easybuild/framework/easyconfig/tools.py index b8be79be36..5ca7fd6e6b 100644 --- a/easybuild/framework/easyconfig/tools.py +++ b/easybuild/framework/easyconfig/tools.py @@ -760,22 +760,3 @@ def det_copy_ec_specs(orig_paths, from_pr): raise EasyBuildError("Found multiple paths for %s in PR: %s", filename, pr_matches) return paths, target_path - - -def copy_ecs_to_target(determined_paths, target_path, prefix=False, target_is_dir=False): - """ - Copy list of easyconfigs to specified path - - :param determined_paths: paths to ecs to copy - :param target_path: target to copy files to - :param prefix: include message prefix characters - :param target_is_dir: target is always a directory - """ - if len(determined_paths) == 1 and not target_is_dir: - copy_file(determined_paths[0], target_path) - print_msg("%s copied to %s" % (os.path.basename(determined_paths[0]), target_path), prefix=prefix) - elif determined_paths: - copy_files(determined_paths, target_path) - print_msg("%d file(s) copied to %s" % (len(determined_paths), target_path), prefix=prefix) - else: - raise EasyBuildError("One of more files to copy should be specified!") diff --git a/easybuild/main.py b/easybuild/main.py index e2c85b0dec..b9f0031d4f 100644 --- a/easybuild/main.py +++ b/easybuild/main.py @@ -50,16 +50,15 @@ from easybuild.framework.easyconfig.easyconfig import clean_up_easyconfigs from easybuild.framework.easyconfig.easyconfig import fix_deprecated_easyconfigs, verify_easyconfig_filename from easybuild.framework.easyconfig.style import cmdline_easyconfigs_style_check -from easybuild.framework.easyconfig.tools import categorize_files_by_type, copy_ecs_to_target, dep_graph -from easybuild.framework.easyconfig.tools import det_copy_ec_specs, det_easyconfig_paths, dump_env_script -from easybuild.framework.easyconfig.tools import get_paths_for, parse_easyconfigs, review_pr, run_contrib_checks -from easybuild.framework.easyconfig.tools import skip_available +from easybuild.framework.easyconfig.tools import categorize_files_by_type, dep_graph, det_copy_ec_specs +from easybuild.framework.easyconfig.tools import det_easyconfig_paths, dump_env_script, get_paths_for +from easybuild.framework.easyconfig.tools import parse_easyconfigs, review_pr, run_contrib_checks, skip_available from easybuild.framework.easyconfig.tweak import obtain_ec_for, tweak from easybuild.tools.config import find_last_log, get_repository, get_repositorypath, build_option from easybuild.tools.containers.common import containerize from easybuild.tools.docs import list_software -from easybuild.tools.filetools import adjust_permissions, cleanup, dump_index, load_index, locate_files -from easybuild.tools.filetools import read_file, register_lock_cleanup_signal_handlers, write_file +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 new_pr_from_branch @@ -330,7 +329,7 @@ def main(args=None, logfile=None, do_build=None, testing=False, modtool=None): # so try to determine full path for those too via robot search path paths = locate_files(orig_paths, robot_path) - copy_ecs_to_target(paths, target_path) + copy_files(paths, target_path, target_single_file=True, allow_empty=False, verbose=True) elif options.fix_deprecated_easyconfigs: fix_deprecated_easyconfigs(determined_paths) @@ -428,7 +427,7 @@ def main(args=None, logfile=None, do_build=None, testing=False, modtool=None): if tweaked_ecs_in_all_ecs: # Clean them, then copy them clean_up_easyconfigs(tweaked_ecs_in_all_ecs) - copy_ecs_to_target(tweaked_ecs_in_all_ecs, target_path, target_is_dir=True) + copy_files(tweaked_ecs_in_all_ecs, target_path, allow_empty=False, verbose=True) # creating/updating PRs if pr_options: diff --git a/easybuild/tools/filetools.py b/easybuild/tools/filetools.py index 372ea88893..709ba0d0ec 100644 --- a/easybuild/tools/filetools.py +++ b/easybuild/tools/filetools.py @@ -2085,26 +2085,49 @@ def copy_file(path, target_path, force_in_dry_run=False): raise EasyBuildError("Failed to copy file %s to %s: %s", path, target_path, err) -def copy_files(paths, target_dir, force_in_dry_run=False): +def copy_files(paths, target_path, force_in_dry_run=False, target_single_file=False, allow_empty=True, verbose=False): """ - Copy list of files to specified target directory (which is created if it doesn't exist yet). + Copy list of files to specified target path. + Target directory is created if it doesn't exist yet. - :param filepaths: list of files to copy - :param target_dir: target directory to copy files into + :param paths: list of filepaths to copy + :param target_path: path to copy files to :param force_in_dry_run: force copying of files during dry run + :param target_single_file: if there's only a single file to copy, copy to a file at target path (not a directory) + :param allow_empty: allow empty list of paths to copy as input (if False: raise error on empty input list) + :param verbose: print a message to report copying of files """ + # dry run: just report copying, don't actually copy if not force_in_dry_run and build_option('extended_dry_run'): - dry_run_msg("copied files %s to %s" % (paths, target_dir)) - else: - if os.path.exists(target_dir): - if os.path.isdir(target_dir): - _log.info("Copying easyconfigs into existing directory %s...", target_dir) + if len(paths) == 1: + dry_run_msg("copied %s to %s" % (paths[0], target_path)) + else: + dry_run_msg("copied %d files to %s" % (len(paths), target_path)) + + # special case: single file to copy and target_single_file is True => copy to file + elif len(paths) == 1 and target_single_file: + copy_file(paths[0], target_path) + if verbose: + print_msg("%s copied to %s" % (os.path.basename(paths[0]), target_path), prefix=False) + + elif paths: + # check target path: if it exists it should be a directory; if it doesn't exist, we create it + if os.path.exists(target_path): + if os.path.isdir(target_path): + _log.info("Copying easyconfigs into existing directory %s...", target_path) else: - raise EasyBuildError("%s exists but is not a directory", target_dir) + raise EasyBuildError("%s exists but is not a directory", target_path) else: - mkdir(target_dir, parents=True) + mkdir(target_path, parents=True) + for path in paths: - copy_file(path, target_dir) + copy_file(path, target_path) + + if verbose: + print_msg("%d file(s) copied to %s" % (len(paths), target_path), prefix=False) + + elif not allow_empty: + raise EasyBuildError("One of more files to copy should be specified!") def copy_dir(path, target_path, force_in_dry_run=False, dirs_exist_ok=False, **kwargs): diff --git a/test/framework/filetools.py b/test/framework/filetools.py index 8081aef00c..f4295d44f6 100644 --- a/test/framework/filetools.py +++ b/test/framework/filetools.py @@ -1485,7 +1485,7 @@ def test_copy_file(self): else: # printing this message will make test suite fail in Travis/GitHub CI, # since we check for unexpected output produced by the tests - print("Skipping overwrite-file-owned-by-other-user copy_file test (%s is missing)", test_file_to_overwrite) + print("Skipping overwrite-file-owned-by-other-user copy_file test (%s is missing)" % test_file_to_overwrite) # also test behaviour of copy_file under --dry-run build_options = { @@ -1551,6 +1551,89 @@ def test_copy_files(self): error_pattern = "/toy-0.0.eb exists but is not a directory" self.assertErrorRegex(EasyBuildError, error_pattern, ft.copy_files, [bzip2_ec], copied_toy_ec) + # by default copy_files allows empty input list, but if allow_empty=False then an error is raised + ft.copy_files([], self.test_prefix) + error_pattern = 'One of more files to copy should be specified!' + self.assertErrorRegex(EasyBuildError, error_pattern, ft.copy_files, [], self.test_prefix, allow_empty=False) + + # test special case: copying a single file to a file target via target_single_file=True + target = os.path.join(self.test_prefix, 'target') + self.assertFalse(os.path.exists(target)) + ft.copy_files([toy_ec], target, target_single_file=True) + self.assertTrue(os.path.exists(target)) + self.assertTrue(os.path.isfile(target)) + self.assertEqual(toy_ec_txt, ft.read_file(target)) + + ft.remove_file(target) + + # default behaviour is to copy single file list to target *directory* + self.assertFalse(os.path.exists(target)) + ft.copy_files([toy_ec], target) + self.assertTrue(os.path.exists(target)) + self.assertTrue(os.path.isdir(target)) + copied_toy_ec = os.path.join(target, 'toy-0.0.eb') + self.assertTrue(os.path.exists(copied_toy_ec)) + self.assertEqual(toy_ec_txt, ft.read_file(copied_toy_ec)) + + ft.remove_dir(target) + + # test enabling verbose mode + self.mock_stderr(True) + self.mock_stdout(True) + ft.copy_files([toy_ec], target, verbose=True) + stderr, stdout = self.get_stderr(), self.get_stdout() + self.mock_stderr(False) + self.mock_stdout(False) + self.assertEqual(stderr, '') + regex = re.compile(r"^1 file\(s\) copied to .*/target") + self.assertTrue(regex.match(stdout), "Pattern '%s' should be found in: %s" % (regex.pattern, stdout)) + + ft.remove_dir(target) + + self.mock_stderr(True) + self.mock_stdout(True) + ft.copy_files([toy_ec], target, target_single_file=True, verbose=True) + stderr, stdout = self.get_stderr(), self.get_stdout() + self.mock_stderr(False) + self.mock_stdout(False) + self.assertEqual(stderr, '') + regex = re.compile(r"^toy-0\.0\.eb copied to .*/target") + self.assertTrue(regex.match(stdout), "Pattern '%s' should be found in: %s" % (regex.pattern, stdout)) + + ft.remove_file(target) + + # check behaviour under -x: only printing, no actual copying + init_config(build_options={'extended_dry_run': True}) + self.assertFalse(os.path.exists(target)) + self.assertFalse(os.path.exists(os.path.join(target, 'test.eb'))) + + self.mock_stderr(True) + self.mock_stdout(True) + ft.copy_files(['test.eb'], target) + stderr, stdout = self.get_stderr(), self.get_stdout() + self.mock_stderr(False) + self.mock_stdout(False) + + self.assertFalse(os.path.exists(os.path.join(target, 'test.eb'))) + self.assertEqual(stderr, '') + + regex = re.compile("^copied test.eb to .*/target") + self.assertTrue(regex.match(stdout), "Pattern '%s' should be found in: %s" % (regex.pattern, stdout)) + + self.mock_stderr(True) + self.mock_stdout(True) + ft.copy_files(['bar.eb', 'foo.eb'], target) + stderr, stdout = self.get_stderr(), self.get_stdout() + self.mock_stderr(False) + self.mock_stdout(False) + + self.assertFalse(os.path.exists(os.path.join(target, 'bar.eb'))) + self.assertFalse(os.path.exists(os.path.join(target, 'foo.eb'))) + self.assertEqual(stderr, '') + + regex = re.compile("^copied 2 files to .*/target") + self.assertTrue(regex.match(stdout), "Pattern '%s' should be found in: %s" % (regex.pattern, stdout)) + def test_copy_dir(self): """Test copy_dir function.""" testdir = os.path.dirname(os.path.abspath(__file__)) From 19402a667c811e451c44506616a0f07757667d43 Mon Sep 17 00:00:00 2001 From: Kenneth Hoste Date: Sun, 25 Oct 2020 11:28:29 +0100 Subject: [PATCH 46/51] clean up imports in easybuild/framework/easyconfig/tools.py --- easybuild/framework/easyconfig/tools.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/easybuild/framework/easyconfig/tools.py b/easybuild/framework/easyconfig/tools.py index 5ca7fd6e6b..8273d28265 100644 --- a/easybuild/framework/easyconfig/tools.py +++ b/easybuild/framework/easyconfig/tools.py @@ -53,7 +53,7 @@ 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 copy_file, copy_files, find_easyconfigs, is_patch_file, locate_files +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.multidiff import multidiff From e32d815a1f6d3f50937fa4aa84c5dd72ba881382 Mon Sep 17 00:00:00 2001 From: Kenneth Hoste Date: Sun, 25 Oct 2020 11:32:12 +0100 Subject: [PATCH 47/51] exit after copying when combining --copy-ec and --try-* --- easybuild/main.py | 2 ++ 1 file changed, 2 insertions(+) diff --git a/easybuild/main.py b/easybuild/main.py index b9f0031d4f..616aca23cc 100644 --- a/easybuild/main.py +++ b/easybuild/main.py @@ -429,6 +429,8 @@ def main(args=None, logfile=None, do_build=None, testing=False, modtool=None): clean_up_easyconfigs(tweaked_ecs_in_all_ecs) copy_files(tweaked_ecs_in_all_ecs, target_path, allow_empty=False, verbose=True) + clean_exit(logfile, eb_tmpdir, testing) + # creating/updating PRs if pr_options: if options.new_pr: From bc2d08eed709a6096a2f8ddfd933e7805bb64210 Mon Sep 17 00:00:00 2001 From: Kenneth Hoste Date: Thu, 26 Nov 2020 14:17:13 +0100 Subject: [PATCH 48/51] fix broken tests --- test/framework/options.py | 16 ++++++++++------ 1 file changed, 10 insertions(+), 6 deletions(-) diff --git a/test/framework/options.py b/test/framework/options.py index f47bbc2f37..27b0fd0888 100644 --- a/test/framework/options.py +++ b/test/framework/options.py @@ -989,7 +989,8 @@ def test_copy_ec(self): test_ec = os.path.join(self.test_prefix, 'test.eb') args = ['--copy-ec', 'toy-0.0.eb', test_ec] stdout = self.mocked_main(args) - self.assertEqual(stdout, 'toy-0.0.eb copied to %s' % test_ec) + regex = re.compile(r'.*/toy-0.0.eb copied to %s' % test_ec) + self.assertTrue(regex.search(stdout), "Pattern '%s' found in: %s" % (regex.pattern, stdout)) self.assertTrue(os.path.exists(test_ec)) self.assertEqual(toy_ec_txt, read_file(test_ec)) @@ -1003,7 +1004,8 @@ def test_copy_ec(self): args = ['--copy-ec', 'toy-0.0.eb', target_fn] stdout = self.mocked_main(args) - self.assertEqual(stdout, 'toy-0.0.eb copied to test.eb') + regex = re.compile(r'.*/toy-0.0.eb copied to test.eb') + self.assertTrue(regex.search(stdout), "Pattern '%s' found in: %s" % (regex.pattern, stdout)) change_dir(cwd) @@ -1015,7 +1017,8 @@ def test_copy_ec(self): mkdir(test_target_dir) args = ['--copy-ec', 'toy-0.0.eb', test_target_dir] stdout = self.mocked_main(args) - self.assertEqual(stdout, 'toy-0.0.eb copied to %s' % test_target_dir) + regex = re.compile(r'.*/toy-0.0.eb copied to %s' % test_target_dir) + self.assertTrue(regex.search(stdout), "Pattern '%s' found in: %s" % (regex.pattern, stdout)) copied_toy_ec = os.path.join(test_target_dir, 'toy-0.0.eb') self.assertTrue(os.path.exists(copied_toy_ec)) @@ -1067,7 +1070,7 @@ def check_copied_files(): self.assertEqual(len(os.listdir(os.getcwd())), 0) args = ['--copy-ec', 'toy-0.0.eb'] stdout = self.mocked_main(args) - regex = re.compile('toy-0.0.eb copied to .*/%s' % os.path.basename(test_working_dir)) + regex = re.compile('.*/toy-0.0.eb copied to .*/%s' % os.path.basename(test_working_dir)) self.assertTrue(regex.match(stdout), "Pattern '%s' found in: %s" % (regex.pattern, stdout)) copied_toy_cwd = os.path.join(test_working_dir, 'toy-0.0.eb') self.assertTrue(os.path.exists(copied_toy_cwd)) @@ -1075,7 +1078,7 @@ def check_copied_files(): # --copy-ec without arguments results in a proper error args = ['--copy-ec'] - error_pattern = "One of more files to copy should be specified!" + error_pattern = "One or more files to copy should be specified!" self.assertErrorRegex(EasyBuildError, error_pattern, self.eb_main, args, raise_error=True) def test_copy_ec_from_pr(self): @@ -1191,7 +1194,8 @@ def test_copy_ec_from_pr(self): args = ['--copy-ec', '--from-pr', '11521', test_ec] ec_pr11521 = "ExifTool-12.00-GCCcore-9.3.0.eb" stdout = self.mocked_main(args) - self.assertEqual(stdout, '%s copied to %s' % (ec_pr11521, test_ec)) + regex = re.compile(r'.*/%s copied to %s' % (ec_pr11521, test_ec)) + self.assertTrue(regex.search(stdout), "Pattern '%s' found in: %s" % (regex.pattern, stdout)) self.assertTrue(os.path.exists(test_ec)) remove_file(test_ec) From c37f6e692b729a2c23662cfe2241295c78a3223c Mon Sep 17 00:00:00 2001 From: Kenneth Hoste Date: Thu, 26 Nov 2020 17:46:23 +0100 Subject: [PATCH 49/51] fix double return in cache_aware_func that was introduced in merge with develop --- easybuild/tools/github.py | 2 -- 1 file changed, 2 deletions(-) diff --git a/easybuild/tools/github.py b/easybuild/tools/github.py index 95acda641c..abed829172 100644 --- a/easybuild/tools/github.py +++ b/easybuild/tools/github.py @@ -411,8 +411,6 @@ def cache_aware_func(pr, path=None, github_user=None, github_account=None, githu return cache_aware_func - return cache_aware_func - @pr_files_cache def fetch_files_from_pr(pr, path=None, github_user=None, github_account=None, github_repo=None): From 7bb0ecfafe6938fe7dc94b8a021540830d374f56 Mon Sep 17 00:00:00 2001 From: Kenneth Hoste Date: Sat, 28 Nov 2020 13:43:21 +0100 Subject: [PATCH 50/51] fix typo in comment --- easybuild/framework/easyconfig/tools.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/easybuild/framework/easyconfig/tools.py b/easybuild/framework/easyconfig/tools.py index 8273d28265..56a64c5588 100644 --- a/easybuild/framework/easyconfig/tools.py +++ b/easybuild/framework/easyconfig/tools.py @@ -736,7 +736,7 @@ def det_copy_ec_specs(orig_paths, from_pr): # check files touched by PR and see if the target directory for --copy-ec # corresponds to the name of one of these files; # if so we should copy the specified file(s) to the current working directory, - # since interpreting the last argument as target location is very unlikely incorrect in this case + # since interpreting the last argument as target location is very unlikely to be correct in this case pr_filenames = [os.path.basename(p) for p in pr_paths] if last_path in pr_filenames: paths = orig_paths[:] From 3ce315a646dcf0693a7f9014bea956941f0b935e Mon Sep 17 00:00:00 2001 From: ocaisa Date: Mon, 30 Nov 2020 14:17:51 +0100 Subject: [PATCH 51/51] also check file contents in test for --copy-ec --from-pr --- test/framework/options.py | 1 + 1 file changed, 1 insertion(+) diff --git a/test/framework/options.py b/test/framework/options.py index 27b0fd0888..620be17c34 100644 --- a/test/framework/options.py +++ b/test/framework/options.py @@ -1197,6 +1197,7 @@ def test_copy_ec_from_pr(self): regex = re.compile(r'.*/%s copied to %s' % (ec_pr11521, test_ec)) self.assertTrue(regex.search(stdout), "Pattern '%s' found in: %s" % (regex.pattern, stdout)) self.assertTrue(os.path.exists(test_ec)) + self.assertTrue("name = 'ExifTool'" in read_file(test_ec)) remove_file(test_ec) def test_dry_run(self):