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

Don't pass --enable-all-security-services to GraalVM >= 21.1 #16584

Merged

Conversation

zakkak
Copy link
Contributor

@zakkak zakkak commented Apr 16, 2021

The option was removed in GraalVM 21.1
oracle/graal#3258

@zakkak zakkak requested a review from geoand April 16, 2021 13:25
@@ -656,7 +656,8 @@ public NativeImageInvokerInfo build() {
if (!protocols.isEmpty()) {
nativeImageArgs.add("-H:EnableURLProtocols=" + String.join(",", protocols));
}
if (enableAllSecurityServices) {
if (enableAllSecurityServices && graalVMVersion.isOlderThan(GraalVM.Version.VERSION_21_1)) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Should we log a warning if the flag is not going to be applied?

Copy link
Contributor Author

@zakkak zakkak Apr 16, 2021

Choose a reason for hiding this comment

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

I thought about it and decided not to since this flag cannot be deprecated yet (it's still needed by GraalVM 20.3 and 21.1).
Printing a warning here would just confuse users IMO.

Instead I think it's OK to silently skip it in GraalVM >= 21.1 and once we drop support for 20.3 and move to >=21.1 we can deprecate the option and print a warning.

@quarkus-bot
Copy link

quarkus-bot bot commented Apr 16, 2021

Failing Jobs - Building c882d22

Status Name Step Test failures Logs Raw logs
Native Tests - Windows - hibernate-validator Build ⚠️ Check → Logs Raw logs

@geoand geoand merged commit 09f5734 into quarkusio:main Apr 16, 2021
@quarkus-bot quarkus-bot bot added this to the 2.0 - main milestone Apr 16, 2021
@zakkak zakkak deleted the enable-all-security-services-removed-in-21.1 branch April 16, 2021 19:41
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants