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

fix: get theUnsafe by reflection #1228

Merged
merged 3 commits into from
Jun 10, 2022
Merged

Conversation

tisonkun
Copy link
Contributor

@tisonkun tisonkun commented Jun 5, 2022

This is a follow-up to #1224. Refer to #834.

I'm sorry that I don't push this commit at the first place :(

cc @nedtwigg

Signed-off-by: tison <wander4096@gmail.com>
@@ -102,7 +102,9 @@ private static void openPackages(Collection<String> packagesToOpen) throws Throw
if (modules == null) {
return;
}
final Unsafe unsafe = Unsafe.getUnsafe();
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Otherwise it will cause a failed privilege check:

    @CallerSensitive
    public static Unsafe getUnsafe() {
        Class<?> caller = Reflection.getCallerClass();
        if (!VM.isSystemDomainLoader(caller.getClassLoader()))
            throw new SecurityException("Unsafe");
        return theUnsafe;
    }

@tisonkun
Copy link
Contributor Author

tisonkun commented Jun 6, 2022

@lazystone you may try this patch locally and help verify by:

  1. Checkout the patch:
gh repo clone diffplug/spotless
gh pr checkout 1228
  1. Publish to maven local repository (requires passing jvmargs before we can "bootstrap", we use a latest spotless to check the current spotless IIUC):
./gradlew publishToMavenLocal -x test -Dorg.gradle.jvmargs="--add-exports jdk.compiler/com.sun.tools.javac.api=ALL-UNNAMED --add-exports jdk.compiler/com.sun.tools.javac.file=ALL-UNNAMED --add-exports jdk.compiler/com.sun.tools.javac.parser=ALL-UNNAMED --add-exports jdk.compiler/com.sun.tools.javac.tree=ALL-UNNAMED --add-exports jdk.compiler/com.sun.tools.javac.util=ALL-UNNAMED
  1. Try out with one of your project:
buildscript {
    repositories {
        mavenCentral()
        mavenLocal()
        gradlePluginPortal()
    }

    dependencies {
        classpath 'com.diffplug.spotless:spotless-plugin-gradle:6.7.0-SNAPSHOT'
    }
}

subprojects {
    apply plugin: 'com.diffplug.spotless'
}

I'm sorry that I miss this local commit at #1224 :(

@nedtwigg
Copy link
Member

nedtwigg commented Jun 6, 2022

Out of curiosity, what git client do you use?

@tisonkun
Copy link
Contributor Author

tisonkun commented Jun 6, 2022

@nedtwigg I use bare git command line tool. I forget why I don't push such a fix commit when verified at local >_<

You may notice that the fix is done about half an hour after the last previous commit:

commit 3cc562682d7ab810672fce6d222cf5cf2bbabd5f (HEAD -> open-modules, dev/open-modules)
Author: tison <wander4096@gmail.com>
Date:   Fri Jun 3 00:14:13 2022 +0800

    get theUnsafe by reflection
    
    Signed-off-by: tison <wander4096@gmail.com>

commit 2631a3f276b0350c360ea92eec553e1a8780f5ea
Author: tison <wander4096@gmail.com>
Date:   Thu Jun 2 23:34:18 2022 +0800

    only doOpenInternalPackagesIfRequired when GoogleJavaFormatStep is relevant
    
    Signed-off-by: tison <wander4096@gmail.com>

@nedtwigg
Copy link
Member

nedtwigg commented Jun 6, 2022

No big deal. I am frustrated at the faulty attestation / did not push combo, but I am wayyyyyy more grateful for the very good fix. Thanks very much for contributing, you are wayyyy in the positive sum contribution to the project. You should feel good, not bad!

But I guessed that you were a bare CLI user. Everyone I've worked with in person who is committed to the bare CLI is unreliable about whether they pushed what they thought they pushed or not. Shameless plug for the free DiffPlug git client, but you should definitely use some graphical client or another. I haven't yet worked with someone who can be reliable without one.

I'll verify locally and push this out later this week.

@tisonkun
Copy link
Contributor Author

tisonkun commented Jun 6, 2022

But I guessed that you were a bare CLI user. Everyone I've worked with in person who is committed to the bare CLI is unreliable about whether they pushed what they thought they pushed or not. Shameless plug for the free DiffPlug git client, but you should definitely use some graphical client or another. I haven't yet worked with someone who can be reliable without one.

Thanks for your suggestion! I'll try to adopt one :)

@andrej-urvantsev
Copy link

@tisonkun it worked!

------------------------------------------------------------
Gradle 7.4.2
------------------------------------------------------------

Build time:   2022-03-31 15:25:29 UTC
Revision:     540473b8118064efcc264694cbcaa4b677f61041

Kotlin:       1.5.31
Groovy:       3.0.9
Ant:          Apache Ant(TM) version 1.10.11 compiled on July 10 2021
JVM:          18.0.1 (Eclipse Adoptium 18.0.1+10)
OS:           Linux 5.14.0-1038-oem amd64

If somebody wants to test this using kotlin DSL:

buildscript {
    repositories {
        mavenLocal()
    }
    dependencies {
        classpath("com.diffplug.spotless:spotless-plugin-gradle:6.7.0-SNAPSHOT")
    }
}
apply(plugin = "com.diffplug.spotless")

and instead of

spotless {
//...
}

I had to use

configure<SpotlessExtension> {
    java {
        // https://github.com/google/google-java-format/releases/latest
        googleJavaFormat("1.15.0")
    }
}

@tisonkun
Copy link
Contributor Author

tisonkun commented Jun 9, 2022

@lazystone thanks for your verification.

ping @nedtwigg as a reminder :)

@nedtwigg nedtwigg enabled auto-merge June 10, 2022 00:03
@nedtwigg nedtwigg merged commit 6b66e45 into diffplug:main Jun 10, 2022
@tisonkun tisonkun deleted the open-modules branch June 10, 2022 01:22
@nedtwigg
Copy link
Member

Published in plugin-gradle 6.7.1 and plugin-maven 2.22.7.

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