Improve Java Opts Coherence

Previously, the way that JAVA_OPTS were handled was a bit inconsistent. First,
JAVA_OPTS from the environment could be used to affect the memory calculator,
but JAVA_OPTS from configuration could not due to the fact that they were
assembled on the command line after the calculator had run.  In addition, any
changes to the environment's JAVA_OPTS would require a restage as those values
were written into the command line instead of just being passed through the
command stream.

This change attempts to improve the consistency of how JAVA_OPTS are handled
by doing two things.  First, the environment's JAVA_OPTS are never processed
as part of staging.  Instead, a $JAVA_OPTS is appended (or not depending on
configuration) to the JAVA_OPTS contributed to the buildpack.  This solves the
restage issue.  Second, it declares the JAVA_OPTS as the very first part of
the execution command, before the memory calculator.  This solves the issue of
ensuring that both environment and configured JAVA_OPTS are available to the
memory calculator at startup.

[#423][#424]

Author: nebhale

lib/java_buildpack/buildpack.rb

@@ -81,6 +81,8 @@ def release
       component_detection('framework', @frameworks, false).map(&:release)
 
       commands << container.release
+
+      commands.insert 0, @java_opts.as_env_var
       command = commands.flatten.compact.join(' && ')
 
       payload = {
@@ -111,6 +113,8 @@ def initialize(app_dir, application)
       log_environment_variables
       log_application_contents application
 
+      @java_opts = Component::JavaOpts.new(app_dir)
+
       mutable_java_home   = Component::MutableJavaHome.new
       immutable_java_home = Component::ImmutableJavaHome.new mutable_java_home, app_dir
 
@@ -120,7 +124,7 @@ def initialize(app_dir, application)
         'application'           => application,
         'env_vars'              => Component::EnvironmentVariables.new(app_dir),
         'extension_directories' => Component::ExtensionDirectories.new(app_dir),
-        'java_opts'             => Component::JavaOpts.new(app_dir),
+        'java_opts'             => @java_opts,
         'security_providers'    => Component::SecurityProviders.new
       }
 

lib/java_buildpack/container/dist_zip_like.rb

@@ -42,7 +42,6 @@ def release
         [
           @droplet.environment_variables.as_env_vars,
           @droplet.java_home.as_env_var,
-          @droplet.java_opts.as_env_var,
           'exec',
           qualify_path(start_script(root), @droplet.root)
         ].flatten.compact.join(' ')

lib/java_buildpack/container/groovy.rb

@@ -54,7 +54,6 @@ def release
         [
           @droplet.environment_variables.as_env_vars,
           @droplet.java_home.as_env_var,
-          @droplet.java_opts.as_env_var,
           'exec',
           qualify_path(@droplet.sandbox + 'bin/groovy', @droplet.root),
           @droplet.additional_libraries.as_classpath,

lib/java_buildpack/container/java_main.rb

@@ -72,8 +72,6 @@ def release
 
       def release_text(classpath)
         [
-          @droplet.java_opts.as_env_var,
-          '&&',
           @droplet.environment_variables.as_env_vars,
           'eval',
           'exec',

lib/java_buildpack/container/spring_boot_cli.rb

@@ -49,7 +49,6 @@ def release
         [
           @droplet.environment_variables.as_env_vars,
           @droplet.java_home.as_env_var,
-          @droplet.java_opts.as_env_var,
           'exec',
           qualify_path(@droplet.sandbox + 'bin/spring', @droplet.root),
           'run',

lib/java_buildpack/container/tomcat.rb

@@ -39,7 +39,6 @@ def command
         [
           @droplet.environment_variables.as_env_vars,
           @droplet.java_home.as_env_var,
-          @droplet.java_opts.as_env_var,
           'exec',
           "$PWD/#{(@droplet.sandbox + 'bin/catalina.sh').relative_path_from(@droplet.root)}",
           'run'

lib/java_buildpack/framework/java_opts.rb

@@ -26,15 +26,22 @@ class JavaOpts < JavaBuildpack::Component::BaseComponent
 
       # (see JavaBuildpack::Component::BaseComponent#detect)
       def detect
-        supports_configuration? || supports_environment? ? JavaOpts.to_s.dash_case : nil
+        JavaOpts.to_s.dash_case
       end
 
       # (see JavaBuildpack::Component::BaseComponent#compile)
       def compile; end
 
       # (see JavaBuildpack::Component::BaseComponent#release)
       def release
-        @droplet.java_opts.concat parsed_java_opts
+        configured
+          .shellsplit
+          .map { |java_opt| /(?<key>.+?)=(?<value>.+)/ =~ java_opt ? "#{key}=#{escape_value(value)}" : java_opt }
+          .each { |java_opt| @droplet.java_opts << java_opt }
+
+        @droplet.java_opts << '$JAVA_OPTS' if from_environment?
+
+        @droplet.java_opts.as_env_var
       end
 
       private
@@ -43,39 +50,22 @@ def release
 
       ENVIRONMENT_PROPERTY = 'from_environment'.freeze
 
-      ENVIRONMENT_VARIABLE = 'JAVA_OPTS'.freeze
-
-      private_constant :CONFIGURATION_PROPERTY, :ENVIRONMENT_PROPERTY, :ENVIRONMENT_VARIABLE
-
-      def parsed_java_opts
-        parsed_java_opts = []
+      private_constant :CONFIGURATION_PROPERTY, :ENVIRONMENT_PROPERTY
 
-        parsed_java_opts.concat @configuration[CONFIGURATION_PROPERTY].shellsplit if supports_configuration?
-        parsed_java_opts.concat ENV[ENVIRONMENT_VARIABLE].shellsplit if supports_environment?
-
-        parsed_java_opts.map do |java_opt|
-          if /(?<key>.+?)=(?<value>.+)/ =~ java_opt
-            "#{key}=#{parse_shell_string(value)}"
-          else
-            java_opt
-          end
-        end
+      def configured
+        @configuration[CONFIGURATION_PROPERTY] || ''
       end
 
-      def parse_shell_string(str)
+      def escape_value(str)
         return "''" if str.empty?
-        str = str.dup
-        str.gsub!(%r{([^A-Za-z0-9_\-.,:\/@\n$\\])}, '\\\\\\1')
-        str.gsub!(/\n/, "'\n'")
-        str
-      end
 
-      def supports_configuration?
-        @configuration.key?(CONFIGURATION_PROPERTY) && !@configuration[CONFIGURATION_PROPERTY].nil?
+        str
+          .gsub(%r{([^A-Za-z0-9_\-.,:\/@\n$\\])}, '\\\\\\1')
+          .gsub(/\n/, "'\n'")
       end
 
-      def supports_environment?
-        @configuration[ENVIRONMENT_PROPERTY] && ENV.key?(ENVIRONMENT_VARIABLE)
+      def from_environment?
+        @configuration[ENVIRONMENT_PROPERTY]
       end
 
     end

lib/java_buildpack/jre/open_jdk_like_memory_calculator.rb

@@ -43,8 +43,7 @@ def compile
           memory_calculator.chmod 0o755
 
           puts "       Loaded Classes: #{class_count @configuration}, " \
-               "Threads: #{stack_threads @configuration}, " \
-               "JAVA_OPTS: '#{java_opts}'"
+               "Threads: #{stack_threads @configuration}"
         end
       end
 
@@ -53,13 +52,12 @@ def compile
       # @return [String] the memory calculation command
       def memory_calculation_command
         "CALCULATED_MEMORY=$(#{memory_calculation_string(@droplet.root)}) && " \
-        'echo JVM Memory Configuration: $CALCULATED_MEMORY'
+        'echo JVM Memory Configuration: $CALCULATED_MEMORY && ' \
+        'JAVA_OPTS="$JAVA_OPTS $CALCULATED_MEMORY"'
       end
 
       # (see JavaBuildpack::Component::BaseComponent#release)
-      def release
-        @droplet.java_opts.add_preformatted_options '$CALCULATED_MEMORY'
-      end
+      def release; end
 
       protected
 
@@ -85,10 +83,6 @@ def class_count(configuration)
         configuration['class_count'] || (0.35 * actual_class_count(root)).ceil
       end
 
-      def java_opts
-        ENV['JAVA_OPTS']
-      end
-
       def memory_calculator
         @droplet.sandbox + "bin/java-buildpack-memory-calculator-#{@version}"
       end
@@ -104,7 +98,7 @@ def memory_calculation_string(relative_path)
         memory_calculation_string << "-stackThreads=#{stack_threads @configuration}"
         memory_calculation_string << "-loadedClasses=#{class_count @configuration}"
         memory_calculation_string << "-poolType=#{pool_type}"
-        memory_calculation_string << "-vmOptions='#{java_opts}'" if java_opts
+        memory_calculation_string << '-vmOptions="$JAVA_OPTS"'
 
         memory_calculation_string.join(' ')
       end

lib/java_buildpack/util/play/post22.rb

@@ -39,15 +39,7 @@ def augment_classpath
 
         # (see JavaBuildpack::Util::Play::Base#java_opts)
         def java_opts
-          java_opts = @droplet.java_opts
-
-          java_opts.each do |option|
-            next unless option.shellsplit.length > 1 && !bash_expression?(option)
-
-            raise "Invalid Java option contains more than one option: '#{option}'"
-          end
-
-          java_opts.map { |option| option == '$CALCULATED_MEMORY' ? '${CALCULATED_MEMORY//-/-J-}' : "-J#{option}" }
+          '${JAVA_OPTS//-/-J-}'
         end
 
         # (see JavaBuildpack::Util::Play::Base#lib_dir)

lib/java_buildpack/util/play/pre22.rb

@@ -26,7 +26,7 @@ class Pre22 < Base
 
         # (see JavaBuildpack::Util::Play::Base#java_opts)
         def java_opts
-          @droplet.java_opts
+          '$JAVA_OPTS'
         end
 
         # (see JavaBuildpack::Util::Play::Base#start_script)