launchpad-reviewers team mailing list archive
-
launchpad-reviewers team
-
Mailing list archive
-
Message #28298
[Merge] ~cjwatson/launchpad-buildd:git-clone-then-checkout into launchpad-buildd:master
Colin Watson has proposed merging ~cjwatson/launchpad-buildd:git-clone-then-checkout into launchpad-buildd:master.
Commit message:
Use "git checkout" rather than "git clone -b"
Requested reviews:
Launchpad code reviewers (launchpad-reviewers)
For more details, see:
https://code.launchpad.net/~cjwatson/launchpad-buildd/+git/launchpad-buildd/+merge/418543
"git clone -b" has awkward semantics: it doesn't accept fully-qualified ref paths (e.g. "refs/tags/1.0"), and it doesn't accept commit IDs. The latter in particular is a showstopper for CI builds, where we always want to build from a particular commit rather than from a branch or tag. Instead, use "git clone -n" to suppress the checkout phase, and run "git checkout" separately.
--
Your team Launchpad code reviewers is requested to review the proposed merge of ~cjwatson/launchpad-buildd:git-clone-then-checkout into launchpad-buildd:master.
diff --git a/debian/changelog b/debian/changelog
index 0b16e27..bd34f55 100644
--- a/debian/changelog
+++ b/debian/changelog
@@ -1,3 +1,10 @@
+launchpad-buildd (211) UNRELEASED; urgency=medium
+
+ * Use "git checkout" rather than "git clone -b", since that allows
+ checking out by commit ID.
+
+ -- Colin Watson <cjwatson@xxxxxxxxxx> Tue, 05 Apr 2022 14:11:21 +0100
+
launchpad-buildd (210) bionic; urgency=medium
* Stop building with dpkg-deb -Zgzip; we no longer need to install on
diff --git a/lpbuildd/target/tests/test_build_charm.py b/lpbuildd/target/tests/test_build_charm.py
index 42092ba..b110591 100644
--- a/lpbuildd/target/tests/test_build_charm.py
+++ b/lpbuildd/target/tests/test_build_charm.py
@@ -212,7 +212,11 @@ class TestBuildCharm(TestCase):
build_charm.repo()
self.assertThat(build_charm.backend.run.calls, MatchesListwise([
RanBuildCommand(
- ["git", "clone", "lp:foo", "test-image"], cwd="/home/buildd"),
+ ["git", "clone", "-n", "lp:foo", "test-image"],
+ cwd="/home/buildd"),
+ RanBuildCommand(
+ ["git", "checkout", "-q", "HEAD"],
+ cwd="/home/buildd/test-image"),
RanBuildCommand(
["git", "submodule", "update", "--init", "--recursive"],
cwd="/home/buildd/test-image"),
@@ -237,9 +241,12 @@ class TestBuildCharm(TestCase):
build_charm.repo()
self.assertThat(build_charm.backend.run.calls, MatchesListwise([
RanBuildCommand(
- ["git", "clone", "-b", "next", "lp:foo", "test-image"],
+ ["git", "clone", "-n", "lp:foo", "test-image"],
cwd="/home/buildd"),
RanBuildCommand(
+ ["git", "checkout", "-q", "next"],
+ cwd="/home/buildd/test-image"),
+ RanBuildCommand(
["git", "submodule", "update", "--init", "--recursive"],
cwd="/home/buildd/test-image"),
RanBuildCommand(
@@ -264,9 +271,12 @@ class TestBuildCharm(TestCase):
build_charm.repo()
self.assertThat(build_charm.backend.run.calls, MatchesListwise([
RanBuildCommand(
- ["git", "clone", "-b", "1.0", "lp:foo", "test-image"],
+ ["git", "clone", "-n", "lp:foo", "test-image"],
cwd="/home/buildd"),
RanBuildCommand(
+ ["git", "checkout", "-q", "refs/tags/1.0"],
+ cwd="/home/buildd/test-image"),
+ RanBuildCommand(
["git", "submodule", "update", "--init", "--recursive"],
cwd="/home/buildd/test-image"),
RanBuildCommand(
@@ -298,9 +308,12 @@ class TestBuildCharm(TestCase):
}
self.assertThat(build_charm.backend.run.calls, MatchesListwise([
RanBuildCommand(
- ["git", "clone", "lp:foo", "test-image"],
+ ["git", "clone", "-n", "lp:foo", "test-image"],
cwd="/home/buildd", **env),
RanBuildCommand(
+ ["git", "checkout", "-q", "HEAD"],
+ cwd="/home/buildd/test-image", **env),
+ RanBuildCommand(
["git", "submodule", "update", "--init", "--recursive"],
cwd="/home/buildd/test-image", **env),
RanBuildCommand(
diff --git a/lpbuildd/target/tests/test_build_oci.py b/lpbuildd/target/tests/test_build_oci.py
index 2d17ef3..add1f86 100644
--- a/lpbuildd/target/tests/test_build_oci.py
+++ b/lpbuildd/target/tests/test_build_oci.py
@@ -179,9 +179,12 @@ class TestBuildOCI(TestCase):
build_oci.repo()
self.assertThat(build_oci.backend.run.calls, MatchesListwise([
RanBuildCommand(
- ["git", "clone", "--depth", "1", "lp:foo", "test-image"],
+ ["git", "clone", "-n", "--depth", "1", "lp:foo", "test-image"],
cwd="/home/buildd"),
RanBuildCommand(
+ ["git", "checkout", "-q", "HEAD"],
+ cwd="/home/buildd/test-image"),
+ RanBuildCommand(
["git", "submodule", "update", "--init", "--recursive"],
cwd="/home/buildd/test-image"),
]))
@@ -198,8 +201,11 @@ class TestBuildOCI(TestCase):
build_oci.repo()
self.assertThat(build_oci.backend.run.calls, MatchesListwise([
RanBuildCommand(
- ["git", "clone", "--depth", "1", "-b", "next",
- "lp:foo", "test-image"], cwd="/home/buildd"),
+ ["git", "clone", "-n", "--depth", "1", "lp:foo", "test-image"],
+ cwd="/home/buildd"),
+ RanBuildCommand(
+ ["git", "checkout", "-q", "next"],
+ cwd="/home/buildd/test-image"),
RanBuildCommand(
["git", "submodule", "update", "--init", "--recursive"],
cwd="/home/buildd/test-image"),
@@ -218,8 +224,11 @@ class TestBuildOCI(TestCase):
build_oci.repo()
self.assertThat(build_oci.backend.run.calls, MatchesListwise([
RanBuildCommand(
- ["git", "clone", "--depth", "1", "-b", "1.0", "lp:foo",
- "test-image"], cwd="/home/buildd"),
+ ["git", "clone", "-n", "--depth", "1", "lp:foo", "test-image"],
+ cwd="/home/buildd"),
+ RanBuildCommand(
+ ["git", "checkout", "-q", "refs/tags/1.0"],
+ cwd="/home/buildd/test-image"),
RanBuildCommand(
["git", "submodule", "update", "--init", "--recursive"],
cwd="/home/buildd/test-image"),
@@ -245,9 +254,12 @@ class TestBuildOCI(TestCase):
}
self.assertThat(build_oci.backend.run.calls, MatchesListwise([
RanBuildCommand(
- ["git", "clone", "--depth", "1", "lp:foo", "test-image"],
+ ["git", "clone", "-n", "--depth", "1", "lp:foo", "test-image"],
cwd="/home/buildd", **env),
RanBuildCommand(
+ ["git", "checkout", "-q", "HEAD"],
+ cwd="/home/buildd/test-image", **env),
+ RanBuildCommand(
["git", "submodule", "update", "--init", "--recursive"],
cwd="/home/buildd/test-image", **env),
]))
diff --git a/lpbuildd/target/tests/test_build_snap.py b/lpbuildd/target/tests/test_build_snap.py
index 48fa416..d9025eb 100644
--- a/lpbuildd/target/tests/test_build_snap.py
+++ b/lpbuildd/target/tests/test_build_snap.py
@@ -186,7 +186,9 @@ class TestBuildSnap(TestCase):
build_snap.repo()
self.assertThat(build_snap.backend.run.calls, MatchesListwise([
RanBuildCommand(
- ["git", "clone", "lp:foo", "test-snap"], cwd="/build"),
+ ["git", "clone", "-n", "lp:foo", "test-snap"], cwd="/build"),
+ RanBuildCommand(
+ ["git", "checkout", "-q", "HEAD"], cwd="/build/test-snap"),
RanBuildCommand(
["git", "submodule", "update", "--init", "--recursive"],
cwd="/build/test-snap"),
@@ -211,8 +213,9 @@ class TestBuildSnap(TestCase):
build_snap.repo()
self.assertThat(build_snap.backend.run.calls, MatchesListwise([
RanBuildCommand(
- ["git", "clone", "-b", "next", "lp:foo", "test-snap"],
- cwd="/build"),
+ ["git", "clone", "-n", "lp:foo", "test-snap"], cwd="/build"),
+ RanBuildCommand(
+ ["git", "checkout", "-q", "next"], cwd="/build/test-snap"),
RanBuildCommand(
["git", "submodule", "update", "--init", "--recursive"],
cwd="/build/test-snap"),
@@ -238,8 +241,10 @@ class TestBuildSnap(TestCase):
build_snap.repo()
self.assertThat(build_snap.backend.run.calls, MatchesListwise([
RanBuildCommand(
- ["git", "clone", "-b", "1.0", "lp:foo", "test-snap"],
- cwd="/build"),
+ ["git", "clone", "-n", "lp:foo", "test-snap"], cwd="/build"),
+ RanBuildCommand(
+ ["git", "checkout", "-q", "refs/tags/1.0"],
+ cwd="/build/test-snap"),
RanBuildCommand(
["git", "submodule", "update", "--init", "--recursive"],
cwd="/build/test-snap"),
@@ -272,7 +277,11 @@ class TestBuildSnap(TestCase):
}
self.assertThat(build_snap.backend.run.calls, MatchesListwise([
RanBuildCommand(
- ["git", "clone", "lp:foo", "test-snap"], cwd="/build", **env),
+ ["git", "clone", "-n", "lp:foo", "test-snap"],
+ cwd="/build", **env),
+ RanBuildCommand(
+ ["git", "checkout", "-q", "HEAD"],
+ cwd="/build/test-snap", **env),
RanBuildCommand(
["git", "submodule", "update", "--init", "--recursive"],
cwd="/build/test-snap", **env),
diff --git a/lpbuildd/target/tests/test_generate_translation_templates.py b/lpbuildd/target/tests/test_generate_translation_templates.py
index 97f4dac..0ce26c8 100644
--- a/lpbuildd/target/tests/test_generate_translation_templates.py
+++ b/lpbuildd/target/tests/test_generate_translation_templates.py
@@ -244,9 +244,12 @@ class TestGenerateTranslationTemplates(TestCase):
self.assertThat(generator.backend.run.calls, MatchesListwise([
RanAptGet("install", "intltool", "git"),
RanCommand(
- ["git", "clone", "lp:~my/repository", "source-tree"],
+ ["git", "clone", "-n", "lp:~my/repository", "source-tree"],
cwd=self.home_dir, LANG="C.UTF-8", SHELL="/bin/sh"),
RanCommand(
+ ["git", "checkout", "-q", "HEAD"],
+ cwd=branch_dir, LANG="C.UTF-8", SHELL="/bin/sh"),
+ RanCommand(
["git", "submodule", "update", "--init", "--recursive"],
cwd=branch_dir, LANG="C.UTF-8", SHELL="/bin/sh"),
RanCommand(
diff --git a/lpbuildd/target/tests/test_run_ci.py b/lpbuildd/target/tests/test_run_ci.py
index d5cc266..7c91efe 100644
--- a/lpbuildd/target/tests/test_run_ci.py
+++ b/lpbuildd/target/tests/test_run_ci.py
@@ -152,7 +152,10 @@ class TestRunCIPrepare(TestCase):
run_ci_prepare.backend.run = FakeRevisionID("0" * 40)
run_ci_prepare.repo()
self.assertThat(run_ci_prepare.backend.run.calls, MatchesListwise([
- RanBuildCommand(["git", "clone", "lp:foo", "tree"], cwd="/build"),
+ RanBuildCommand(
+ ["git", "clone", "-n", "lp:foo", "tree"], cwd="/build"),
+ RanBuildCommand(
+ ["git", "checkout", "-q", "HEAD"], cwd="/build/tree"),
RanBuildCommand(
["git", "submodule", "update", "--init", "--recursive"],
cwd="/build/tree"),
@@ -176,8 +179,9 @@ class TestRunCIPrepare(TestCase):
run_ci_prepare.repo()
self.assertThat(run_ci_prepare.backend.run.calls, MatchesListwise([
RanBuildCommand(
- ["git", "clone", "-b", "next", "lp:foo", "tree"],
- cwd="/build"),
+ ["git", "clone", "-n", "lp:foo", "tree"], cwd="/build"),
+ RanBuildCommand(
+ ["git", "checkout", "-q", "next"], cwd="/build/tree"),
RanBuildCommand(
["git", "submodule", "update", "--init", "--recursive"],
cwd="/build/tree"),
@@ -201,7 +205,9 @@ class TestRunCIPrepare(TestCase):
run_ci_prepare.repo()
self.assertThat(run_ci_prepare.backend.run.calls, MatchesListwise([
RanBuildCommand(
- ["git", "clone", "-b", "1.0", "lp:foo", "tree"], cwd="/build"),
+ ["git", "clone", "-n", "lp:foo", "tree"], cwd="/build"),
+ RanBuildCommand(
+ ["git", "checkout", "-q", "refs/tags/1.0"], cwd="/build/tree"),
RanBuildCommand(
["git", "submodule", "update", "--init", "--recursive"],
cwd="/build/tree"),
@@ -232,7 +238,9 @@ class TestRunCIPrepare(TestCase):
}
self.assertThat(run_ci_prepare.backend.run.calls, MatchesListwise([
RanBuildCommand(
- ["git", "clone", "lp:foo", "tree"], cwd="/build", **env),
+ ["git", "clone", "-n", "lp:foo", "tree"], cwd="/build", **env),
+ RanBuildCommand(
+ ["git", "checkout", "-q", "HEAD"], cwd="/build/tree", **env),
RanBuildCommand(
["git", "submodule", "update", "--init", "--recursive"],
cwd="/build/tree", **env),
@@ -259,7 +267,7 @@ class TestRunCIPrepare(TestCase):
AnyMatch(RanSnap("install", "--classic", "lpcraft")),
AnyMatch(
RanBuildCommand(
- ["git", "clone", "lp:foo", "tree"], cwd="/build")),
+ ["git", "clone", "-n", "lp:foo", "tree"], cwd="/build")),
))
def test_run_install_fails(self):
diff --git a/lpbuildd/target/vcs.py b/lpbuildd/target/vcs.py
index a5c9be9..fadc2c3 100644
--- a/lpbuildd/target/vcs.py
+++ b/lpbuildd/target/vcs.py
@@ -70,34 +70,27 @@ class VCSOperationMixin(StatusOperationMixin):
cmd.insert(1, "-Ossl.cert_reqs=none")
else:
assert self.args.git_repository is not None
- cmd = ["git", "clone"]
+ cmd = ["git", "clone", "-n"]
if quiet:
cmd.append("-q")
if git_shallow_clone:
cmd.extend(["--depth", "1"])
- if self.args.git_path is not None:
- git_path = self.args.git_path
- # "git clone -b" is a bit odd: it takes either branches or
- # tags, but they must be in their short form, i.e. "master"
- # rather than "refs/heads/master" and "1.0" rather than
- # "refs/tags/1.0". There's thus room for ambiguity if a
- # repository has a branch and a tag with the same name (the
- # branch will win), but using tags in the first place is
- # pretty rare here and a name collision is rarer still.
- # Launchpad shortens branch names before sending them to us,
- # but not tag names.
- if git_path.startswith("refs/tags/"):
- git_path = git_path[len("refs/tags/"):]
- cmd.extend(["-b", git_path])
cmd.extend([self.args.git_repository, name])
if not self.ssl_verify:
env["GIT_SSL_NO_VERIFY"] = "1"
self.backend.run(cmd, cwd=cwd, env=full_env)
if self.args.git_repository is not None:
+ repository = os.path.join(cwd, name)
+ git_path = self.args.git_path
+ if self.args.git_path is None:
+ git_path = "HEAD"
+ self.backend.run(
+ ["git", "checkout", "-q", git_path],
+ cwd=repository, env=full_env)
try:
self.backend.run(
["git", "submodule", "update", "--init", "--recursive"],
- cwd=os.path.join(cwd, name), env=full_env)
+ cwd=repository, env=full_env)
except subprocess.CalledProcessError as e:
logger.error(
"'git submodule update --init --recursive failed with "
