Merge pull request #582 from jeffmccune/improve/master/SERVER-297_con…

…solidate_env_handling

(SERVER-297) Consolidate JRuby environment handling

acceptance/suites/tests/010-puppetserver-cli/subcommand/SERVER-297_common_behavior.rb

@@ -0,0 +1,23 @@
+require 'test/unit/assertions'
+require 'json'
+
+test_name "Puppetserver subcommand consolidated ENV handling tests."
+
+step "ruby: Check that PATH, HOME, GEM_HOME JARS_REQUIRE and JARS_NO_REQUIRE are present"
+on(master, "puppetserver ruby -rjson -e 'puts JSON.pretty_generate(ENV.to_hash)'") do
+  env = JSON.parse(stdout)
+  assert(env['PATH'], "PATH missing")
+  assert(env['HOME'], "HOME missing")
+  assert(env['GEM_HOME'], "GEM_HOME missing")
+  assert(env['JARS_REQUIRE'], "JARS_REQUIRE missing")
+  assert(env['JARS_NO_REQUIRE'], "JARS_NO_REQUIRE missing")
+end
+
+step "irb: Check that PATH, HOME, GEM_HOME JARS_REQUIRE and JARS_NO_REQUIRE are present"
+on(master, "puppetserver irb -f -rjson -e 'puts JSON.pretty_generate(ENV.to_hash)'") do
+  assert_match(/\bPATH\b/, output, "PATH missing")
+  assert_match(/\bHOME\b/, output, "HOME missing")
+  assert_match(/\bGEM_HOME\b/, output, "GEM_HOME missing")
+  assert_match(/\bJARS_REQUIRE\b/, output, "JARS_REQUIRE missing")
+  assert_match(/\bJARS_NO_REQUIRE\b/, output, "JARS_NO_REQUIRE missing")
+end

acceptance/suites/tests/010-puppetserver-cli/subcommand/gem.rb

@@ -66,3 +66,6 @@
 initial_installed_gems.each do |gem_info|
   assert_send([final_installed_gems, :include?, gem_info])
 end
+
+step "Verify that gem env operates"
+on(master, "#{cli} gem env", :acceptable_exit_codes => [0])

src/clj/puppetlabs/puppetserver/cli/gem.clj

