Merge pull request #3795 from Flamefire/git-tags

boegel · web-flow · commit 5f77fa028624 · 2021-09-01T18:37:12.000+02:00
make sure git clone with a tag argument actually downloads a tag
diff --git a/easybuild/tools/filetools.py b/easybuild/tools/filetools.py
@@ -2503,32 +2503,65 @@ def get_source_tarball_from_git(filename, targetdir, git_config):
     # compose 'git clone' command, and run it
     clone_cmd = ['git', 'clone']
 
+    if not keep_git_dir:
+        # Speed up cloning by only fetching the most recent commit, not the whole history
+        # When we don't want to keep the .git folder there won't be a difference in the result
+        clone_cmd.extend(['--depth', '1'])
+
     if tag:
         clone_cmd.extend(['--branch', tag])
-
-    if recursive:
-        clone_cmd.append('--recursive')
+        if recursive:
+            clone_cmd.append('--recursive')
+    else:
+        # checkout is done separately below for specific commits
+        clone_cmd.append('--no-checkout')
 
     clone_cmd.append('%s/%s.git' % (url, repo_name))
 
     tmpdir = tempfile.mkdtemp()
     cwd = change_dir(tmpdir)
-    run.run_cmd(' '.join(clone_cmd), log_all=True, log_ok=False, simple=False, regexp=False)
+    run.run_cmd(' '.join(clone_cmd), log_all=True, simple=True, regexp=False)
 
     # if a specific commit is asked for, check it out
     if commit:
         checkout_cmd = ['git', 'checkout', commit]
         if recursive:
             checkout_cmd.extend(['&&', 'git', 'submodule', 'update', '--init', '--recursive'])
 
-        run.run_cmd(' '.join(checkout_cmd), log_all=True, log_ok=False, simple=False, regexp=False, path=repo_name)
+        run.run_cmd(' '.join(checkout_cmd), log_all=True, simple=True, regexp=False, path=repo_name)
+
+    elif not build_option('extended_dry_run'):
+        # If we wanted to get a tag make sure we actually got a tag and not a branch with the same name
+        # This doesn't make sense in dry-run mode as we don't have anything to check
+        cmd = 'git describe --exact-match --tags HEAD'
+        # Note: Disable logging to also disable the error handling in run_cmd
+        (out, ec) = run.run_cmd(cmd, log_ok=False, log_all=False, regexp=False, path=repo_name)
+        if ec != 0 or tag not in out.splitlines():
+            print_warning('Tag %s was not downloaded in the first try due to %s/%s containing a branch'
+                          ' with the same name. You might want to alert the maintainers of %s about that issue.',
+                          tag, url, repo_name, repo_name)
+            cmds = []
+
+            if not keep_git_dir:
+                # make the repo unshallow first;
+                # this is equivalent with 'git fetch -unshallow' in Git 1.8.3+
+                # (first fetch seems to do nothing, unclear why)
+                cmds.append('git fetch --depth=2147483647 && git fetch --depth=2147483647')
+
+            cmds.append('git checkout refs/tags/' + tag)
+            # Clean all untracked files, e.g. from left-over submodules
+            cmds.append('git clean --force -d -x')
+            if recursive:
+                cmds.append('git submodule update --init --recursive')
+            for cmd in cmds:
+                run.run_cmd(cmd, log_all=True, simple=True, regexp=False, path=repo_name)
 
     # create an archive and delete the git repo directory
     if keep_git_dir:
         tar_cmd = ['tar', 'cfvz', targetpath, repo_name]
     else:
         tar_cmd = ['tar', 'cfvz', targetpath, '--exclude', '.git', repo_name]
-    run.run_cmd(' '.join(tar_cmd), log_all=True, log_ok=False, simple=False, regexp=False)
+    run.run_cmd(' '.join(tar_cmd), log_all=True, simple=True, regexp=False)
 
     # cleanup (repo_name dir does not exist in dry run mode)
     change_dir(cwd)
diff --git a/test/framework/filetools.py b/test/framework/filetools.py
@@ -2537,62 +2537,8 @@ def test_diff_files(self):
     def test_get_source_tarball_from_git(self):
         """Test get_source_tarball_from_git function."""
 
-        git_config = {
-            'repo_name': 'testrepository',
-            'url': 'https://github.com/easybuilders',
-            'tag': 'main',
-        }
         target_dir = os.path.join(self.test_prefix, 'target')
 
