Merge lp:~lifeless/bzr-builder/bug-469874 into lp:~james-w/bzr-builder/trunk-old

[Robert Collins]

[Robert Collins (lifeless)]

This permits recipes that need less gardening.

[lp:~lifeless/bzr-builder/bug-469874]

59. By Robert Collins on 2009-11-13

Merge trunk

[James Westby (james-w)]

A few comments that could be fixed when merging:

  * I don't like "debupstream," as I think it's slightly ambiguous,
    e.g. is it the upstream part of the debian upstream version number?
    I would go for "dch" I think.
  * "{debupstream}" is repeated lots of times in the code. Sticking it
    in a variable somewhere would be good, probably pulling out the others
    too.
  * BaseRecipeBranch has methods for substituting time and revnos, but this
    does it externally. I think a method should be used for consistency.

18 + raise errors.BzrCommandError("No previous changelog to "
19 + "take the upstream version from - {debupstream} was "
20 + "used.")

I'd replace "-" with "as".

62 + def _substitute_stuff

I don't like the use of "stuff", and the method does more than just substitute.

176 + finally:
177 + # package_dir -> working_directory
178 + os.rename(package_dir, working_directory)

Should exceptions from the rename be caught, warned about, then swallowed
if handling an exception?
We have had issues in the past with errors in unwind code masking the real
issue.

I have some uneasiness about the whole idea, given that you have to look
at the built tree to know the version number, but I think it is worth it.
Looking in the revision trees at resolve_revisions time would be possible,
but knowing which version will actually end up on disk isn't really possible.

The only big question I have is whether it should be possible/enforced
to use "{dch:branch-id}" here. Then you could rid the concern above
if it was enforced. I'm not sure whether you would want to use a changelog
from a branch that wasn't going to put the changelog on disk though.

I think this is good to land with the tweaks above, and we can always
bump format to enforce a branch id if it is a good idea.

I'm not sure whether we should be bumping format for the addition of a
{} variable. I think we should.

Thanks,

James

[James Westby (james-w)]

Voting needs-fixing, but unless we bump the format version the
tweaks could be made on merging.

Thanks,

James

review: Needs Fixing

[Robert Collins (lifeless)]

On Mon, 2009-11-23 at 16:35 +0000, James Westby wrote:
> A few comments that could be fixed when merging:
>
> * I don't like "debupstream," as I think it's slightly ambiguous,
> e.g. is it the upstream part of the debian upstream version number?
> I would go for "dch" I think.

Do you mean 'dchupstream' ? Just dch would be very unclear I think.

> * "{debupstream}" is repeated lots of times in the code. Sticking it
> in a variable somewhere would be good, probably pulling out the others
> too.

Sure.

> * BaseRecipeBranch has methods for substituting time and revnos, but this
> does it externally. I think a method should be used for consistency.

We can do that as well; BRB doesn't really have strong encapsulation of
this though, and as this substitution happens after building it didn't
seem to fit to me, BRB seemed to want to work on the model, this is on
the concrete contents.

> 18 + raise errors.BzrCommandError("No previous changelog to "
> 19 + "take the upstream version from - {debupstream} was "
> 20 + "used.")
>
> I'd replace "-" with "as".
>
> 62 + def _substitute_stuff
>
> I don't like the use of "stuff", and the method does more than just substitute.

Yeah, we didn't know what to call it. Suggestions welcome.

> 176 + finally:
> 177 + # package_dir -> working_directory
> 178 + os.rename(package_dir, working_directory)
>
> Should exceptions from the rename be caught, warned about, then swallowed
> if handling an exception?
> We have had issues in the past with errors in unwind code masking the real
> issue.

We could, but thats really not trivial to do well. There is some
supporting code in bzr core to help with this now. OTOH if the rename
fails I think people should know about it: package_dir will be in the
way of later runs.

> I have some uneasiness about the whole idea, given that you have to look
> at the built tree to know the version number, but I think it is worth it.
> Looking in the revision trees at resolve_revisions time would be possible,
> but knowing which version will actually end up on disk isn't really possible.

You'd have to do the merge first. And with the also desired 'get version
from configure' feature you'd have to simulate m4/shell ;).

> The only big question I have is whether it should be possible/enforced
> to use "{dch:branch-id}" here. Then you could rid the concern above
> if it was enforced. I'm not sure whether you would want to use a changelog
> from a branch that wasn't going to put the changelog on disk though.

I don't know what {dch:branch-id} would mean.

> I think this is good to land with the tweaks above, and we can always
> bump format to enforce a branch id if it is a good idea.
>
> I'm not sure whether we should be bumping format for the addition of a
> {} variable. I think we should.

