Move pre-flight package validation to middleware

This commit separates the scratch and output directory creation from the
main package middleware into its own PackageSetupFolders middleware.
Additionally, the validation that ensures an output file does not exist
is moved into a validation function that can be shared across multiple
methods.

This refactor permits a pre-flight check to ensure box packaging would
be successful before actually stopping the VM.

Fixes GH-7351

lib/vagrant/action/general/package.rb

@@ -20,25 +20,63 @@ module Vagrant
       class Package
         include Util
 
+        # Perform sanity validations that the provided output filepath is sane.
+        # In particular, this function validates:
+        #
+        #   - The output path is a regular file (not a directory or symlink)
+        #   - No file currently exists at the given path
+        #   - A directory of package files was actually provided (internal)
+        #
+        # @param [String] output path to the output file
+        # @param [String] directory path to a directory containing the files
+        def self.validate!(output, directory)
+          filename = File.basename(output.to_s)
+          output   = fullpath(output)
+
+          if File.directory?(output)
+            raise Vagrant::Errors::PackageOutputDirectory
+          end
+
+          if File.exist?(output)
+            raise Vagrant::Errors::PackageOutputExists, filename: filename
+          end
+
+          if !Vagrant::Util::Presence.present?(directory) || !File.directory?(directory)
+            raise Vagrant::Errors::PackageRequiresDirectory
+          end
+        end
+
+        # Calculate the full path of the given path, relative to the current
+        # working directory (where the command was run).
+        #
+        # @param [String] output the relative path
+        def self.fullpath(output)
+          File.expand_path(output, Dir.pwd)
+        end
+
+        # The path to the final output file.
+        # @return [String]
+        attr_reader :fullpath
+
         def initialize(app, env)
           @app = app
 
           env["package.files"]  ||= {}
           env["package.output"] ||= "package.box"
+
+          @fullpath = self.class.fullpath(env["package.output"])
         end
 
         def call(env)
           @env = env
-          file_name = File.basename(@env["package.output"].to_s)
 
-          raise Errors::PackageOutputDirectory if File.directory?(tar_path)
+          self.class.validate!(env["package.output"], env["package.directory"])
-          raise Errors::PackageOutputExists, file_name:file_name if File.exist?(tar_path)
+
-          raise Errors::PackageRequiresDirectory if !env["package.directory"] ||
+          raise Errors::PackageOutputDirectory if File.directory?(fullpath)
-            !File.directory?(env["package.directory"])
 
           @app.call(env)
 
-          @env[:ui].info I18n.t("vagrant.actions.general.package.compressing", tar_path: tar_path)
+          @env[:ui].info I18n.t("vagrant.actions.general.package.compressing", fullpath: fullpath)
           copy_include_files
           setup_private_key
           compress
@@ -54,7 +92,7 @@ module Vagrant
           end
 
           # Cleanup any packaged files if the packaging failed at some point.
-          File.delete(tar_path) if File.exist?(tar_path)
+          File.delete(fullpath) if File.exist?(fullpath)
         end
 
         # This method copies the include files (passed in via command line)
@@ -88,7 +126,7 @@ module Vagrant
         def compress
           # Get the output path. We have to do this up here so that the
           # pwd returns the proper thing.
-          output_path = tar_path.to_s
+          output_path = fullpath.to_s
 
           # Switch into that directory and package everything up
           Util::SafeChdir.safe_chdir(@env["package.directory"]) do
@@ -147,11 +185,6 @@ module Vagrant
             f.puts %Q[end]
           end
         end
-
-        # Path to the final box output file
-        def tar_path
-          File.expand_path(@env["package.output"], FileUtils.pwd)
-        end
       end
     end
   end

plugins/providers/virtualbox/action.rb

@@ -33,6 +33,8 @@ module VagrantPlugins
       autoload :Network, File.expand_path("../action/network", __FILE__)
       autoload :NetworkFixIPv6, File.expand_path("../action/network_fix_ipv6", __FILE__)
       autoload :Package, File.expand_path("../action/package", __FILE__)