-        try:
-            ft.get_source_tarball_from_git('test.tar.gz', target_dir, git_config)
-            # (only) tarball is created in specified target dir
-            self.assertTrue(os.path.isfile(os.path.join(target_dir, 'test.tar.gz')))
-            self.assertEqual(os.listdir(target_dir), ['test.tar.gz'])
-
-            del git_config['tag']
-            git_config['commit'] = '8456f86'
-            ft.get_source_tarball_from_git('test2.tar.gz', target_dir, git_config)
-            self.assertTrue(os.path.isfile(os.path.join(target_dir, 'test2.tar.gz')))
-            self.assertEqual(sorted(os.listdir(target_dir)), ['test.tar.gz', 'test2.tar.gz'])
-
-        except EasyBuildError as err:
-            if "Network is down" in str(err):
-                print("Ignoring download error in test_get_source_tarball_from_git, working offline?")
-            else:
-                raise err
-
-        git_config = {
-            'repo_name': 'testrepository',
-            'url': 'git@github.com:easybuilders',
-            'tag': 'master',
-        }
-        args = ['test.tar.gz', self.test_prefix, git_config]
-
-        for key in ['repo_name', 'url', 'tag']:
-            orig_value = git_config.pop(key)
-            if key == 'tag':
-                error_pattern = "Neither tag nor commit found in git_config parameter"
-            else:
-                error_pattern = "%s not specified in git_config parameter" % key
-            self.assertErrorRegex(EasyBuildError, error_pattern, ft.get_source_tarball_from_git, *args)
-            git_config[key] = orig_value
-
-        git_config['commit'] = '8456f86'
-        error_pattern = "Tag and commit are mutually exclusive in git_config parameter"
-        self.assertErrorRegex(EasyBuildError, error_pattern, ft.get_source_tarball_from_git, *args)
-        del git_config['commit']
-
-        git_config['unknown'] = 'foobar'
-        error_pattern = "Found one or more unexpected keys in 'git_config' specification"
-        self.assertErrorRegex(EasyBuildError, error_pattern, ft.get_source_tarball_from_git, *args)
-        del git_config['unknown']
-
-        args[0] = 'test.txt'
-        error_pattern = "git_config currently only supports filename ending in .tar.gz"
-        self.assertErrorRegex(EasyBuildError, error_pattern, ft.get_source_tarball_from_git, *args)
-        args[0] = 'test.tar.gz'
-
         # only test in dry run mode, i.e. check which commands would be executed without actually running them
         build_options = {
             'extended_dry_run': True,
@@ -2602,13 +2548,10 @@ def test_get_source_tarball_from_git(self):
 
         def run_check():
             """Helper function to run get_source_tarball_from_git & check dry run output"""
-            self.mock_stdout(True)
-            self.mock_stderr(True)
-            res = ft.get_source_tarball_from_git('test.tar.gz', target_dir, git_config)
-            stdout = self.get_stdout()
-            stderr = self.get_stderr()
-            self.mock_stdout(False)
-            self.mock_stderr(False)
+            with self.mocked_stdout_stderr():
+                res = ft.get_source_tarball_from_git('test.tar.gz', target_dir, git_config)
+                stdout = self.get_stdout()
+                stderr = self.get_stderr()
             self.assertEqual(stderr, '')
             regex = re.compile(expected)
             self.assertTrue(regex.search(stdout), "Pattern '%s' found in: %s" % (regex.pattern, stdout))
@@ -2619,58 +2562,138 @@ def run_check():
         git_config = {
             'repo_name': 'testrepository',
             'url': 'git@github.com:easybuilders',
-            'tag': 'master',
+            'tag': 'tag_for_tests',
         }
+        git_repo = {'git_repo': 'git@github.com:easybuilders/testrepository.git'}  # Just to make the below shorter
         expected = '\n'.join([
-            r'  running command "git clone --branch master git@github.com:easybuilders/testrepository.git"',
+            r'  running command "git clone --depth 1 --branch tag_for_tests %(git_repo)s"',
             r"  \(in .*/tmp.*\)",
             r'  running command "tar cfvz .*/target/test.tar.gz --exclude .git testrepository"',
             r"  \(in .*/tmp.*\)",
-        ])
+        ]) % git_repo
         run_check()
 
         git_config['recursive'] = True
         expected = '\n'.join([
-            r'  running command "git clone --branch master --recursive git@github.com:easybuilders/testrepository.git"',
+            r'  running command "git clone --depth 1 --branch tag_for_tests --recursive %(git_repo)s"',
             r"  \(in .*/tmp.*\)",
             r'  running command "tar cfvz .*/target/test.tar.gz --exclude .git testrepository"',
             r"  \(in .*/tmp.*\)",
-        ])
+        ]) % git_repo
         run_check()
 
         git_config['keep_git_dir'] = True
         expected = '\n'.join([
-            r'  running command "git clone --branch master --recursive git@github.com:easybuilders/testrepository.git"',
+            r'  running command "git clone --branch tag_for_tests --recursive %(git_repo)s"',
             r"  \(in .*/tmp.*\)",
             r'  running command "tar cfvz .*/target/test.tar.gz testrepository"',
             r"  \(in .*/tmp.*\)",
-        ])
+        ]) % git_repo
         run_check()
         del git_config['keep_git_dir']
 
         del git_config['tag']
         git_config['commit'] = '8456f86'
         expected = '\n'.join([
-            r'  running command "git clone --recursive git@github.com:easybuilders/testrepository.git"',
+            r'  running command "git clone --depth 1 --no-checkout %(git_repo)s"',
             r"  \(in .*/tmp.*\)",
             r'  running command "git checkout 8456f86 && git submodule update --init --recursive"',
             r"  \(in testrepository\)",
             r'  running command "tar cfvz .*/target/test.tar.gz --exclude .git testrepository"',
             r"  \(in .*/tmp.*\)",
-        ])
+        ]) % git_repo
         run_check()
 
         del git_config['recursive']
         expected = '\n'.join([
-            r'  running command "git clone git@github.com:easybuilders/testrepository.git"',
+            r'  running command "git clone --depth 1 --no-checkout %(git_repo)s"',
             r"  \(in .*/tmp.*\)",
             r'  running command "git checkout 8456f86"',
             r"  \(in testrepository\)",
             r'  running command "tar cfvz .*/target/test.tar.gz --exclude .git testrepository"',
             r"  \(in .*/tmp.*\)",
-        ])
+        ]) % git_repo
         run_check()
 