I don't think you need to:
Old versions will always error (they want the string fully substituted)
New versions will work.

-Rob

[James Westby (james-w)]

On Mon Nov 23 21:03:09 UTC 2009 Robert Collins wrote:
> On Mon, 2009-11-23 at 16:35 +0000, James Westby wrote:
> > A few comments that could be fixed when merging:
> >
> > * I don't like "debupstream," as I think it's slightly ambiguous,
> > e.g. is it the upstream part of the debian upstream version number?
> > I would go for "dch" I think.
>
> Do you mean 'dchupstream' ? Just dch would be very unclear I think.

No, I meant 'dch', being a fairly standard abbreviation of 'debian changelog.'
'debchangelog' would work for me too, it's the 'upstream' part that is
mainly confusing to me.

> > 18 + raise errors.BzrCommandError("No previous changelog to "
> > 19 + "take the upstream version from - {debupstream} was "
> > 20 + "used.")
> >
> > I'd replace "-" with "as".
> >
> > 62 + def _substitute_stuff
> >
> > I don't like the use of "stuff", and the method does more than just substitute.
>
> Yeah, we didn't know what to call it. Suggestions welcome.

Something like "get_prepared_branch_from_recipe" would seem to be closer to
the functions it performs.

> > 176 + finally:
> > 177 + # package_dir -> working_directory
> > 178 + os.rename(package_dir, working_directory)
> >
> > Should exceptions from the rename be caught, warned about, then swallowed
> > if handling an exception?
> > We have had issues in the past with errors in unwind code masking the real
> > issue.
>
> We could, but thats really not trivial to do well. There is some
> supporting code in bzr core to help with this now. OTOH if the rename
> fails I think people should know about it: package_dir will be in the
> way of later runs.

Well, the way it is currently coded, the user will only know about one
of the issues, if you want them to know about both then we should write
it such that they will know about both (as I suggested).

> > The only big question I have is whether it should be possible/enforced
> > to use "{dch:branch-id}" here. Then you could rid the concern above
> > if it was enforced. I'm not sure whether you would want to use a changelog
> > from a branch that wasn't going to put the changelog on disk though.
>
> I don't know what {dch:branch-id} would mean.

Get the tree of the desired revision of the branch known as 'branch-id' in the
recipe, read the debian/changelog in that, and extract the version from there.

Thanks,

James

[Robert Collins (lifeless)]

On Mon, 2009-11-23 at 21:45 +0000, James Westby wrote:
> On Mon Nov 23 21:03:09 UTC 2009 Robert Collins wrote:
> > On Mon, 2009-11-23 at 16:35 +0000, James Westby wrote:
> > > A few comments that could be fixed when merging:
> > >
> > > * I don't like "debupstream," as I think it's slightly ambiguous,
> > > e.g. is it the upstream part of the debian upstream version number?
> > > I would go for "dch" I think.
> >
> > Do you mean 'dchupstream' ? Just dch would be very unclear I think.
>
> No, I meant 'dch', being a fairly standard abbreviation of 'debian changelog.'
> 'debchangelog' would work for me too, it's the 'upstream' part that is
> mainly confusing to me.

Debian version numbers are structured and contain an upstream component
and a debian specific component. The code in this patch only uses the
upstream component. Both 'upstream' and 'debian changelog' are key
concepts to communicate in the {} variable name. Just either would be
incorrect and misleading.

> > > The only big question I have is whether it should be possible/enforced
> > > to use "{dch:branch-id}" here. Then you could rid the concern above
> > > if it was enforced. I'm not sure whether you would want to use a changelog
> > > from a branch that wasn't going to put the changelog on disk though.
> >
> > I don't know what {dch:branch-id} would mean.
>
> Get the tree of the desired revision of the branch known as 'branch-id' in the
> recipe, read the debian/changelog in that, and extract the version from there.

I think that that would be more awkward, because you'd have to know
where the final changelog was coming from.

-Rob

[James Westby (james-w)]

On Mon Nov 23 21:57:10 UTC 2009 Robert Collins wrote:
> Debian version numbers are structured and contain an upstream component
> and a debian specific component. The code in this patch only uses the
> upstream component. Both 'upstream' and 'debian changelog' are key
> concepts to communicate in the {} variable name. Just either would be
> incorrect and misleading.

In that case then your name makes more sense. I said it was confusing,
and gave exactly that as the possible interpretation, as I didn't think
it was that. I thought it would take the whole version number.

