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

Use the correct source directory set on JVM #5163

Merged
merged 7 commits into from Oct 6, 2022

Conversation

Goooler
Copy link
Contributor

@Goooler Goooler commented Aug 1, 2022

Same fixes borrowed from square/wire#2310.

@3flex

This comment was marked as duplicate.

Copy link
Collaborator

@3flex 3flex left a comment

Choose a reason for hiding this comment

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

This logic doesn't make sense to me - it seems that in the case of a null source set, a detekt task would still be created, but source would be set to an empty value - so we'd be left with a task that will be skipped with a NO-SOURCE status.

If we need to filter out null source sets (and it would be good to understand what change has been made in the Kotlin Gradle Plugin that led to the change in behaviour) that should be done in a way that skips registration of a task that won't execute. I'd suggest logging a warning would be a good idea in that case as well.

Copy link
Member

@cortinico cortinico left a comment

Choose a reason for hiding this comment

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

Thanks for sending this over 🙏

@Goooler Goooler requested review from cortinico and 3flex and removed request for cortinico August 30, 2022 11:54
@Goooler Goooler changed the title Avoid NPEs in Kotlin 1.7.20 Use the correct source directory set on JVM Aug 30, 2022
private fun Project.registerJvmDetektTask(
compilation: KotlinCompilation<KotlinCommonOptions>,
extension: DetektExtension,
inputSource: FileCollection
) {
registerDetektTask(DetektPlugin.DETEKT_TASK_NAME + compilation.name.capitalize(), extension) {
setSource(inputSource)
classpath.setFrom(inputSource, compilation.compileDependencyFiles)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Seems we can merge DetektJvm into DetektMultiplatform, see Goooler#3.

# Conflicts:
#	detekt-gradle-plugin/src/main/kotlin/io/gitlab/arturbosch/detekt/internal/DetektJvm.kt
@cortinico
Copy link
Member

@3flex how do you feel about this change?

It looks promising on my end buy I'm lacking a bit of context. @Goooler could you expand on the PR description on why this is needed?

@Goooler
Copy link
Contributor Author

Goooler commented Sep 8, 2022

@cortinico See square/wire#2321.

@3flex
Copy link
Collaborator

3flex commented Sep 11, 2022

I support this change, using the new approach - I'd still like to know what's changing in Kotlin 1.7.20 that means we have to make this change now, but putting that aside, we should do this anyway because:

  1. The concept of Conventions is deprecated in Gradle, and KGP will drop them soon enough, requiring changes like this
  2. As Goooler pointed out, it should allow consolidation of logic between MPP and Java variants at some point
  3. Something like this will be needed for Align inputs for detekt type resolution tasks with Kotlin compilation tasks #4465

I just wish we had better test coverage!

3flex
3flex approved these changes Sep 11, 2022
@cortinico
Copy link
Member

The concept of Conventions is deprecated in Gradle, and KGP will drop them soon enough, requiring changes like this

TIL. Is this documented somehwere?

@3flex
Copy link
Collaborator

3flex commented Sep 11, 2022

@3flex 3flex mentioned this pull request Sep 20, 2022
@BraisGabin
Copy link
Collaborator

This is ready to merge, right? I would vote to merge it for 1.22.0-RC2.

@BraisGabin BraisGabin added this to the 1.22.0 milestone Oct 5, 2022
@cortinico cortinico merged commit d642648 into detekt:main Oct 6, 2022
21 checks passed
@cortinico
Copy link
Member

This is ready to merge, right? I would vote to merge it for 1.22.0-RC2.

Yup fine with merging this for RC2

@Goooler
Copy link
Contributor Author

Goooler commented Oct 6, 2022

https://github.com/detekt/detekt/actions/runs/3196227698/jobs/5217876383

Seems org.jetbrains.kotlin.gradle.plugin.mpp.pm20.targets was removed from Kotlin 1.7.20, I'll fix it later.

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.

None yet

4 participants