-
Notifications
You must be signed in to change notification settings - Fork 470
Fix suppressGeneratedFiles was unused
#4348
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
base: master
Are you sure you want to change the base?
Conversation
9eb05a5 to
d62e095
Compare
|
@adam-enko, |
| /** | ||
| * We don't care about the content of those [suppressedFiles] as we use those only as paths to filter sources. | ||
| */ | ||
| @get:Input | ||
| internal val suppressedFilesInput: List<String> | ||
| get() = suppressedFiles.map { it.path }.sorted() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Using the paths as an input will break relocatable build-cache. This could be the cause of the CI failure.
org.jetbrains.dokka.gradle.MultiModuleFunctionalTest > Gradle caching > Gradle caching build cache relocation > Gradle caching build cache relocation relocated project > org.jetbrains.dokka.gradle.MultiModuleFunctionalTest.Gradle caching build cache relocation relocated project should load all generation tasks from cache FAILED
io.kotest.assertions.MultiAssertionError at MultiModuleFunctionalTest.kt:279
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hm, what would be a better way than to rely on paths, but not to rely on the content of the files? Relativize them?
The issue with relying on the content, apart from the fact that we don't care about the content here, is that in that case, our test starts to fail because we now declare an implicit dependency on some files generated by AGP. And that's correct.
|
I think this needs to be fixed another way. It doesn't make sense for It's similar for Instead of users supplying the files to exclude, I think it makes sense for them to pass in glob patterns for files to suppress. Something like this: abstract class DokkaSourceSetSpec {
// ...
@get:Internal // tracked by `actualFiles()`
abstract val sourceRoots: ConfigurableFileCollection
@get:Internal
@Deprecated("Use `suppressedFilesPatterns` instead")
abstract val suppressedFiles: ConfigurableFileCollection
/** Glob patterns, relative to each source root. */
@get:Internal // tracked by `actualFiles()`
abstract val suppressedFilesPatterns: SetProperty<String>
// this is just for task input tracking, it's not used anywhere
@InputFiles
@IgnoreEmptyDirectories
@PathSensitive(PathSensitivity.RELATIVE)
fun actualFiles(): FileCollection {
return objects.fileCollection().from(
sourceRoots
.asFileTree
.matching {
suppressedFilesPatterns.orNull?.forEach { pattern ->
exclude(pattern)
}
}
.files
.sorted()
)
}
// TODO Use this in DokkaSourceSetBuilder instead of `val suppressedFiles`
@Internal
fun suppressedFiles(): FileCollection {
return objects.fileCollection().from(
sourceRoots
.asFileTree
.matching {
suppressedFilesPatterns.orNull?.forEach { pattern ->
include(pattern)
}
}
.files
.sorted()
)
}There's also a new 'generated sources' option in KGP. DGP could integrate with that somehow... |
|
Hm, yes, that looks like a nice idea. The only problem I see is that in that case, we will provide all files separately to Dokka instead of just At this moment, I would really avoid such a significant change. The new KGP API is available only in 2.3.0 (which does not even have a stable version), and is not yet adopted by any third-party plugins, but yes, we can integrate it with the For patterns (or regex), there is #4106. |
I don't think my proposal would affect that. |
Oh, sorry, my bad! Looks like I misinterpreted that. Thanks! |
aa563a6 to
9ed0e73
Compare
Fixes #4217