Merge pull request #4797 from lexming/fix-git-clone

clean up use get_source_tarball_from_git` to use same checkout mechanism for creating tarball for Git repos with tags and commits

Author: boegel

easybuild/tools/filetools.py

@@ -2700,27 +2700,16 @@ def get_source_tarball_from_git(filename, target_dir, git_config):
     # prepare target directory and clone repository
     mkdir(target_dir, parents=True)
 
+    # compose base git command
+    git_cmd = 'git'
+    if extra_config_params is not None:
+        git_cmd_params = [f"-c {param}" for param in extra_config_params]
+        git_cmd += f" {' '.join(git_cmd_params)}"
+
     # compose 'git clone' command, and run it
-    if extra_config_params:
-        git_cmd = 'git ' + ' '.join(['-c %s' % param for param in extra_config_params])
-    else:
-        git_cmd = 'git'
     clone_cmd = [git_cmd, 'clone']
-
-    if not keep_git_dir and not commit:
-        # 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 recurse_submodules:
-            clone_cmd.extend(["--recurse-submodules='%s'" % pat for pat in recurse_submodules])
-    else:
-        # checkout is done separately below for specific commits
-        clone_cmd.append('--no-checkout')
+    # checkout is done separately below for specific commits
+    clone_cmd.append('--no-checkout')
 
     clone_cmd.append(f'{url}/{repo_name}.git')
 
@@ -2735,56 +2724,31 @@ def get_source_tarball_from_git(filename, target_dir, git_config):
     if clone_into:
         repo_name = clone_into
 
+    repo_dir = os.path.join(tmpdir, repo_name)
+
+    # compose checkout command
+    checkout_cmd = [git_cmd, 'checkout']
     # if a specific commit is asked for, check it out
     if commit:
-        checkout_cmd = [git_cmd, 'checkout', commit]
-
-        if recursive or recurse_submodules:
-            checkout_cmd.extend(['&&', git_cmd, 'submodule', 'update', '--init'])
-            if recursive:
-                checkout_cmd.append('--recursive')
-            if recurse_submodules:
-                checkout_cmd.extend(["--recurse-submodules='%s'" % pat for pat in recurse_submodules])
-
-        work_dir = os.path.join(tmpdir, repo_name) if repo_name else tmpdir
-        run_shell_cmd(' '.join(checkout_cmd), work_dir=work_dir, hidden=True, verbose_dry_run=True)
-
-    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 = f"{git_cmd} describe --exact-match --tags HEAD"
-        work_dir = os.path.join(tmpdir, repo_name) if repo_name else tmpdir
-        res = run_shell_cmd(cmd, fail_on_error=False, work_dir=work_dir, hidden=True, verbose_dry_run=True)
-
-        if res.exit_code != EasyBuildExit.SUCCESS or tag not in res.output.splitlines():
-            msg = f"Tag {tag} was not downloaded in the first try due to {url}/{repo_name} containing a branch"
-            msg += f" with the same name. You might want to alert the maintainers of {repo_name} about that issue."
-            print_warning(msg)
-
-            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(f"{git_cmd} fetch --depth=2147483647 && git fetch --depth=2147483647")
-
-            cmds.append(f"{git_cmd} checkout refs/tags/{tag}")
-            # Clean all untracked files, e.g. from left-over submodules
-            cmds.append(f"{git_cmd} clean --force -d -x")
-            if recursive:
-                cmds.append(f"{git_cmd} submodule update --init --recursive")
-            elif recurse_submodules:
-                cmds.append(f"{git_cmd} submodule update --init ")
-                cmds[-1] += ' '.join(["--recurse-submodules='%s'" % pat for pat in recurse_submodules])
-
-            for cmd in cmds:
-                run_shell_cmd(cmd, work_dir=work_dir, hidden=True, verbose_dry_run=True)
+        checkout_cmd.append(f"{commit}")
+    elif tag:
+        checkout_cmd.append(f"refs/tags/{tag}")
+
+    run_shell_cmd(' '.join(checkout_cmd), work_dir=repo_dir, hidden=True, verbose_dry_run=True)
+
+    if recursive or recurse_submodules:
+        submodule_cmd = [git_cmd, 'submodule', 'update', '--init']
+        if recursive:
+            submodule_cmd.append('--recursive')
+        if recurse_submodules:
+            submodule_pathspec = [f"':{submod_path}'" for submod_path in recurse_submodules]
+            submodule_cmd.extend(['--'] + submodule_pathspec)
+
+        run_shell_cmd(' '.join(submodule_cmd), work_dir=repo_dir, hidden=True, verbose_dry_run=True)
 
     # Create archive
-    repo_path = os.path.join(tmpdir, repo_name)
     reproducible = not keep_git_dir  # presence of .git directory renders repo unreproducible
-    archive_path = make_archive(repo_path, archive_file=filename, archive_dir=target_dir, reproducible=reproducible)
+    archive_path = make_archive(repo_dir, archive_file=filename, archive_dir=target_dir, reproducible=reproducible)
 
     # cleanup (repo_name dir does not exist in dry run mode)
     remove(tmpdir)

test/framework/filetools.py

@@ -2978,38 +2978,50 @@ def run_check():
         }
         string_args = {
             'git_repo': '[email protected]:easybuilders/testrepository.git',
+            'git_clone_cmd': 'git clone --no-checkout',
             'test_prefix': self.test_prefix,
         }
 
         expected = '\n'.join([
-            r'  running shell command "git clone --depth 1 --branch tag_for_tests {git_repo}"',
+            r'  running shell command "{git_clone_cmd} {git_repo}"',
             r"  \(in .*/tmp.*\)",
+            r'  running shell command "git checkout refs/tags/tag_for_tests"',
+            r"  \(in .*/{repo_name}\)",
             r"Archiving '.*/{repo_name}' into '{test_prefix}/target/test.tar.xz'...",
         ]).format(**string_args, repo_name='testrepository')
         run_check()
 
         git_config['clone_into'] = 'test123'
         expected = '\n'.join([
-            r'  running shell command "git clone --depth 1 --branch tag_for_tests {git_repo} test123"',
+            r'  running shell command "{git_clone_cmd} {git_repo} test123"',
             r"  \(in .*/tmp.*\)",
+            r'  running shell command "git checkout refs/tags/tag_for_tests"',
+            r"  \(in .*/{repo_name}\)",
             r"Archiving '.*/{repo_name}' into '{test_prefix}/target/test.tar.xz'...",
         ]).format(**string_args, repo_name='test123')
         run_check()
         del git_config['clone_into']
 
         git_config['recursive'] = True
         expected = '\n'.join([
-            r'  running shell command "git clone --depth 1 --branch tag_for_tests --recursive {git_repo}"',
+            r'  running shell command "{git_clone_cmd} {git_repo}"',
             r"  \(in .*/tmp.*\)",
+            r'  running shell command "git checkout refs/tags/tag_for_tests"',
+            r"  \(in .*/{repo_name}\)",
+            r'  running shell command "git submodule update --init --recursive"',
+            r"  \(in .*/{repo_name}\)",
             r"Archiving '.*/{repo_name}' into '{test_prefix}/target/test.tar.xz'...",
         ]).format(**string_args, repo_name='testrepository')
         run_check()
 
         git_config['recurse_submodules'] = ['!vcflib', '!sdsl-lite']
         expected = '\n'.join([
-            '  running shell command "git clone --depth 1 --branch tag_for_tests --recursive'
-            + ' --recurse-submodules=\'!vcflib\' --recurse-submodules=\'!sdsl-lite\' {git_repo}"',
+            r'  running shell command "{git_clone_cmd} {git_repo}"',
             r"  \(in .*/tmp.*\)",
+            r'  running shell command "git checkout refs/tags/tag_for_tests"',
+            r"  \(in .*/{repo_name}\)",
+            r'  running shell command "git submodule update --init --recursive -- \':!vcflib\' \':!sdsl-lite\'"',
+            r"  \(in .*/{repo_name}\)",
             r"Archiving '.*/{repo_name}' into '{test_prefix}/target/test.tar.xz'...",
         ]).format(**string_args, repo_name='testrepository')
         run_check()
@@ -3018,33 +3030,30 @@ def run_check():
             'submodule."fastahack".active=false',
             'submodule."sha1".active=false',
         ]
+        git_cmd_extra = 'git -c submodule."fastahack".active=false -c submodule."sha1".active=false'
         expected = '\n'.join([
-            '  running shell command "git -c submodule."fastahack".active=false -c submodule."sha1".active=false'
-            + ' clone --depth 1 --branch tag_for_tests --recursive'
-            + ' --recurse-submodules=\'!vcflib\' --recurse-submodules=\'!sdsl-lite\' {git_repo}"',
+            r'  running shell command "{git_cmd_extra} clone --no-checkout {git_repo}"',
             r"  \(in .*/tmp.*\)",
+            r'  running shell command "{git_cmd_extra} checkout refs/tags/tag_for_tests"',
+            r"  \(in .*/{repo_name}\)",
+            r'  running shell command "{git_cmd_extra} submodule update --init --recursive --'
+            + ' \':!vcflib\' \':!sdsl-lite\'"',
+            r"  \(in .*/{repo_name}\)",
             r"Archiving '.*/{repo_name}' into '{test_prefix}/target/test.tar.xz'...",
-        ]).format(**string_args, repo_name='testrepository')
+        ]).format(**string_args, repo_name='testrepository', git_cmd_extra=git_cmd_extra)
         run_check()
         del git_config['recurse_submodules']
         del git_config['extra_config_params']
 
-        git_config['keep_git_dir'] = True
-        expected = '\n'.join([
-            r'  running shell command "git clone --branch tag_for_tests --recursive {git_repo}"',
-            r"  \(in .*/tmp.*\)",
-            r"Archiving '.*/{repo_name}' into '{test_prefix}/target/test.tar.xz'...",
-        ]).format(**string_args, repo_name='testrepository')
-        run_check()
-        del git_config['keep_git_dir']
-
         del git_config['tag']
         git_config['commit'] = '8456f86'
         expected = '\n'.join([
             r'  running shell command "git clone --no-checkout {git_repo}"',
             r"  \(in .*/tmp.*\)",
-            r'  running shell command "git checkout 8456f86 && git submodule update --init --recursive"',
-            r"  \(in .*/testrepository\)",
+            r'  running shell command "git checkout 8456f86"',
+            r"  \(in .*/{repo_name}\)",
+            r'  running shell command "git submodule update --init --recursive"',
+            r"  \(in .*/{repo_name}\)",
             r"Archiving '.*/{repo_name}' into '{test_prefix}/target/test.tar.xz'...",
         ]).format(**string_args, repo_name='testrepository')
         run_check()
@@ -3053,9 +3062,10 @@ def run_check():
         expected = '\n'.join([
             r'  running shell command "git clone --no-checkout {git_repo}"',
             r"  \(in .*/tmp.*\)",
-            r'  running shell command "git checkout 8456f86 && git submodule update --init '
-            r"--recursive --recurse-submodules='!vcflib' --recurse-submodules='!sdsl-lite'\"",
-            r"  \(in .*/testrepository\)",
+            r'  running shell command "git checkout 8456f86"',
+            r"  \(in .*/{repo_name}\)",
+            r'  running shell command "git submodule update --init --recursive -- \':!vcflib\' \':!sdsl-lite\'"',
+            r"  \(in .*/{repo_name}\)",
             r"Archiving '.*/{repo_name}' into '{test_prefix}/target/test.tar.xz'...",
         ]).format(**string_args, repo_name='testrepository')
         run_check()
@@ -3064,9 +3074,9 @@ def run_check():
         del git_config['recurse_submodules']
         expected = '\n'.join([
             r'  running shell command "git clone --no-checkout {git_repo}"',
-            r"  \(in /.*\)",
+            r"  \(in .*\)",
             r'  running shell command "git checkout 8456f86"',
-            r"  \(in /.*/testrepository\)",
+            r"  \(in .*/{repo_name}\)",
             r"Archiving '.*/{repo_name}' into '{test_prefix}/target/test.tar.xz'...",
         ]).format(**string_args, repo_name='testrepository')
         run_check()
@@ -3116,21 +3126,21 @@ def run_check():
             with self.mocked_stdout_stderr():
                 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')))
+            self.assertFalse(os.path.isdir(os.path.join(extracted_repo_dir, '.git')))
             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', 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')
             with self.mocked_stdout_stderr():
                 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')))
+            self.assertFalse(os.path.isdir(os.path.join(extracted_repo_dir, '.git')))
 
             del git_config['tag']
             git_config['commit'] = '90366ea'
@@ -3139,15 +3149,74 @@ def run_check():
             self.assertEqual(res, test_file)
             self.assertTrue(os.path.isfile(test_file))
             test_tar_files.append(os.path.basename(test_file))
-            self.assertEqual(sorted(os.listdir(target_dir)), test_tar_files)
+            self.assertCountEqual(sorted(os.listdir(target_dir)), test_tar_files)
+            extracted_dir = tempfile.mkdtemp(prefix='extracted_dir')
+            with self.mocked_stdout_stderr():
+                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, 'README.md')))
+            self.assertFalse(os.path.isdir(os.path.join(extracted_repo_dir, '.git')))
 
             git_config['keep_git_dir'] = True
             res = ft.get_source_tarball_from_git('test3', target_dir, git_config)
             test_file = os.path.join(target_dir, 'test3.tar.xz')
             self.assertEqual(res, test_file)
             self.assertTrue(os.path.isfile(test_file))
             test_tar_files.append(os.path.basename(test_file))
-            self.assertEqual(sorted(os.listdir(target_dir)), test_tar_files)
+            self.assertCountEqual(sorted(os.listdir(target_dir)), test_tar_files)
+            extracted_dir = tempfile.mkdtemp(prefix='extracted_dir')
+            with self.mocked_stdout_stderr():
+                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, 'README.md')))
+            self.assertTrue(os.path.isdir(os.path.join(extracted_repo_dir, '.git')))
+
+            del git_config['keep_git_dir']
+            git_config['commit'] = '17a551c'
+            git_config['recursive'] = True
+            res = ft.get_source_tarball_from_git('test_recursive', target_dir, git_config)
+            test_file = os.path.join(target_dir, 'test_recursive.tar.xz')
+            self.assertEqual(res, test_file)
+            self.assertTrue(os.path.isfile(test_file))
+            test_tar_files.append(os.path.basename(test_file))
+            self.assertCountEqual(sorted(os.listdir(target_dir)), test_tar_files)
+            extracted_dir = tempfile.mkdtemp(prefix='extracted_dir')
+            with self.mocked_stdout_stderr():
+                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, 'README.md')))
+            self.assertFalse(os.path.isdir(os.path.join(extracted_repo_dir, '.git')))
+            self.assertTrue(os.path.isdir(os.path.join(extracted_repo_dir, 'easybuilders.github.io')))
+            self.assertTrue(os.path.isfile(os.path.join(extracted_repo_dir, 'easybuilders.github.io', 'index.html')))
+
+            git_config['commit'] = '17a551c'
+            git_config['recurse_submodules'] = ['easybuilders.github.io']
+            res = ft.get_source_tarball_from_git('test_submodules', target_dir, git_config)
+            test_file = os.path.join(target_dir, 'test_submodules.tar.xz')
+            self.assertEqual(res, test_file)
+            self.assertTrue(os.path.isfile(test_file))
+            test_tar_files.append(os.path.basename(test_file))
+            self.assertCountEqual(sorted(os.listdir(target_dir)), test_tar_files)
+            extracted_dir = tempfile.mkdtemp(prefix='extracted_dir')
+            with self.mocked_stdout_stderr():
+                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, 'README.md')))
+            self.assertFalse(os.path.isdir(os.path.join(extracted_repo_dir, '.git')))
+            self.assertTrue(os.path.isdir(os.path.join(extracted_repo_dir, 'easybuilders.github.io')))
+            self.assertTrue(os.path.isfile(os.path.join(extracted_repo_dir, 'easybuilders.github.io', 'index.html')))
+
+            git_config['commit'] = '17a551c'
+            git_config['recurse_submodules'] = ['!easybuilders.github.io']
+            res = ft.get_source_tarball_from_git('test_exclude_submodules', target_dir, git_config)
+            test_file = os.path.join(target_dir, 'test_exclude_submodules.tar.xz')
+            self.assertEqual(res, test_file)
+            self.assertTrue(os.path.isfile(test_file))
+            test_tar_files.append(os.path.basename(test_file))
+            self.assertCountEqual(sorted(os.listdir(target_dir)), test_tar_files)
+            extracted_dir = tempfile.mkdtemp(prefix='extracted_dir')
+            with self.mocked_stdout_stderr():
+                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, 'README.md')))
+            self.assertFalse(os.path.isdir(os.path.join(extracted_repo_dir, '.git')))
+            self.assertTrue(os.path.isdir(os.path.join(extracted_repo_dir, 'easybuilders.github.io')))
+            self.assertFalse(os.path.isfile(os.path.join(extracted_repo_dir, 'easybuilders.github.io', 'index.html')))
 
         except EasyBuildError as err:
             if "Network is down" in str(err):