Skip to content

Use the correct source directory set on JVM #5163

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
Merged
merged 7 commits into from
Oct 6, 2022
Merged

Use the correct source directory set on JVM #5163

merged 7 commits into from
Oct 6, 2022
+35 −29

Conversation

Goooler
Copy link
Contributor

@Goooler Goooler commented Aug 1, 2022

Same fixes borrowed from square/wire#2310.

Goooler added 3 commits August 1, 2022 09:51
@Goooler
Reuse SourceSet.kotlin

Verified

This commit was created on GitHub.com and signed with GitHub’s verified signature. The key has expired.
GPG key ID: 4AEE18F83AFDEB23
Expired
Verified
Learn about vigilant mode
e959b22
@Goooler
Avoid NPEs in Kotlin 1.7.20
bd745db
@Goooler
Inline
6557141
@3flex

This comment was marked as duplicate.

Sign in to view
3flex
3flex requested changes Aug 1, 2022
View reviewed changes
Copy link
Member

@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.

cortinico
cortinico reviewed Aug 1, 2022
View reviewed changes
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
Goooler
Goooler commented Aug 30, 2022
View reviewed changes
detekt-gradle-plugin/src/main/kotlin/io/gitlab/arturbosch/detekt/internal/DetektJvm.kt
Comment on lines +27 to +34
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.

@Goooler Goooler requested review from cortinico and removed request for 3flex August 31, 2022 01:28
@BraisGabin BraisGabin requested a review from 3flex September 4, 2022 10:20
@Goooler
Merge branch 'main' into fixKotlin1720
172d579 
# 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
Member

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!

@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
Member

3flex commented Sep 11, 2022 •
edited
Loading

This is probably most relevant:

https://docs.gradle.org/current/userguide/upgrading_version_7.html#all_convention_deprecation

And https://youtrack.jetbrains.com/issue/KT-47047/Migrate-Kotlin-Gradle-Plugin-from-using-Gradle-conventions

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

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
@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.

@Goooler Goooler mentioned this pull request Oct 6, 2022
Merged
@Goooler Goooler deleted the fixKotlin1720 branch October 6, 2022 12:48
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@3flex 3flex

@cortinico cortinico

Assignees
No one assigned
Labels
gradle-plugin
Projects
None yet
Milestone
1.22.0
Development

Successfully merging this pull request may close these issues.

None yet
4 participants
@Goooler @3flex @cortinico @BraisGabin