Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

JDK11+ nashorn scripting, SecurityManager update, Gradle 7.6 #1426

Merged
merged 13 commits into from
Oct 29, 2023

Conversation

tcnh
Copy link
Contributor

@tcnh tcnh commented Mar 21, 2023

For reference.

This branch runs scripting (slim symbol expressions) with Java 11 and up. It incorporates OpenJDK Nashorn instead of depending on the previously built in nashorn scriptengine.
Also removed the no-longer existed methods from the SecurityManager implementation (but it should be removed entirely imho) and upgraded Gradle and other dependencies.

Note: this branch can no longer be compiled using JDK8.

Copy link
Collaborator

@fhoeben fhoeben left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks pretty good.
If you are happy with it I think it might be mergeable once the release notes page is also updated (we'll also need to update the FitNesse.org site as well of course).
Do you have any open issues that should be addressed?

buildSrc/src/main/groovy/LessCompiler.groovy Outdated Show resolved Hide resolved
buildSrc/src/main/groovy/WikiFileListBuilderTask.groovy Outdated Show resolved Hide resolved
this(ENGINE_MANAGER.getEngineByName(engineName));
}

public SlimExpressionEvaluator(ScriptEngine engine) {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why not keep this constructor and feed it the nashhorn engine?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I thought to do it this way, so we had the nashorn import and scriptengine initialization only in this file. I can see the compatibility argument to keep the old constructor. Not really a preference for one or the other.

@fhoeben
Copy link
Collaborator

fhoeben commented Aug 9, 2023

@jediwhale what do you think about switching to Java 11 and dropping Java 8 support. Is that an issue for you?

@jediwhale
Copy link
Collaborator

OK with me

@tcnh tcnh marked this pull request as ready for review August 28, 2023 10:16
fhoeben added a commit to fhoeben/fitnesse that referenced this pull request Oct 29, 2023
# Conflicts:
#	build.gradle
@fhoeben fhoeben merged commit 389bb59 into unclebob:master Oct 29, 2023
1 check passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants