scoverage / gradle-scoverage

A plugin to enable the use of Scoverage in a gradle Scala project
Apache License 2.0
53 stars 38 forks source link

Fix: ScoverageReport task inputs declaration #139

Closed CristianGM closed 4 years ago

CristianGM commented 4 years ago

As you can see in Gradle documentation an input of type File shouldn't be annotated as @Input, in this case they are directories, so @InputDirectory seems more appropriate, and using Relative path sensitivity you allow it to be relocatable (so, for example shared between 2 different users, or CI and a dev).

I found that because a project had a report showing a class that was deleted, just the class, inside a file with more classes, and if you look on the @InputDirectory documentation it will make clear why:

Note: To make the task dependent on the directory's location but not its contents, expose the path of the directory as an Input property instead.

eyalroth commented 4 years ago

Thank you for the contribution!

According to the the lazy configuration documentation, it seems that file properties should now be declared with:

project.objects.fileProperty()
// or
project.objects.directoryProperty()

So maybe it's worth changing those as well.

I'm not sure I understand why is the @PathSensitive(RELATIVE) annotation is added. I thought that the default is ABSOLUTE (according to the documentation). Wouldn't this change break builds?

CristianGM commented 4 years ago

Absolute is the default because Gradle doesn't want to break any legacy build, but Relative should always be the starting point. When you use absolute, if you have 2 clones of the project in different folders it will be a miss, but using relative it's a hit. We don't care about the absolute path of the files, we care about the files and the content, so relative is perfect.

And yes, both inputs and the output should be a DirectoryProperty, but that's a bigger change and I just wanted to hotfix the issue (that is hurting us, and will make people lose trust on the Gradle BuildCache)

CristianGM commented 4 years ago

Just to add more context about why absolute is the default (from Gradle Slack):

If you handle a relative path as absolute it’ll probably continue working (though provide a cache miss) where the opposite will break

Also, changing the default could introduce problems unnoticed in existing builds

Gradle did not have shared/remote caching until 3.5. For up-to-date checking there’s little use in normalizing paths, so input snapshotting worked with absolute paths as the default.

eyalroth commented 4 years ago

If you handle a relative path as absolute it’ll probably continue working (though provide a cache miss) where the opposite will break

Also, changing the default could introduce problems unnoticed in existing builds

So it actually seems that if there are current builds which use absolute paths in these options, this change will break them, and perhaps even introduce problems unnoticed.

CristianGM commented 4 years ago

No, hehe, if Gradle changes that. So that's why for Gradle the default is still Absolute, while if I remember correctly they will start showing warnings for tasks using Absolute (or simply not specifying the path sensitivity).

This task doesn't require the path to be absolute (I don't think any task should), but Gradle wouldn't ever do a change that can make builds to misbehave when they use old plugins.

eyalroth commented 4 years ago

It seems like you've researched this pretty well :)

Unfortunately, I don't have merge permissions on this repository, but the PR looks good to me.