Skip to content

Fix: ScoverageReport task inputs declaration #139

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

Merged

Conversation

CristianGM
Copy link
Contributor

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
Copy link
Contributor

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
Copy link
Contributor Author

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
Copy link
Contributor Author

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
Copy link
Contributor

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
Copy link
Contributor Author

CristianGM commented May 30, 2020

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
Copy link
Contributor

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.

@maiflai maiflai merged commit b657ecc into scoverage:master Jun 9, 2020
@CristianGM CristianGM deleted the fix-scoverage-report-cacheability branch June 10, 2020 12:56
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants