NodeTask's getScript should be Relative sensitivity

[jfbibeau]

Again apologies for not directly raising a PR due to my company's open source contribution rules.

In NodeTask, there is an @InputFiles annotation on getScript, which is the script used to execute with node. To allow caching to work properly across different machines, then this should be annotated with the relative path annotation (and InputFiles should be InputFile):

    @InputFile
    @PathSensitive(PathSensitivity.RELATIVE)
    File getScript()
    {
        return this.script
    }

Thanks!

[bsautel]

Thanks for this contribution that makes the plugin better.

You are right, the path sensitivity should be relative and the @InputFile is enough for a single file.

I just come to fix it in the master branch.

[bsautel]

Hum, changing @InputFiles to @InputFile broke the build, more precisely a series of integration tests that are run after gradle build and that I always forget to execute before pushing.

@deepy , if it is possible, don't you think we should try to integrate these integration examples in our integration tests so that they are automatically ran by gradle build?

In the example that is broken, the Node.js script is called index.js and is configured this way: script = file( 'src/node' ). In this situation, Node.js looks for a file called index.js and executes it.
When using @InputFile the task does not want to execute because the path is not a file. I think it explains why it was @InputFiles.

I think we should let @InputFiles unless we want to remove support of this Node.js feature.

[jfbibeau]

I believe leaving it as @InputFiles will lead to some subtle caching bugs. Right now this will consider every file in the src/node directory as inputs to the cache, but really it should only care about the actual script (1 file) that NodeJS will execute. For example:

src/node/index.js
src/node/unrelated.js

node src/node/index.js

If I change unrelated.js, it should still be a cache hit, as long as I haven't changed index.js. Marking all of src/node as input, will cause a cache miss if anything under src/node/* changes which is not accurate.

I believe the fact that node executes index.js if you pass a folder to CLI is a funny quirk (and is not even documented in the CLI as far as I can tell), so I believe we should err on the side of accurate caching in the plugin vs supporting this quirky scenario. A user wanting to do that would simply have to append index.js to the script property (which is way more clear anyways), so not a huge deal breaker.

My 2 cents!

[bsautel]

I agree with you, this would be better replacing @InputFiles by @InputFile.

@deepy , are you okay to make this change and thus introducing a backward-compatibility break? It can be fixed very quickly for instance in the example project by replacing file( 'src/node' ) by file( 'src/node/index.js' ). If it's okay for you, I will push this change. We will add to document that in the release notes.

[deepy]

I guess the easiest way is to note this change for the next milestone and in the getter for script check if it's a directory, if it is we log a warning about deprecated functionality and that it'll be dropped in the next major but return script + /index.js

[bsautel]

That's a nice way to handle this issue @deepy.
I just come to push a commit that does that.
If it is okay for you, I'll close the issue.