RFR: 8314327: Issues with JShell when using "local" execution engine [v2]

[Archie Cobbs]

On Thu, 5 Oct 2023 22:31:31 GMT, Archie Cobbs <acobbs at openjdk.org> wrote:

>> Seems adding `createExecutionControl()` to `LocalExecutionControlProvider` should be enough? Something along these lines possibly?
>> 
>> diff --git a/src/jdk.jshell/share/classes/jdk/jshell/execution/LocalExecutionControlProvider.java b/src/jdk.jshell/share/classes/jdk/jshell/execution/LocalExecutionControlProvider.java
>> index cc3879c5c6d..370fbb6bdfd 100644
>> --- a/src/jdk.jshell/share/classes/jdk/jshell/execution/LocalExecutionControlProvider.java
>> +++ b/src/jdk.jshell/share/classes/jdk/jshell/execution/LocalExecutionControlProvider.java
>> @@ -25,6 +25,7 @@
>>  
>>  package jdk.jshell.execution;
>>  
>> +import java.util.List;
>>  import java.util.Map;
>>  import jdk.jshell.spi.ExecutionControl;
>>  import jdk.jshell.spi.ExecutionControlProvider;
>> @@ -78,7 +79,31 @@ public Map<String,String> defaultParameters() {
>>       */
>>      @Override
>>      public ExecutionControl generate(ExecutionEnv env, Map<String, String> parameters) {
>> -        return new LocalExecutionControl();
>> +        ExecutionControl result = createExecutionControl(env, parameters);
>> +
>> +        // Apply any configured class path
>> +        List<String> remoteOptions = env.extraRemoteVMOptions();
>> +        int classPathIndex = remoteOptions.indexOf("--class-path") + 1;
>> +        if (classPathIndex > 0 && classPathIndex < remoteOptions.size()) {
>> +            try {
>> +                result.addToClasspath(remoteOptions.get(classPathIndex));
>> +            } catch (ExecutionControl.ExecutionControlException e) {
>> +                throw new RuntimeException("error configuring class path", e);
>> +            }
>> +        }
>> +
>> +        return result;
>>      }
>>  
>> +    /**
>> +     * Create the {@link ExecutionControl} using the specified delegate.
>> +     * Called from {@link #generate(ExecutionEnv, Map) }.
>> +     *
>> +     * @param env the {@code ExecutionEnv} for which the ExecutionControl should be created
>> +     * @param parameters the parameters that were passed to {@code generate}
>> +     * @return the newly created {@code ExecutionControl}
>> +     */
>> +    public ExecutionControl createExecutionControl(ExecutionEnv env, Map<String, String> parameters) {
>> +        return new LocalExecutionControl(new DefaultLoaderDelegate());
>> +    }
>>  }
>
>> Seems adding createExecutionControl() to LocalExecutionControlProvider should be enough? Something along these lines possibly?
> 
> Ah yes, that would work... because `DirectExecutionControl.addToClasspath()` invokes `LoaderDelegate.addToClasspath()`.
> 
> It's better place to apply the class path fix at that higher level anyway.
> 
> Thanks! I've applied this in 03565459ec3 ... let me know what you think.

> @archiecobbs Thanks for this work; any chance this will get sponsored & merged?

I hope so...   @lahodaj any remaining issues?

-------------

PR Comment: https://git.openjdk.org/jdk/pull/15311#issuecomment-1841278478