@@ -1,18 +1,10 @@
 (ns puppetlabs.puppetserver.cli.gem
-  (:import (org.jruby.embed ScriptingContainer))
-  (:require [puppetlabs.puppetserver.cli.subcommand :as cli]))
+  (:require [puppetlabs.puppetserver.cli.subcommand :as cli]
+            [puppetlabs.services.jruby.jruby-puppet-core :as jruby-core]))
 
 (defn run!
   [config args]
-  (doto (ScriptingContainer.)
-    (.setArgv (into-array String args))
-    (.setEnvironment
-      (hash-map
-        "GEM_HOME" (get-in config [:jruby-puppet :gem-home])
-        "JARS_NO_REQUIRE" "true"
-        "JARS_REQUIRE" "false"))
-    (.runScriptlet "require 'jar-dependencies'")
-    (.runScriptlet "load 'META-INF/jruby.home/bin/gem'")))
+  (jruby-core/cli-run! config "gem" args))
 
 (defn -main
   [& args]

src/clj/puppetlabs/puppetserver/cli/irb.clj

@@ -1,39 +1,10 @@
 (ns puppetlabs.puppetserver.cli.irb
-  (:import (org.jruby Main RubyInstanceConfig CompatVersion)
-           (java.util HashMap))
   (:require [puppetlabs.puppetserver.cli.subcommand :as cli]
-            [puppetlabs.services.jruby.jruby-puppet-internal :as jruby-internal]))
-
-(defn new-jruby-main
-  [config]
-  (let [jruby-config (RubyInstanceConfig.)
-        gem-home     (get-in config [:jruby-puppet :gem-home])
-        env          (doto (HashMap. (.getEnvironment jruby-config))
-                       (.put "GEM_HOME" gem-home)
-                       (.put "JARS_NO_REQUIRE" "true")
-                       (.put "JARS_REQUIRE" "false"))
-        jruby-home   (.getJRubyHome jruby-config)
-        load-path    (->> (get-in config [:jruby-puppet :ruby-load-path])
-                       (cons jruby-internal/ruby-code-dir)
-                       (cons (str jruby-home "/lib/ruby/1.9"))
-                       (cons (str jruby-home "/lib/ruby/shared"))
-                       (cons (str jruby-home "/lib/ruby/1.9/site_ruby")))]
-    (doto jruby-config
-      (.setEnvironment env)
-      (.setLoadPaths load-path)
-      (.setCompatVersion (CompatVersion/RUBY1_9)))
-    (Main. jruby-config)))
+            [puppetlabs.services.jruby.jruby-puppet-core :as jruby-core]))
 
 (defn run!
-  [config ruby-args]
-  (doto (new-jruby-main config)
-    (.run (into-array String
-            (concat
-              ["-rjar-dependencies"
-               "-e"
-               "load 'META-INF/jruby.home/bin/irb'"
-               "--"]
-              ruby-args)))))
+  [config args]
+  (jruby-core/cli-run! config "irb" args))
 
 (defn -main
   [& args]

src/clj/puppetlabs/puppetserver/cli/ruby.clj

@@ -1,35 +1,7 @@
 (ns puppetlabs.puppetserver.cli.ruby
-  (:import (org.jruby Main RubyInstanceConfig CompatVersion)
-           (java.util HashMap))
   (:require [puppetlabs.puppetserver.cli.subcommand :as cli]
-            [puppetlabs.services.jruby.jruby-puppet-internal :as jruby-internal]))
-
-(defn new-jruby-main
-  [config]
-  (let [jruby-config (RubyInstanceConfig.)
-        gem-home     (get-in config [:jruby-puppet :gem-home])
-        env          (doto (HashMap. (.getEnvironment jruby-config))
-                       (.put "GEM_HOME" gem-home)
-                       (.put "JARS_NO_REQUIRE" "true")
-                       (.put "JARS_REQUIRE" "false"))
-        jruby-home   (.getJRubyHome jruby-config)
-        load-path    (->> (get-in config [:jruby-puppet :ruby-load-path])
-                       (cons jruby-internal/ruby-code-dir)
-                       (cons (str jruby-home "/lib/ruby/1.9"))
-                       (cons (str jruby-home "/lib/ruby/shared"))
-                       (cons (str jruby-home "/lib/ruby/1.9/site_ruby")))]
-    (doto jruby-config
-      (.setEnvironment env)
-      (.setLoadPaths load-path)
-      (.setCompatVersion (CompatVersion/RUBY1_9)))
-    (Main. jruby-config)))
-
-(defn run!
-  [config ruby-args]
-  (doto (new-jruby-main config)
-    (.run (into-array String
-            (cons "-rjar-dependencies" ruby-args)))))
+            [puppetlabs.services.jruby.jruby-puppet-core :as jruby-core]))
 
 (defn -main
   [& args]
-  (cli/run run! args))
+  (cli/run jruby-core/cli-ruby! args))

src/clj/puppetlabs/services/jruby/jruby_puppet_core.clj

@@ -4,7 +4,8 @@
             [puppetlabs.services.jruby.jruby-puppet-schemas :as jruby-schemas]
             [puppetlabs.services.jruby.puppet-environments :as puppet-env]
             [puppetlabs.services.jruby.jruby-puppet-internal :as jruby-internal]
-            [puppetlabs.services.jruby.jruby-puppet-agents :as jruby-agents])
+            [puppetlabs.services.jruby.jruby-puppet-agents :as jruby-agents]
+            [clojure.java.io :as io])
   (:import (puppetlabs.services.jruby.jruby_puppet_schemas JRubyPuppetInstance)
            (com.puppetlabs.puppetserver PuppetProfiler)))
 