I think this discussion shows that the change needs some tweaks to the
docs to explain what it is and when you would use it.

Thanks,

James

[James Westby (james-w)]

Fixes done in a local branch and merged.

Thanks,

James

review: Approve

=== modified file '__init__.py'
--- __init__.py	2009-10-23 13:14:30 +0000
+++ __init__.py	2009-11-13 06:05:22 +0000
@@ -272,11 +272,19 @@
             distribution = cl._blocks[0].distributions.split()[0]
         if package is None:
             package = cl._blocks[0].package
+        if "{debupstream}" in base_branch.deb_version:
+            cl_version = cl._blocks[0].version
+            base_branch.deb_version = base_branch.deb_version.replace(
+                "{debupstream}", cl_version.upstream_version)
     else:
         if package is None:
             raise errors.BzrCommandError("No previous changelog to "
                     "take the package name from, and --package not "
                     "specified.")
+        if "{debupstream}" in base_branch.deb_version:
+            raise errors.BzrCommandError("No previous changelog to "
+                    "take the upstream version from - {debupstream} was "
+                    "used.")
         if distribution is None:
             distribution = "jaunty"
     # Use debian packaging environment variables
@@ -338,33 +346,6 @@
                 "%s" % output)
 
 
-def get_base_branch(recipe_file, if_changed_from=None):
-    base_branch = get_branch_from_recipe_file(recipe_file)
-    time = datetime.datetime.utcnow()
-    base_branch.substitute_time(time)
-    old_recipe = None
-    if if_changed_from is not None:
-        old_recipe = get_old_recipe(if_changed_from)
-    changed = resolve_revisions(base_branch, if_changed_from=old_recipe)
-    if not changed:
-        return None
-    return base_branch
-
-
-def build_one_recipe(recipe_file, working_directory, manifest=None,
-        if_changed_from=None):
-    base_branch = get_base_branch(recipe_file, if_changed_from=if_changed_from)
-    if base_branch is None:
-        trace.note("Unchanged")
-        return 0
-    build_tree(base_branch, working_directory)
-    if manifest is not None:
-        write_manifest_to_path(manifest, base_branch)
-    else:
-        write_manifest_to_path(os.path.join(working_directory,
-                    "bzr-builder.manifest"), base_branch)
-
-
 class cmd_build(Command):
     """Build a tree based on a 'recipe'.
 
@@ -381,10 +362,36 @@
                         "to that specified in the specified manifest."),
                     ]
 
+    def _substitute_stuff(self, recipe_file, if_changed_from):
+        """Common code to substitute stuff
+        
+        :return: A tuple with (retcode, base_branch). If retcode is None
+            then the command execution should continue.
+        """
+        base_branch = get_branch_from_recipe_file(recipe_file)
+        time = datetime.datetime.utcnow()
+        base_branch.substitute_time(time)
+        old_recipe = None
+        if if_changed_from is not None:
+            old_recipe = get_old_recipe(if_changed_from)
+        # Save the unsubstituted version for dailydeb.
+        self._template_version = base_branch.deb_version
+        changed = resolve_revisions(base_branch, if_changed_from=old_recipe)
+        if not changed:
+            trace.note("Unchanged")
+            return 0, base_branch
+        return None, base_branch
+
     def run(self, recipe_file, working_directory, manifest=None,
             if_changed_from=None):
-        return build_one_recipe(recipe_file, working_directory,
-                manifest=manifest, if_changed_from=if_changed_from)
+        result, base_branch = self._substitute_stuff(recipe_file,
+            if_changed_from)
+        if result is not None:
+            return result
+        manifest_path = manifest or os.path.join(working_directory,
+                        "bzr-builder.manifest")
+        build_tree(base_branch, working_directory)
+        write_manifest_to_path(manifest_path, base_branch)
 
 
 register_command(cmd_build)
@@ -418,52 +425,78 @@
                             "must be specified if you use --dput."),
             ]
 
-    takes_args = ["recipe_file", "working_directory?"]
+    takes_args = ["recipe_file", "working_basedir?"]
 
-    def run(self, recipe_file, working_directory=None, manifest=None,
+    def run(self, recipe_file, working_basedir=None, manifest=None,
             if_changed_from=None, package=None, distribution=None,
             dput=None, key_id=None):
 
         if dput is not None and key_id is None:
             raise errors.BzrCommandError("You must specify --key-id if you "
                     "specify --dput.")
-
-        base_branch = get_base_branch(recipe_file, if_changed_from=if_changed_from)
-        if base_branch is None:
-            trace.note("Unchanged")
-            return 0
-        recipe_name = os.path.basename(recipe_file)
-        if recipe_name.endswith(".recipe"):
-            recipe_name = recipe_name[:-len(".recipe")]
-        version = base_branch.deb_version
-        if "-" in version:
-            version = version[:version.rindex("-")]
-        package_basedir = "%s-%s" % (package or recipe_name, version)
-        if working_directory is None:
+        result, base_branch = self._substitute_stuff(recipe_file,
+            if_changed_from)
+        if result is not None:
+            return result
+        if working_basedir is None:
             temp_dir = tempfile.mkdtemp(prefix="bzr-builder-")
-            working_directory = temp_dir
+            working_basedir = temp_dir
         else:
             temp_dir = None
-            if not os.path.exists(working_directory):
-                os.makedirs(working_directory)
+            if not os.path.exists(working_basedir):
+                os.makedirs(working_basedir)
+        self._calculate_package_name(recipe_file, package)
+        working_directory = os.path.join(working_basedir,
+            "%s-%s" % (self._package_name, self._template_version))
         try:
-            package_dir = os.path.join(working_directory, package_basedir)
-            build_tree(base_branch, package_dir)
-            write_manifest_to_path(os.path.join(package_dir, "debian",
-                        "bzr-builder.manifest"), base_branch)
-            add_changelog_entry(base_branch, package_dir,
-                    distribution=distribution, package=package)
-            build_source_package(package_dir)
-            if key_id is not None:
-                sign_source_package(package_dir, key_id)
-            if dput is not None:
-                dput_source_package(package_dir, dput)
+            # we want to use a consistent package_dir always to support
+            # updates in place, but debuild etc want PACKAGE-UPSTREAMVERSION
+            # on disk, so we build_tree with the unsubstituted version number
+            # and do a final rename-to step before calling into debian build
+            # tools. We then rename the working dir back.
+            manifest_path = os.path.join(working_directory, "debian",
+                "bzr-builder.manifest")
+            build_tree(base_branch, working_directory)
+            write_manifest_to_path(manifest_path, base_branch)
+            # Add changelog also substitutes {debupstream}.
+            add_changelog_entry(base_branch, working_directory,
+                distribution=distribution, package=package)
+            package_dir = self._calculate_package_dir(base_branch,
+                working_basedir)
+            # working_directory -> package_dir: after this debian stuff works.
+            os.rename(working_directory, package_dir)
+            try:
+                build_source_package(package_dir)
+                if key_id is not None:
+                    sign_source_package(package_dir, key_id)
+                if dput is not None:
+                    dput_source_package(package_dir, dput)
+            finally:
+                # package_dir -> working_directory
+                os.rename(package_dir, working_directory)
+            # Note that this may write a second manifest.
             if manifest is not None:
                 write_manifest_to_path(manifest, base_branch)
         finally:
             if temp_dir is not None:
                 shutil.rmtree(temp_dir)
 
+    def _calculate_package_dir(self, base_branch, working_basedir):
+        """Calculate the directory name that should be used while debuilding."""
+        version = base_branch.deb_version
+        if "-" in version:
+            version = version[:version.rindex("-")]
+        package_basedir = "%s-%s" % (self._package_name, version)
+        package_dir = os.path.join(working_basedir, package_basedir)
+        return package_dir
+
+    def _calculate_package_name(self, recipe_file, package):
+        """Calculate the directory name that should be used while debuilding."""
+        recipe_name = os.path.basename(recipe_file)
+        if recipe_name.endswith(".recipe"):
+            recipe_name = recipe_name[:-len(".recipe")]
+        self._package_name = package or recipe_name
+
 
 register_command(cmd_dailydeb)
 

=== modified file 'recipe.py'
--- recipe.py	2009-08-18 21:16:17 +0000
+++ recipe.py	2009-11-13 06:05:22 +0000
@@ -226,7 +226,7 @@
         def get_revno():
             try:
                 revno = br_from.revision_id_to_revno(revision_id)
-                return "%s" % revno
+                return str(revno)
             except errors.NoSuchRevision:
                 # We need to load and use the full revno map after all
                 result = br_from.get_revision_id_to_revno_map().get(
@@ -288,7 +288,11 @@
             if_changed_from=if_changed_from_revisions)
     if not changed:
         changed = changed_revisions
-    if "{" in base_branch.deb_version:
+    ok_to_preserve = ["{debupstream}"]
+    checked_version = base_branch.deb_version
+    for token in ok_to_preserve:
+        checked_version = checked_version.replace(token, "")
+    if "{" in checked_version:
         raise errors.BzrCommandError("deb-version not fully "
                 "expanded: %s" % base_branch.deb_version)
     if if_changed_from is not None and not changed:

=== modified file 'tests/test_blackbox.py'
--- tests/test_blackbox.py	2009-09-21 18:07:53 +0000
+++ tests/test_blackbox.py	2009-11-13 06:05:22 +0000
@@ -155,6 +155,7 @@
 
     def test_cmd_dailydeb_no_work_dir(self):
         #TODO: define a test feature for debuild and require it here.
+        self.permit_dir('/') # Allow the made working dir to be accessed.
         source = self.make_branch_and_tree("source")
         self.build_tree(["source/a", "source/debian/"])
         self.build_tree_contents([("source/debian/rules",
@@ -170,6 +171,7 @@
 
     def test_cmd_dailydeb_if_changed_from_non_existant(self):
         #TODO: define a test feature for debuild and require it here.
+        self.permit_dir('/') # Allow the made working dir to be accessed.
         source = self.make_branch_and_tree("source")
         self.build_tree(["source/a", "source/debian/"])
         self.build_tree_contents([("source/debian/rules",
@@ -183,8 +185,7 @@
         out, err = self.run_bzr("dailydeb test.recipe "
                 "--manifest manifest --package foo --if-changed-from bar")
 
-    def test_cmd_dailydeb_with_package_from_changelog(self):
-        #TODO: define a test feature for debuild and require it here.
+    def make_simple_package(self):
         source = self.make_branch_and_tree("source")
         self.build_tree(["source/a", "source/debian/"])
         cl_contents = ("package (0.1-1) unstable; urgency=low\n  * foo\n"
@@ -197,7 +198,12 @@
                 ("source/debian/changelog", cl_contents)])
         source.add(["a", "debian/", "debian/rules", "debian/control",
                 "debian/changelog"])
-        revid = source.commit("one")
+        source.commit("one")
+        return source
+
+    def test_cmd_dailydeb_with_package_from_changelog(self):
+        #TODO: define a test feature for debuild and require it here.
+        source = self.make_simple_package()
         self.build_tree_contents([("test.recipe", "# bzr-builder format 0.1 "
                     "deb-version 1\nsource 1\n")])
         out, err = self.run_bzr("dailydeb test.recipe "
@@ -209,5 +215,18 @@
             actual_cl_contents = f.read()
         finally:
             f.close()
-        self.assertEqual(new_cl_contents,
-                actual_cl_contents[:len(new_cl_contents)])
+        self.assertStartsWith(actual_cl_contents, new_cl_contents)
+
+    def test_cmd_dailydeb_with_upstream_version_from_changelog(self):
+        source = self.make_simple_package()
+        self.build_tree_contents([("test.recipe", "# bzr-builder format 0.1 "
+                    "deb-version {debupstream}-2\nsource 1\n")])
+        out, err = self.run_bzr("dailydeb test.recipe working")
+        new_cl_contents = ("package (0.1-2) unstable; urgency=low\n\n"
+                "  * Auto build.\n\n -- M. Maintainer <[e-mail]>  ")
+        f = open("working/test-{debupstream}-2/debian/changelog")
+        try:
+            actual_cl_contents = f.read()
+        finally:
+            f.close()
+        self.assertStartsWith(actual_cl_contents, new_cl_contents)

=== modified file 'tests/test_recipe.py'
--- tests/test_recipe.py	2009-08-18 21:25:32 +0000
+++ tests/test_recipe.py	2009-11-13 06:05:22 +0000
@@ -714,7 +714,7 @@
     def test_substitute(self):
         source =self.make_branch_and_tree("source")
         revid1 = source.commit("one")
-        revid2 = source.commit("two")
+        source.commit("two")
         branch1 = BaseRecipeBranch("source",
                 "{revno}-{revno:packaging}", 0.2, revspec="1")
         branch2 = RecipeBranch("packaging", "source")
@@ -725,6 +725,16 @@
         self.assertEqual("1", branch1.revspec)
         self.assertEqual("1-2", branch1.deb_version)
 
+    def test_substitute_supports_debupstream(self):
+        # resolve_revisions should leave debupstream parameters alone and not
+        # complain.
+        source =self.make_branch_and_tree("source")
+        source.commit("one")
+        source.commit("two")
+        branch1 = BaseRecipeBranch("source", "{debupstream}-{revno}", 0.2)
+        resolve_revisions(branch1)
+        self.assertEqual("{debupstream}-2", branch1.deb_version)
+
 
 class BuildManifestTests(TestCaseInTempDir):