Merge pull request #976 from daspilker/groovy-shell

use Groovy Shell instead of Groovy Script Engine to run DSL scripts
jenkinsci · Jan 3, 2017 · bf05859 · bf05859
2 parents 97e460f + 43be01f
commit bf05859
Show file tree

Hide file tree

Showing 8 changed files with 151 additions and 114 deletions.
diff --git a/docs/Home.md b/docs/Home.md
@@ -33,6 +33,8 @@ Browse the Jenkins issue tracker to see any [open issues](https://issues.jenkins
    ([JENKINS-39769](https://issues.jenkins-ci.org/browse/JENKINS-39769))
  * Updated optional [Managed Scripts Plugin](https://wiki.jenkins-ci.org/display/JENKINS/Managed+Script+Plugin)
    dependency to version 1.3
+ * Use Groovy Shell instead of Groovy Script Engine to run DSL scripts, see [Migration](Migration#migrating-to-155)
+   ([#976](https://github.com/jenkinsci/job-dsl-plugin/pull/976))
 * 1.54 (December 24 2016)
  * Enhanced support for the [S3 Plugin](https://wiki.jenkins-ci.org/display/JENKINS/S3+Plugin)
    ([#953](https://github.com/jenkinsci/job-dsl-plugin/pull/953))

diff --git a/docs/Migration.md b/docs/Migration.md
@@ -1,3 +1,11 @@
+## Migrating to 1.55
+
+### ScriptRequest
+
+Some constructors and the `location` property in `javaposse.jobdsl.dsl.ScriptRequest` are
+[[deprecated|Deprecation-Policy]] and will be removed. The `body` and `scriptPath` properties should be used instead.
+The class is part of the internal implementation and should not affect DSL scripts.
+
 ## Migrating to 1.54
 
 ### Embedded API Viewer

diff --git a/job-dsl-core/src/main/groovy/javaposse/jobdsl/Run.groovy b/job-dsl-core/src/main/groovy/javaposse/jobdsl/Run.groovy
@@ -32,8 +32,9 @@ class Run {
         }
 
         args.each { String scriptName ->
-            String scriptPath = new File(scriptName).absolutePath
-            ScriptRequest request = new ScriptRequest(scriptName, null, cwdURL, false, scriptPath)
+            File scriptFile = new File(scriptName)
+            String scriptBody = scriptFile.getText('UTF-8')
+            ScriptRequest request = new ScriptRequest(scriptBody, cwdURL, false, scriptFile.absolutePath)
             GeneratedItems generatedItems = new DslScriptLoader(jm).runScripts([request])
 
             for (GeneratedJob job : generatedItems.jobs) {

diff --git a/job-dsl-core/src/main/groovy/javaposse/jobdsl/dsl/DslScriptLoader.groovy b/job-dsl-core/src/main/groovy/javaposse/jobdsl/dsl/DslScriptLoader.groovy
@@ -3,11 +3,12 @@ package javaposse.jobdsl.dsl
 import org.codehaus.groovy.control.CompilationFailedException
 import org.codehaus.groovy.control.CompilerConfiguration
 import org.codehaus.groovy.control.customizers.ImportCustomizer
-import org.codehaus.groovy.runtime.InvokerHelper
 
 import java.util.logging.Level
 import java.util.logging.Logger
 
+import static groovy.lang.GroovyShell.DEFAULT_CODE_BASE
+
 /**
  * Runs provided DSL scripts via an external {@link JobManagement}.
  */
@@ -34,48 +35,24 @@ class DslScriptLoader {
      * @since 1.45
      */
     GeneratedItems runScripts(Collection<ScriptRequest> scriptRequests) throws IOException {
-        ClassLoader parentClassLoader = DslScriptLoader.classLoader
-        CompilerConfiguration config = createCompilerConfiguration()
-
-        // Otherwise baseScript won't take effect
-        GroovyClassLoader groovyClassLoader = new GroovyClassLoader(parentClassLoader, config)
-
-        try {
-            runScriptsWithClassLoader(scriptRequests, groovyClassLoader, config)
-        } finally {
-            if (groovyClassLoader instanceof Closeable) {
-                ((Closeable) groovyClassLoader).close()
-            }
-        }
-    }
-
-    /**
-     * Executes the script and returns the generated items.
-     *
-     * @since 1.47
-     */
-    GeneratedItems runScript(String script) throws IOException {
-        runScripts([new ScriptRequest(script)])
-    }
-
-    private GeneratedItems runScriptsWithClassLoader(Collection<ScriptRequest> scriptRequests,
-                                                     GroovyClassLoader groovyClassLoader,
-                                                     CompilerConfiguration config) {
         GeneratedItems generatedItems = new GeneratedItems()
-        Map<String, GroovyScriptEngine> engineCache = [:]
-
+        CompilerConfiguration config = createCompilerConfiguration()
+        Map<String, GroovyShell> groovyShellCache = [:]
         try {
             scriptRequests.each { ScriptRequest scriptRequest ->
                 String key = scriptRequest.urlRoots*.toString().sort().join(',')
 
-                GroovyScriptEngine engine = engineCache[key]
-                if (!engine) {
-                    engine = new GroovyScriptEngine(scriptRequest.urlRoots, groovyClassLoader)
-                    engine.config = config
-                    engineCache[key] = engine
+                GroovyShell groovyShell = groovyShellCache[key]
+                if (!groovyShell) {
+                    groovyShell = new GroovyShell(
+                            new URLClassLoader(scriptRequest.urlRoots, DslScriptLoader.classLoader),
+                            new Binding(),
+                            config
+                    )
+                    groovyShellCache[key] = groovyShell
                 }
 
-                JobParent jobParent = runScriptEngine(scriptRequest, engine)
+                JobParent jobParent = runScriptEngine(scriptRequest, groovyShell)
 
                 generatedItems.configFiles.addAll(
                         extractGeneratedConfigFiles(jobParent.referencedConfigFiles, scriptRequest.ignoreExisting)
@@ -93,40 +70,54 @@ class DslScriptLoader {
                 scheduleJobsToRun(jobParent.queueToBuild)
             }
         } finally {
-            engineCache.values().each { GroovyScriptEngine engine ->
-                if (engine?.groovyClassLoader instanceof Closeable) {
-                    ((Closeable) engine.groovyClassLoader).close()
+            groovyShellCache.values().each { GroovyShell groovyShell ->
+                if (groovyShell.classLoader instanceof Closeable) {
+                    ((Closeable) groovyShell.classLoader).close()
+                }
+                if (groovyShell.classLoader.parent instanceof Closeable) {
+                    ((Closeable) groovyShell.classLoader.parent).close()
                 }
             }
         }
-
         generatedItems
     }
 
-    private JobParent runScriptEngine(ScriptRequest scriptRequest, GroovyScriptEngine engine) {
-        LOGGER.log(Level.FINE, String.format("Request for ${scriptRequest.location}"))
+    /**
+     * Executes the script and returns the generated items.
+     *
+     * @since 1.47
+     */
+    GeneratedItems runScript(String script) throws IOException {
+        runScripts([new ScriptRequest(script)])
+    }
 
-        Binding binding = createBinding(scriptRequest)
+    private JobParent runScriptEngine(ScriptRequest scriptRequest, GroovyShell groovyShell) {
         try {
-            Script script
-            if (scriptRequest.body != null) {
+            if (scriptRequest.scriptPath || scriptRequest.location) {
+                String scriptName = scriptRequest.location ?: new File(scriptRequest.scriptPath).name
+                logger.println("Processing DSL script ${ scriptName}")
+                checkValidScriptName(scriptName)
+                checkCollidingScriptName(scriptName, groovyShell.classLoader, logger)
+            } else {
                 logger.println('Processing provided DSL script')
-                Class cls = engine.groovyClassLoader.parseClass(scriptRequest.body, 'script')
-                script = InvokerHelper.createScript(cls, binding)
+            }
+
+            GroovyCodeSource source
+            if (scriptRequest.body != null) {
+                source = new GroovyCodeSource(
+                        scriptRequest.body, scriptRequest.scriptPath ?: 'script', DEFAULT_CODE_BASE
+                )
             } else {
-                logger.println("Processing DSL script ${scriptRequest.location}")
-                checkValidScriptName(scriptRequest.location)
-                checkCollidingScriptName(scriptRequest.location, engine.groovyClassLoader, logger)
-                script = engine.createScript(scriptRequest.location, binding)
+                source = new GroovyCodeSource(new URL(scriptRequest.urlRoots[0], scriptRequest.location))
             }
-            assert script instanceof JobParent
+            Script script = groovyShell.parse(source)
+            script.binding = createBinding(scriptRequest)
+            script.binding.setVariable('jobFactory', script)
 
             JobParent jobParent = (JobParent) script
             jobParent.setJm(jobManagement)
 
-            binding.setVariable('jobFactory', jobParent)
-
-            script.run()
+            jobParent.run()
 
             return jobParent
         } catch (CompilationFailedException e) {

diff --git a/job-dsl-core/src/main/groovy/javaposse/jobdsl/dsl/ScriptRequest.groovy b/job-dsl-core/src/main/groovy/javaposse/jobdsl/dsl/ScriptRequest.groovy
@@ -5,6 +5,7 @@ import groovy.transform.EqualsAndHashCode
 @EqualsAndHashCode
 class ScriptRequest {
     // Starting Object
+    @Deprecated
     final String location
 
     // Starting Script
@@ -23,11 +24,21 @@ class ScriptRequest {
         this(null, body, new File('.').toURI().toURL())
     }
 
+    ScriptRequest(String body, URL urlRoot, boolean ignoreExisting = false, String scriptPath = null) {
+        this(null, body, [urlRoot] as URL[], ignoreExisting, scriptPath)
+    }
+
+    ScriptRequest(String body, URL[] urlRoots, boolean ignoreExisting = false, String scriptPath = null) {
+        this(null, body, urlRoots, ignoreExisting, scriptPath)
+    }
+
+    @Deprecated
     ScriptRequest(String location, String body, URL urlRoot, boolean ignoreExisting = false,
                   String scriptPath = null) {
         this(location, body, [urlRoot] as URL[], ignoreExisting, scriptPath)
     }
 
+    @Deprecated
     ScriptRequest(String location, String body, URL[] urlRoots, boolean ignoreExisting = false,
                   String scriptPath = null) {
         this.location = location