@@ -41,21 +42,6 @@
 (def default-master-var-dir
   "/opt/puppetlabs/server/data/puppetserver")
 
-;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;
-;;; Definitions
-
-(def jruby-puppet-env
-  "The environment variables that should be passed to the Puppet JRuby interpreters.
-  We don't want them to read any ruby environment variables, like $GEM_HOME or
-  $RUBY_LIB or anything like that, so pass it an empty environment map - except -
-  Puppet needs HOME and PATH for facter resolution, so leave those."
-  (select-keys (System/getenv) ["HOME" "PATH"]))
-
-(def ruby-code-dir
-  "The name of the directory containing the ruby code in this project.
-  This directory lives under src/ruby/"
-  "puppet-server-lib")
-
 ;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;
 ;;; Private
 
@@ -190,3 +176,20 @@
   "Return a borrowed pool instance to its free pool."
   [instance :- jruby-schemas/JRubyPuppetInstanceOrRetry]
   (jruby-internal/return-to-pool instance))
+
+(schema/defn ^:always-validate cli-ruby! :- jruby-schemas/JRubyMainStatus
+  "Run JRuby as though native `ruby` were invoked with args on the CLI"
+  [config :- {schema/Keyword schema/Any}
+   args :- [schema/Str]]
+  (let [main (jruby-internal/new-main (initialize-config config))
+        argv (into-array String (concat ["-rjar-dependencies"] args))]
+    (.run main argv)))
+
+(schema/defn ^:always-validate cli-run! :- (schema/maybe jruby-schemas/JRubyMainStatus)
+  "Run a JRuby CLI command, e.g. gem, irb, etc..."
+  [config :- {schema/Keyword schema/Any}
+   command :- schema/Str
+   args :- [schema/Str]]
+  (let [load-path (format "META-INF/jruby.home/bin/%s" command)]
+    (if-let [url (io/resource load-path (.getClassLoader org.jruby.Main))]
+      (cli-ruby! config (concat ["-e" (format "load '%s'" url) "--"] args)))))

src/clj/puppetlabs/services/jruby/jruby_puppet_internal.clj

@@ -7,61 +7,92 @@
   (:import (com.puppetlabs.puppetserver PuppetProfiler JRubyPuppet)
            (puppetlabs.services.jruby.jruby_puppet_schemas JRubyPuppetInstance PoisonPill)
            (java.util HashMap)
-           (org.jruby CompatVersion RubyInstanceConfig$CompileMode)
+           (org.jruby CompatVersion Main RubyInstanceConfig RubyInstanceConfig$CompileMode)
            (org.jruby.embed ScriptingContainer LocalContextScope)
            (java.util.concurrent LinkedBlockingDeque TimeUnit)
            (clojure.lang IFn)))
 
 ;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;
 ;;; Definitions
 
-(def jruby-puppet-env
-  "The environment variables that should be passed to the Puppet JRuby interpreters.
-  We don't want them to read any ruby environment variables, like $GEM_HOME or
-  $RUBY_LIB or anything like that, so pass it an empty environment map - except -
-  Puppet needs HOME and PATH for facter resolution, so leave those."
-  (select-keys (System/getenv) ["HOME" "PATH"]))
-
 (def ruby-code-dir
   "The name of the directory containing the ruby code in this project.
-  This directory lives under src/ruby/"
+
+  This directory is relative to `src/ruby` and works from source because the
+  `src/ruby` directory is defined as a resource in `project.clj` which places
+  the directory on the classpath which in turn makes the directory available on
+  the JRuby load path.  Similarly, this works from the uberjar because this
+  directory is placed into the root of the jar structure which is on the
+  classpath.
+
+  See also:  http://jruby.org/apidocs/org/jruby/runtime/load/LoadService.html"
   "puppet-server-lib")
 
