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.

Sign up for GitHub

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

Jump to bottom

Search in all versions.properties, not just the first one #4830 #4831

Merged
merged 2 commits into from May 21, 2022
Merged

Search in all versions.properties, not just the first one #4830 #4831

Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
087bd79
Fix multiple versions.properties files #4830
May 11, 2022
df7bff6
Improve build error when multiple Detekt versions are present
May 18, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
32 changes: 18 additions & 14 deletions ...t-gradle-plugin/src/main/kotlin/io/gitlab/arturbosch/detekt/extensions/DetektExtension.kt
View file
Open in desktop
Expand Up @@ -107,17 +107,21 @@ open class DetektExtension @Inject constructor(objects: ObjectFactory) : CodeQua
}
}

internal fun loadDetektVersion(classLoader: ClassLoader): String = Properties().run {
val inputStream = classLoader.getResource("versions.properties")!!.openConnection()
/*
* Due to https://bugs.openjdk.java.net/browse/JDK-6947916 and https://bugs.openjdk.java.net/browse/JDK-8155607,
* it is necessary to disallow caches to maintain stability on JDK 8 and 11 (and possibly more).
* Otherwise, simultaneous invocations of Detekt in the same VM can fail spuriously. A similar bug is referenced in
* https://github.com/detekt/detekt/issues/3396. The performance regression is likely unnoticeable.
* Due to https://github.com/detekt/detekt/issues/4332 it is included for all JDKs.
*/
.apply { useCaches = false }
.getInputStream()
load(inputStream)
getProperty("detektVersion")
}
internal fun loadDetektVersion(classLoader: ClassLoader): String =
// Other Gradle plugins can also have a versions.properties.
classLoader.getResources("versions.properties").toList().mapNotNull { versions ->
Properties().run {
val inputStream = versions.openConnection()
/*
* Due to https://bugs.openjdk.java.net/browse/JDK-6947916 and https://bugs.openjdk.java.net/browse/JDK-8155607,
* it is necessary to disallow caches to maintain stability on JDK 8 and 11 (and possibly more).
* Otherwise, simultaneous invocations of Detekt in the same VM can fail spuriously. A similar bug is referenced in
* https://github.com/detekt/detekt/issues/3396. The performance regression is likely unnoticeable.
* Due to https://github.com/detekt/detekt/issues/4332 it is included for all JDKs.
*/
.apply { useCaches = false }
.getInputStream()
load(inputStream)
getProperty("detektVersion")
}
}.distinct().single()
Copy link
Member

Choose a reason for hiding this comment

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

Should this be adapted to take the first instead?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah, maybe instead first but log when there are more than one?

Copy link
Member

Choose a reason for hiding this comment

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

I kind of like the idea of single. If there are more than one and they are different the build should crash.

Maybe we should improve the error message but I think that's out of scope here. That could be tracked/done in other issue/PR.

Copy link
Member

Choose a reason for hiding this comment

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

If there are more than one and they are different the build should crash.

Yeah ideally this 👍

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ok cool! What are the next steps? Would it be worthwhile to open a ticket for the improved error message now? Right now it is something like IllegalArgumentException List has more than one element

Copy link
Member

Choose a reason for hiding this comment

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

Right now it is something like IllegalArgumentException List has more than one element

Yup Ideally I would catch this and error("You're importing two Detekt plugins which are having different versions and will cause unexpected behavior in your build. Make sure th align the versions"). You can also print the list of versions if you wish

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sounds great. How's this?

Caused by: java.lang.IllegalStateException: You're importing two Detekt plugins which are have different versions. (1.20.1, 1.20.0) Make sure to align the versions.
	at io.gitlab.arturbosch.detekt.extensions.DetektExtensionKt.loadDetektVersion(DetektExtension.kt:130)
	at io.gitlab.arturbosch.detekt.extensions.DetektExtension.<init>(DetektExtension.kt:14)
	at io.gitlab.arturbosch.detekt.extensions.DetektExtension_Decorated.<init>(Unknown Source)
	at java.base/jdk.internal.reflect.NativeConstructorAccessorImpl.newInstance0(Native Method)
	at java.base/jdk.internal.reflect.NativeConstructorAccessorImpl.newInstance(NativeConstructorAccessorImpl.java:62)
	at java.base/jdk.internal.reflect.DelegatingConstructorAccessorImpl.newInstance(DelegatingConstructorAccessorImpl.java:45)
	at org.gradle.internal.instantiation.generator.AsmBackedClassGenerator$InvokeConstructorStrategy.newInstance(AsmBackedClassGenerator.java:2078)
	at org.gradle.internal.instantiation.generator.AbstractClassGenerator$GeneratedClassImpl$GeneratedConstructorImpl.newInstance(AbstractClassGenerator.java:488)
	at org.gradle.internal.instantiation.generator.DependencyInjectingInstantiator.doCreate(DependencyInjectingInstantiator.java:64)
	... 197 more

internal fun loadDetektVersion(classLoader: ClassLoader): String {
    // Other Gradle plugins can also have a versions.properties.
    val distinctVersions = classLoader.getResources("versions.properties").toList().mapNotNull { versions ->
        Properties().run {
            val inputStream = versions.openConnection()
                /*
                 * Due to https://bugs.openjdk.java.net/browse/JDK-6947916 and https://bugs.openjdk.java.net/browse/JDK-8155607,
                 * it is necessary to disallow caches to maintain stability on JDK 8 and 11 (and possibly more).
                 * Otherwise, simultaneous invocations of Detekt in the same VM can fail spuriously. A similar bug is referenced in
                 * https://github.com/detekt/detekt/issues/3396. The performance regression is likely unnoticeable.
                 * Due to https://github.com/detekt/detekt/issues/4332 it is included for all JDKs.
                 */
                .apply { useCaches = false }
                .getInputStream()
            load(inputStream)
            getProperty("detektVersion")
        }
    }.distinct()
    return distinctVersions.singleOrNull() ?: error(
        "You're importing two Detekt plugins which have different versions. " +
            "(${distinctVersions.joinToString()}) Make sure to align the versions."
    )
}

Copy link
Member

Choose a reason for hiding this comment

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

Looks great 👍