+      autoload :PackageSetupFiles, File.expand_path("../action/package_setup_files", __FILE__)
+      autoload :PackageSetupFolders, File.expand_path("../action/package_setup_folders", __FILE__)
       autoload :PackageVagrantfile, File.expand_path("../action/package_vagrantfile", __FILE__)
       autoload :PrepareCloneSnapshot, File.expand_path("../action/prepare_clone_snapshot", __FILE__)
       autoload :PrepareNFSSettings, File.expand_path("../action/prepare_nfs_settings", __FILE__)
@@ -41,12 +43,14 @@ module VagrantPlugins
       autoload :Resume, File.expand_path("../action/resume", __FILE__)
       autoload :SaneDefaults, File.expand_path("../action/sane_defaults", __FILE__)
       autoload :SetName, File.expand_path("../action/set_name", __FILE__)
-      autoload :SetupPackageFiles, File.expand_path("../action/setup_package_files", __FILE__)
       autoload :SnapshotDelete, File.expand_path("../action/snapshot_delete", __FILE__)
       autoload :SnapshotRestore, File.expand_path("../action/snapshot_restore", __FILE__)
       autoload :SnapshotSave, File.expand_path("../action/snapshot_save", __FILE__)
       autoload :Suspend, File.expand_path("../action/suspend", __FILE__)
 
+      # @deprecated use {PackageSetupFiles} instead
+      autoload :SetupPackageFiles, File.expand_path("../action/setup_package_files", __FILE__)
+
       # Include the built-in modules so that we can use them as top-level
       # things.
       include Vagrant::Action::Builtin
@@ -150,7 +154,8 @@ module VagrantPlugins
               next
             end
 
-            b2.use SetupPackageFiles
+            b2.use PackageSetupFolders
+            b2.use PackageSetupFiles
             b2.use CheckAccessible
             b2.use action_halt
             b2.use ClearForwardedPorts

plugins/providers/virtualbox/action/package.rb

@@ -1,6 +1,4 @@
-require 'fileutils'
+require_relative "../../../../lib/vagrant/action/general/package"
-
-require 'vagrant/action/general/package'
 
 module VagrantPlugins
   module ProviderVirtualBox
@@ -10,33 +8,7 @@ module VagrantPlugins
         # called in the unit tests.
         alias_method :general_call, :call
         def call(env)
-          # Setup the temporary directory
-          @temp_dir = env[:tmp_path].join(Time.now.to_i.to_s)
-          env["export.temp_dir"] = @temp_dir
-          FileUtils.mkpath(env["export.temp_dir"])
-
-          # Just match up a couple environmental variables so that
-          # the superclass will do the right thing. Then, call the
-          # superclass
-          env["package.directory"] = env["export.temp_dir"]
-
           general_call(env)
-
-          # Always call recover to clean up the temp dir
-          clean_temp_dir
-        end
-
-        def recover(env)
-          clean_temp_dir
-          super
-        end
-
-        protected
-
-        def clean_temp_dir
-          if @temp_dir && File.exist?(@temp_dir)
-            FileUtils.rm_rf(@temp_dir)
-          end
         end
       end
     end

plugins/providers/virtualbox/action/package_setup_files.rb

@@ -0,0 +1,51 @@
+module VagrantPlugins
+  module ProviderVirtualBox
+    module Action
+      class PackageSetupFiles
+        def initialize(app, env)
+          @app = app
+
+          env["package.include"] ||= []
+          env["package.vagrantfile"] ||= nil
+        end
+
+        def call(env)
+          files = {}
+          env["package.include"].each do |file|
+            source = Pathname.new(file)
+            dest   = nil
+
+            # If the source is relative then we add the file as-is to the include
+            # directory. Otherwise, we copy only the file into the root of the
+            # include directory. Kind of strange, but seems to match what people
+            # expect based on history.
+            if source.relative?
+              dest = source
+            else
+              dest = source.basename
+            end
+
+            # Assign the mapping
+            files[file] = dest
+          end
+
+          if env["package.vagrantfile"]
+            # Vagrantfiles are treated special and mapped to a specific file
+            files[env["package.vagrantfile"]] = "_Vagrantfile"
+          end
+
+          # Verify the mapping
+          files.each do |from, _|
+            raise Vagrant::Errors::PackageIncludeMissing,
+              file: from if !File.exist?(from)
+          end
+
+          # Save the mapping
+          env["package.files"] = files
+
+          @app.call(env)
+        end
+      end
+    end
+  end
+end

plugins/providers/virtualbox/action/package_setup_folders.rb

