BoxCollection#add transparently upgrades V1 boxes to V2

This means that V1 boxes can be added to a V2 box collection without
issue.

lib/vagrant/box_collection.rb

@@ -63,18 +63,25 @@ module Vagrant
       # Create a temporary directory since we're not sure at this point if
       # the box we're unpackaging already exists (if no provider was given)
       Dir.mktmpdir("vagrant-") do |temp_dir|
+        temp_dir = Pathname.new(temp_dir)
+
         # Extract the box into a temporary directory.
         @logger.debug("Unpacking box into temporary directory: #{temp_dir}")
-
         begin
-          Archive::Tar::Minitar.unpack(path.to_s, temp_dir)
+          Archive::Tar::Minitar.unpack(path.to_s, temp_dir.to_s)
         rescue SystemCallError
           raise Errors::BoxUnpackageFailure
         end
 
+        # If we get a V1 box, we want to update it in place
+        if v1_box?(temp_dir)
+          @logger.debug("Added box is a V1 box. Upgrading in place.")
+          temp_dir = v1_upgrade(temp_dir)
+        end
+
         # Get an instance of the box we just added before it is finalized
         # in the system so we can inspect and use its metadata.
-        box = Box.new(name, provider, Pathname.new(temp_dir))
+        box = Box.new(name, provider, temp_dir)
 
         # Get the provider, since we'll need that to at the least add it
         # to the system or check that it matches what is given to us.
@@ -174,7 +181,7 @@ module Vagrant
       # To determine if it is a V1 box we just do a simple heuristic
       # based approach.
       @logger.info("Searching for V1 box: #{name}")
-      if v1_box?(name)
+      if v1_box?(@directory.join(name))
         @logger.warn("V1 box found: #{name}")
         raise Errors::BoxUpgradeRequired, :name => name
       end
@@ -197,20 +204,52 @@ module Vagrant
       # If the box doesn't exist at all, raise an exception
       raise Errors::BoxNotFound, :name => name if !box_dir.directory?
 
-      if v1_box?(name)
+      if v1_box?(box_dir)
         @logger.debug("V1 box #{name} found. Upgrading!")
 
-        # First, we create a temporary directory within the box to store
+        # First we actually perform the upgrade
-        # the intermediary moved files. We randomize this in case there is
+        temp_dir = v1_upgrade(box_dir)
-        # already a directory named "virtualbox" in here for some reason.
+
-        temp_dir = box_dir.join("vagrant-#{Digest::SHA1.hexdigest(name)}")
+        # Rename the temporary directory to the provider.
+        temp_dir.rename(box_dir.join("virtualbox"))
+        @logger.info("Box '#{name}' upgraded from V1 to V2.")
+      end
+
+      # We did it! Or the v1 box didn't exist so it doesn't matter.
+      return true
+    end
+
+    protected
+
+    # This checks if the given directory represents a V1 box on the
+    # system.
+    #
+    # @param [Pathname] dir Directory where the box is unpacked.
+    # @return [Boolean]
+    def v1_box?(dir)
+      # We detect a V1 box given by whether there is a "box.ovf" which
+      # is a heuristic but is pretty accurate.
+      dir.join("box.ovf").file?
+    end
+
+    # This upgrades the V1 box contained unpacked in the given directory
+    # and returns the directory of the upgraded version. This is
+    # _destructive_ to the contents of the old directory. That is, the
+    # contents of the old V1 box will be destroyed or moved.
+    #
+    # Preconditions:
+    # * `dir` is a valid V1 box. Verify with {#v1_box?}
+    #
+    # @param [Pathname] dir Directory where the V1 box is unpacked.
+    # @return [Pathname] Path to the unpackaged V2 box.
+    def v1_upgrade(dir)
+      @logger.debug("Upgrading box in directory: #{dir}")
+
+      temp_dir = Pathname.new(Dir.mktmpdir("vagrant-"))
       @logger.debug("Temporary directory for upgrading: #{temp_dir}")
 
-        # Make the temporary directory
-        temp_dir.mkpath
-
       # Move all the things into the temporary directory
-        box_dir.children(true).each do |child|
+      dir.children(true).each do |child|
         # Don't move the temp_dir
         next if child == temp_dir
 
@@ -224,28 +263,14 @@ module Vagrant
       metadata_file = temp_dir.join("metadata.json")
       if !metadata_file.file?
         metadata_file.open("w") do |f|
-            f.write(JSON.generate({}))
+          f.write(JSON.generate({
+            :provider => "virtualbox"
+          }))
         end
       end
 
-        # Rename the temporary directory to the provider.
+      # Return the temporary directory
-        temp_dir.rename(box_dir.join("virtualbox"))
+      temp_dir
-        @logger.info("Box '#{name}' upgraded from V1 to V2.")
-      end
-
-      # We did it! Or the v1 box didn't exist so it doesn't matter.
-      return true
-    end
-
-    protected
-
-    # This checks if the given name represents a V1 box on the system.
-    #
-    # @return [Boolean]
-    def v1_box?(name)
-      # We detect a V1 box given by whether there is a "box.ovf" which
-      # is a heuristic but is pretty accurate.
-      @directory.join(name, "box.ovf").file?
     end
   end
 end

test/unit/support/isolated_environment.rb

@@ -86,6 +86,52 @@ module Unit
       box_dir
     end
 
+    # This creates a "box" file that is a valid V1 box.
+    #
+    # @return [Pathname] Path to the newly created box.
+    def box1_file
+      # Create a temporary directory to store our data we will tar up
+      td_source = Tempdir.new
+      td_dest   = Tempdir.new
+
+      # Store the temporary directory so it is not deleted until
+      # this instance is garbage collected.
+      @_box2_file_temp ||= []
+      @_box2_file_temp << td_dest
+
+      # The source as a Pathname, which is easier to work with
+      source = Pathname.new(td_source.path)
+
+      # The destination file
+      result = Pathname.new(td_dest.path).join("temporary.box")
+
+      File.open(result, Vagrant::Util::Platform.tar_file_options) do |tar|
+        Archive::Tar::Minitar::Output.open(tar) do |output|
+          begin
+            # Switch to the source directory so that Archive::Tar::Minitar
+            # can tar things up.
+            current_dir = FileUtils.pwd
+            FileUtils.cd(source)
+
+            # Put a "box.ovf" in there.
+            source.join("box.ovf").open("w") do |f|
+              f.write("FOO!")
+            end
+
+            # Add all the files
+            Dir.glob(File.join(".", "**", "*")).each do |entry|
+              Archive::Tar::Minitar.pack_file(entry, output)
+            end
+          ensure
+            FileUtils.cd(current_dir)
+          end
+        end
+      end
+
+      # Resulting box
+      result
+    end
+
     # This creates a "box" file with the given provider.
     #
     # @param [Symbol] provider Provider for the box.

test/unit/vagrant/box_collection_test.rb

@@ -38,6 +38,17 @@ describe Vagrant::BoxCollection do
       box.provider.should == :vmware
     end
 
+    it "should add a V1 box" do
+      # Create a V1 box.
+      box_path = environment.box1_file
+
+      # Add the box
+      box = instance.add(box_path, "foo")
+      box.should be_kind_of(box_class)
+      box.name.should == "foo"
+      box.provider.should == :virtualbox
+    end
+
     it "should raise an exception if the box already exists" do
       prev_box_name = "foo"
       prev_box_provider = :virtualbox