+(def compile-mode
+  "The JRuby compile mode to use for all ruby components, e.g. the master
+  service and CLI tools."
+  RubyInstanceConfig$CompileMode/OFF)
+
+(def compat-version
+  "The JRuby compatibility version to use for all ruby components, e.g. the
+  master service and CLI tools."
+  (CompatVersion/RUBY1_9))
+
 ;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;
 ;;; Private
 
+(schema/defn get-system-env :- jruby-schemas/EnvPersistentMap
+  "Same as System/getenv, but returns a clojure persistent map instead of a
+  Java unmodifiable map."
+  []
+  (into {} (System/getenv)))
+
 (defn instantiate-free-pool
   "Instantiate a new queue object to use as the pool of free JRubyPuppet's."
   [size]
   {:post [(instance? jruby-schemas/pool-queue-type %)]}
   (LinkedBlockingDeque. size))
 
-(schema/defn ^:always-validate
-  create-pool-from-config :- jruby-schemas/PoolState
-  "Create a new PoolData based on the config input."
-  [{size :max-active-instances} :- jruby-schemas/JRubyPuppetConfig]
-  {:pool (instantiate-free-pool size)
-   :size size})
+(schema/defn ^:always-validate managed-environment :- jruby-schemas/EnvMap
+  "The environment variables that should be passed to the Puppet JRuby
+  interpreters.
+
+  We don't want them to read any ruby environment variables, like $RUBY_LIB or
+  anything like that, so pass it an empty environment map - except - Puppet
+  needs HOME and PATH for facter resolution, so leave those, along with GEM_HOME
+  which is necessary for third party extensions that depend on gems.
+
+  We need to set the JARS..REQUIRE variables in order to instruct JRuby's
+  'jar-dependencies' to not try to load any dependent jars.  This is being
+  done specifically to avoid JRuby trying to load its own version of Bouncy
+  Castle, which may not the same as the one that 'puppetlabs/ssl-utils'
+  uses. JARS_NO_REQUIRE was the legacy way to turn off jar loading but is
+  being phased out in favor of JARS_REQUIRE.  As of JRuby 1.7.20, only
+  JARS_NO_REQUIRE is honored.  Setting both of those here for forward
+  compatibility."
+  [env :- jruby-schemas/EnvMap
+   gem-home :- schema/Str]
+  (let [whitelist ["HOME" "PATH"]
+        clean-env (select-keys env whitelist)]
+    (assoc clean-env
+      "GEM_HOME" gem-home
+      "JARS_NO_REQUIRE" "true"
+      "JARS_REQUIRE" "false")))
+
+(schema/defn ^:always-validate managed-load-path :- [schema/Str]
+  "Return a list of ruby LOAD_PATH directories built from the
+  user-configurable ruby-load-path setting of the jruby-puppet configuration."
+  [ruby-load-path :- [schema/Str]]
+  (cons ruby-code-dir ruby-load-path))
 
 (defn prep-scripting-container
   [scripting-container ruby-load-path gem-home]
+  ; Note, this behavior should remain consistent with new-main
   (doto scripting-container
-    (.setLoadPaths (cons ruby-code-dir
-                         (map fs/absolute-path ruby-load-path)))
-    (.setCompatVersion (CompatVersion/RUBY1_9))
-    (.setCompileMode RubyInstanceConfig$CompileMode/OFF)
-    ;; We need to set the JARS..REQUIRE variables in order to instruct
-    ;; JRuby's 'jar-dependencies' to not try to load any dependent jars.  This
-    ;; is being done specifically to avoid JRuby trying to load its own version
-    ;; of Bouncy Castle, which may not the same as the one that
-    ;; 'puppetlabs/ssl-utils' uses. JARS_NO_REQUIRE was the legacy way to turn
-    ;; off jar loading but is being phased out in favor of JARS_REQUIRE.  As of
-    ;; JRuby 1.7.20, only JARS_NO_REQUIRE is honored.  Setting both of those
-    ;; here for forward compatibility.
-    (.setEnvironment (merge {"GEM_HOME" gem-home
-                             "JARS_NO_REQUIRE" "true"
-                             "JARS_REQUIRE" "false"}
-                            jruby-puppet-env))))
+    (.setLoadPaths (managed-load-path ruby-load-path))
+    (.setCompatVersion compat-version)
+    (.setCompileMode compile-mode)
+    (.setEnvironment (managed-environment (get-system-env) gem-home))))
 
 (defn empty-scripting-container
   "Creates a clean instance of `org.jruby.embed.ScriptingContainer` with no code loaded."
@@ -71,7 +102,7 @@
          (string? gem-home)]
    :post [(instance? ScriptingContainer %)]}
   (-> (ScriptingContainer. LocalContextScope/SINGLETHREAD)
-      (prep-scripting-container ruby-load-path gem-home)))
+    (prep-scripting-container ruby-load-path gem-home)))
 
 (defn create-scripting-container
   "Creates an instance of `org.jruby.embed.ScriptingContainer` and loads up the
@@ -110,6 +141,21 @@
         (.put puppet-config dir (fs/absolute-path value))))
     puppet-config))
 