@@ -0,0 +1,38 @@
+require "fileutils"
+
+require_relative "../../../../lib/vagrant/action/general/package"
+
+module VagrantPlugins
+  module ProviderVirtualBox
+    module Action
+      class PackageSetupFolders
+        include Vagrant::Util::Presence
+
+        def initialize(app, env)
+          @app = app
+        end
+
+        def call(env)
+          env["package.output"] ||= "package.box"
+          env["package.directory"] ||= Dir.mktmpdir("package-", env[:tmp_path])
+
+          # Match up a couple environmental variables so that the other parts of
+          # Vagrant will do the right thing.
+          env["export.temp_dir"] = env["package.directory"]
+
+          Vagrant::Action::General::Package.validate!(
+            env["package.output"], env["package.directory"])
+
+          @app.call(env)
+        end
+
+        def recover(env)
+          dir = env["package.directory"]
+          if File.exist?(dir)
+            FileUtils.rm_rf(dir)
+          end
+        end
+      end
+    end
+  end
+end

plugins/providers/virtualbox/action/setup_package_files.rb

@@ -1,49 +1,15 @@
+require "log4r"
+
+require_relative "package_setup_files"
+
 module VagrantPlugins
   module ProviderVirtualBox
     module Action
-      class SetupPackageFiles
+      class SetupPackageFiles < PackageSetupFiles
-        def initialize(app, env)
+        def initialize(*)
-          @app = app
+          @logger = Log4r::Logger.new("vagrant::plugins::virtualbox::setup_package_files")
-
+          @logger.warn { "SetupPackageFiles has been renamed to PackageSetupFiles" }
-          env["package.include"] ||= []
+          super
-          env["package.vagrantfile"] ||= nil
-        end
-
-        def call(env)
-          files = {}
-          env["package.include"].each do |file|
-            source = Pathname.new(file)
-            dest   = nil
-
-            # If the source is relative then we add the file as-is to the include
-            # directory. Otherwise, we copy only the file into the root of the
-            # include directory. Kind of strange, but seems to match what people
-            # expect based on history.
-            if source.relative?
-              dest = source
-            else
-              dest = source.basename
-            end
-
-            # Assign the mapping
-            files[file] = dest
-          end
-
-          if env["package.vagrantfile"]
-            # Vagrantfiles are treated special and mapped to a specific file
-            files[env["package.vagrantfile"]] = "_Vagrantfile"
-          end
-
-          # Verify the mapping
-          files.each do |from, _|
-            raise Vagrant::Errors::PackageIncludeMissing,
-              file: from if !File.exist?(from)
-          end
-
-          # Save the mapping
-          env["package.files"] = files
-
-          @app.call(env)
         end
       end
     end

templates/locales/en.yml

@@ -1734,10 +1734,10 @@ en:
             Sometimes, Vagrant will attempt to auto-correct this for you. In this
             case, Vagrant was unable to. This is usually because the guest machine
             is in a state which doesn't allow modifying port forwarding. You could
-            try 'vagrant reload' (equivalent of running a halt followed by an up) 
+            try 'vagrant reload' (equivalent of running a halt followed by an up)
-            so vagrant can attempt to auto-correct this upon booting. Be warned 
+            so vagrant can attempt to auto-correct this upon booting. Be warned
             that any unsaved work might be lost.
-            
+
           fixed_collision: |-
             Fixed port collision for %{guest_port} => %{host_port}. Now on port %{new_port}.
           forwarding: Forwarding ports...
@@ -1883,16 +1883,17 @@ en:
       general:
         package:
           packaging: "Packaging additional file: %{file}"
-          compressing: "Compressing package to: %{tar_path}"
+          compressing: "Compressing package to: %{fullpath}"
           output_exists: |-
-            The specified file '%{file_name}' to save the package as already exists. Please
+            The specified file '%{filename}' to save the package as already exists. Please
             remove this file or specify a different file name for outputting.
           output_is_directory: |-
             The specified output is a directory. Please specify a path including
             a filename.
           requires_directory: |-
             A directory was not specified to package. This should never happen
-            and is a result of an internal inconsistency.
+            and is a result of an internal inconsistency. Please report a bug
+            on the Vagrant bug tracker.
           include_file_missing: |-
             Package include file doesn't exist: %{file}