Support for providing sourceRoots as relative paths

[whyoleg]

Fixes #2571

Note, that only integration test was added, as it's not easily possible to test it via unit test:

1. because we need to materialise files somewhere and to use relative paths of them, we need to materialise them somewhere in project directory - which is not good
2. all our test-apis convert all paths to absolute before providing them to configuration

I think, that for this case, integration test will be enough, as supporting testing relative paths will cause a lot of refactoring for sole of this specific use-case.

I have also checked code around other files provided via configuration, and looks like only sourceRoots are now affected. But may be we need to add more tests for relative paths for other properties (classpath, samples, includes, etc).

One more additional note for future. First idea of mine was to make all Files to become absolute during deserialisation of config, but I've stumbled upon DokkaConfiguration.DokkaModuleDescription#getRelativePathToOutputDirectory, which is File, but should be still relative.

[IgnatBeresnev]

👍

[IgnatBeresnev]

First idea of mine was to make all Files to become absolute during deserialisation of config, but I've stumbled upon DokkaConfiguration.DokkaModuleDescription#getRelativePathToOutputDirectory, which is File, but should be still relative.

Good idea! Instead of deserializing the whole configuration like that, you could just change the source paths to be absolute during configuration's initialization. For example, adding something like that to here:

dokkaConfigurationImpl.copy(
    sourceSets = dokkaConfigurationImpl.sourceSets.map {
        it.copy(
            sourceRoots = it.sourceRoots.map { File(it.absolutePath) }.toSet()
        )
    }
)

Then a deserialization unit test could be added to CliTest.kt

But it's optional. The integration test should hopefully be enough to catch the bugs if something changes in the analysis implementations

[whyoleg]

Yeah, we can do it like this, but may be it's not the best idea as this will only works for CLI. While maven and Gradle now uses absolute paths, I would think, that may be we will need to change this for Gradle plugin some day. Relative paths works better regarding remote build cache, while it sounds not needed now, as mostly documentation is generated for release version, I can easily imagine cases when library published and documentation generated on every commit to master (f.e. in some closed-source project, as an API reference for other teams). And so it's better we follow all Gradle best practices.

Then, there is other problem regarding converting to absolute paths eagerly which I found, but forgot to mention - we have possibility to dump configuration back to json (could be useful in Gradle) - and if doing so, we will then always dump absolute paths - which again, if we will use relative paths in Gradle will cause inconvenience, as sharing config for investigation with relative paths IMO is easier to investigate.

So I would propose to stick with current approach, when we absolutise (is there such a word?) in place, where we need them to be absolute. WDYT?

[vmishenev]

I have not strong opinion about how Dokka should (de)serialize paths.
It seems we should not depend on how the File stores a path.
But, as for me, a check if a file/directory is in (sub)directory is common. It can be extracted into an util function.

While maven and Gradle now uses absolute paths, I would think, that may be we will need to change this for Gradle plugin some day.

A user can define their own paths for sources.

[vmishenev]

I copied it from DefaultDescriptorToDocumentableTranslator.kt, but it is not the best check.
For example, let's suppose we have /home/.../src/main/kotlin and /home/.../src/main/kotlin1. Here we will have a false positive check since the both paths start with /home/.../src/main/kotlin.

[whyoleg]

Not really, it works as expected, as this is not String.startsWith(String) but Path.startsWith(Path) - which does structural check

[vmishenev]

Sorry, I have not noticed it.

But can you check how it works with a shorthand (e.g. ..)? and symbolic links?

[whyoleg]

So, yes, there is a problem that toAbsolutePath will not remove .. and will not follow symlinks. To remove .. we need to use Path.normalize and to follow symlinks and remove .. we need to use Path.toRealPath.
Also, Path.toRealPath will fail if there will be some non-existent directory in the path (even if it will be removed because of ..), while just normalize/toAbsolutePath - not

[whyoleg]

I've replaced Path.toAbsolutePath with Path.toRealPath so symlinks and .. will be fully resolved now.
And also, as I mentioned, execution will now fail if there is some non-existent path, like in a test fixed here: 519e9ff

[vmishenev]

Resolving symlinks can be excessive.
I have tested: e.g. if pathString = "/.../project/src/symlink/file.kt" is resolved with a symlink, absolutePath will lead to a symlink /pathToSymLink/file.kt.

I think it would be safer to use analysisContext.modulesWithFiles[sourceModule] in K2 for filtering. It can help to avoid problems with paths.
But it does not work in K1.

[whyoleg]

Thanks, migrated to analysisContext.modulesWithFiles[sourceModule] in 4553f98

[IgnatBeresnev]

Yeah, we can do it like this, but may be it's not the best idea as this will only works for CLI.

Yep, I wasn't proposing to use it instead of the current fix, but rather in addition to it, for consistency, so that all of our runners pass absolute paths to DokkaConfiguration (granted others also do)

I would think, that may be we will need to change this for Gradle plugin some day

It's a different layer though, no? You can use relative paths in the Gradle Plugin properties, but then once the tasks are called, you have to create the common DokkaConfiguration from dokka-core and pass it to the generator, which might require absolute paths, no? If not, I'm sensing some big problems with regards to breaking abstractions :)

It's not a problem at the moment since DokkaConfiguration does not have any specification about relative/absolute paths, so it's completely up to you. I proposed the idea only because you mentioned you couldn't get it working like that :)

At a certain point I expect we'll document DokkaConfiguration, and specify which paths are expected in which property, so it'll have to be adjusted anyway

[whyoleg]

Oops, looks like I misunderstood how Gradle works with configurations. So then yes, there is no problem here.
Also as Vadim mentioned above A user can define their own paths for sources. even now in maven/gradle, so we need to make them all absolute somewhere anyway, not only in cli.

I believe, that we need to support any kinds of paths in DokkaConfiguration, though, not sure when/where it will be better to fully resolve (absolute, .., symbolic links, etc.) them...
Considering this, Im more in favour of leaving resolve now only in analysis as it's now (where we really need it now)