+(schema/defn borrow-with-timeout-fn :- jruby-schemas/JRubyPuppetBorrowResult
+  [timeout :- schema/Int
+   pool :- jruby-schemas/pool-queue-type]
+  (.pollFirst pool timeout TimeUnit/MILLISECONDS))
+
+;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;
+;;; Public
+
+(schema/defn ^:always-validate
+  create-pool-from-config :- jruby-schemas/PoolState
+  "Create a new PoolState based on the config input."
+  [{size :max-active-instances} :- jruby-schemas/JRubyPuppetConfig]
+  {:pool (instantiate-free-pool size)
+   :size size})
+
 (schema/defn ^:always-validate
   create-pool-instance! :- JRubyPuppetInstance
   "Creates a new JRubyPuppet instance and adds it to the pool."
@@ -179,11 +225,6 @@
   [pool :- jruby-schemas/pool-queue-type]
   (.takeFirst pool))
 
-(schema/defn borrow-with-timeout-fn :- jruby-schemas/JRubyPuppetBorrowResult
-  [timeout :- schema/Int
-   pool :- jruby-schemas/pool-queue-type]
-  (.pollFirst pool timeout TimeUnit/MILLISECONDS))
-
 (schema/defn borrow-from-pool!* :- (schema/maybe jruby-schemas/JRubyPuppetInstanceOrRetry)
   "Given a borrow function and a pool, attempts to borrow a JRuby instance from a pool.
   If successful, updates the state information and returns the JRuby instance.
@@ -250,3 +291,18 @@
         (.putFirst pool instance)))
     ;; if we get here, we got a Retry, so we just put it back into the pool.
     (.putFirst (:pool instance) instance)))
+
+(schema/defn ^:always-validate new-main :- jruby-schemas/JRubyMain
+  "Return a new JRuby Main instance which should only be used for CLI purposes,
+  e.g. for the ruby, gem, and irb subcommands.  Internal core services should
+  use `create-scripting-container` instead of `new-main`."
+  [config :- jruby-schemas/JRubyPuppetConfig]
+  (let [jruby-config (RubyInstanceConfig.)
+        {:keys [ruby-load-path gem-home]} config]
+    ; Note, this behavior should remain consistent with prep-scripting-container
+    (doto jruby-config
+      (.setLoadPaths (managed-load-path ruby-load-path))
+      (.setCompatVersion compat-version)
+      (.setCompileMode compile-mode)
+      (.setEnvironment (managed-environment (get-system-env) gem-home)))
+    (Main. jruby-config)))