+        # Test with real data.
+        init_config()
+        git_config = {
+            'repo_name': 'testrepository',
+            'url': 'https://github.com/easybuilders',
+            'tag': 'branch_tag_for_test',
+        }
+
+        try:
+            res = ft.get_source_tarball_from_git('test.tar.gz', target_dir, git_config)
+            # (only) tarball is created in specified target dir
+            test_file = os.path.join(target_dir, 'test.tar.gz')
+            self.assertEqual(res, test_file)
+            self.assertTrue(os.path.isfile(test_file))
+            self.assertEqual(os.listdir(target_dir), ['test.tar.gz'])
+            # Check that we indeed downloaded the right tag
+            extracted_dir = tempfile.mkdtemp(prefix='extracted_dir')
+            extracted_repo_dir = ft.extract_file(test_file, extracted_dir, change_into_dir=False)
+            self.assertTrue(os.path.isfile(os.path.join(extracted_repo_dir, 'this-is-a-branch.txt')))
+            os.remove(test_file)
+
+            # use a tag that clashes with a branch name and make sure this is handled correctly
+            git_config['tag'] = 'tag_for_tests'
+            with self.mocked_stdout_stderr():
+                res = ft.get_source_tarball_from_git('test.tar.gz', target_dir, git_config)
+                stderr = self.get_stderr()
+            self.assertIn('Tag tag_for_tests was not downloaded in the first try', stderr)
+            self.assertEqual(res, test_file)
+            self.assertTrue(os.path.isfile(test_file))
+            # Check that we indeed downloaded the tag and not the branch
+            extracted_dir = tempfile.mkdtemp(prefix='extracted_dir')
+            extracted_repo_dir = ft.extract_file(test_file, extracted_dir, change_into_dir=False)
+            self.assertTrue(os.path.isfile(os.path.join(extracted_repo_dir, 'this-is-a-tag.txt')))
+
+            del git_config['tag']
+            git_config['commit'] = '8456f86'
+            res = ft.get_source_tarball_from_git('test2.tar.gz', target_dir, git_config)
+            test_file = os.path.join(target_dir, 'test2.tar.gz')
+            self.assertEqual(res, test_file)
+            self.assertTrue(os.path.isfile(test_file))
+            self.assertEqual(sorted(os.listdir(target_dir)), ['test.tar.gz', 'test2.tar.gz'])
+
+        except EasyBuildError as err:
+            if "Network is down" in str(err):
+                print("Ignoring download error in test_get_source_tarball_from_git, working offline?")
+            else:
+                raise err
+
+        git_config = {
+            'repo_name': 'testrepository',
+            'url': 'git@github.com:easybuilders',
+            'tag': 'tag_for_tests',
+        }
+        args = ['test.tar.gz', self.test_prefix, git_config]
+
+        for key in ['repo_name', 'url', 'tag']:
+            orig_value = git_config.pop(key)
+            if key == 'tag':
+                error_pattern = "Neither tag nor commit found in git_config parameter"
+            else:
+                error_pattern = "%s not specified in git_config parameter" % key
+            self.assertErrorRegex(EasyBuildError, error_pattern, ft.get_source_tarball_from_git, *args)
+            git_config[key] = orig_value
+
+        git_config['commit'] = '8456f86'
+        error_pattern = "Tag and commit are mutually exclusive in git_config parameter"
+        self.assertErrorRegex(EasyBuildError, error_pattern, ft.get_source_tarball_from_git, *args)
+        del git_config['commit']
+
+        git_config['unknown'] = 'foobar'
+        error_pattern = "Found one or more unexpected keys in 'git_config' specification"
+        self.assertErrorRegex(EasyBuildError, error_pattern, ft.get_source_tarball_from_git, *args)
+        del git_config['unknown']
+
+        args[0] = 'test.txt'
+        error_pattern = "git_config currently only supports filename ending in .tar.gz"
+        self.assertErrorRegex(EasyBuildError, error_pattern, ft.get_source_tarball_from_git, *args)
+        args[0] = 'test.tar.gz'
+
     def test_is_sha256_checksum(self):
         """Test for is_sha256_checksum function."""
         a_sha256_checksum = '44332000aa33b99ad1e00cbd1a7da769220d74647060a10e807b916d73ea27bc'
