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

Setting non-existent Kotlin build script in settings.gradle should not fail #302

Closed
pioterj opened this Issue Mar 14, 2017 · 8 comments

Comments

Projects
None yet
3 participants
@pioterj
Member

pioterj commented Mar 14, 2017

Setting non-existent build script file with .kts extension results in settings.gradle like:

rootProject.buildFileName = 'build.non-existent.kts'

results in build failing with:

kotlin.KotlinNullPointerException (no error message)

It should not fail as build scripts are optional.

@bamboo bamboo added this to the 0.9.0 milestone Mar 23, 2017

@eskatos

This comment has been minimized.

Show comment
Hide comment
@eskatos

eskatos Apr 12, 2017

Member

If the file doesn't exists, Gradle provides a NonExistentFileScriptSource.
The Groovy provider compiles the text provided by this ScriptSource, which is "".
The Kotlin provider works with scriptSource.resource.file which is null in this case, causing the NPE.

Member

eskatos commented Apr 12, 2017

If the file doesn't exists, Gradle provides a NonExistentFileScriptSource.
The Groovy provider compiles the text provided by this ScriptSource, which is "".
The Kotlin provider works with scriptSource.resource.file which is null in this case, causing the NPE.

@bamboo

This comment has been minimized.

Show comment
Hide comment
@bamboo

bamboo Apr 12, 2017

Member

I think there are two different use cases here:

  1. For a project, the default build script (automatically detected or defined in settings.gradle) is optional and so we shouldn't throw if the file isn't there
  2. For a script plugin, e.g. apply from: "someScript.gradle.kts", we should throw if the file isn't there

That reasoning tells me we shouldn't be fixing 1 at the scripting provider level but at a higher-level.

Member

bamboo commented Apr 12, 2017

I think there are two different use cases here:

  1. For a project, the default build script (automatically detected or defined in settings.gradle) is optional and so we shouldn't throw if the file isn't there
  2. For a script plugin, e.g. apply from: "someScript.gradle.kts", we should throw if the file isn't there

That reasoning tells me we shouldn't be fixing 1 at the scripting provider level but at a higher-level.

@eskatos eskatos self-assigned this Apr 12, 2017

@eskatos

This comment has been minimized.

Show comment
Hide comment
@eskatos

eskatos Apr 12, 2017

Member

The Gradle runtime already distinguishes the various use cases and fails with a proper error message when it has to.

A scripting provider may decide to add stuff to build scripts, e.g. base plugins like we envision in order to solve #320. Even when the file isn't there because it is optional.

So, I think this boils down to the contracts of NonExistentFileScriptSource and it's companion EmptyFileTextResource being loosely defined (null File), and, the Gradle Script Kotlin provider failing to handle the non-existing-script-file case.

Member

eskatos commented Apr 12, 2017

The Gradle runtime already distinguishes the various use cases and fails with a proper error message when it has to.

A scripting provider may decide to add stuff to build scripts, e.g. base plugins like we envision in order to solve #320. Even when the file isn't there because it is optional.

So, I think this boils down to the contracts of NonExistentFileScriptSource and it's companion EmptyFileTextResource being loosely defined (null File), and, the Gradle Script Kotlin provider failing to handle the non-existing-script-file case.

@eskatos

This comment has been minimized.

Show comment
Hide comment
@eskatos

eskatos Apr 13, 2017

Member

When it comes to the settings scripts there actually are three cases:

  1. specified settings script file exists
  2. specified settings script file does not exists
  3. using empty settings, there's no File involved, see SettingsLocation and BuildLayout in GBT

In the 1st case the ScriptSource passed to the ScriptPluginFactory is a UriScriptSource, already properly handled.

In the 2nd case it is a NonExistentFileScriptSource whose TextResource returns a null File. We need to accommodate here.

In the 3rd case the ScriptSource passed to the ScriptPluginFactory is a StringScriptSource that use its class name as fileName. In that case we can't distinguish the "file" path and name.

So, the KotlinScriptPluginFactory must not use the nullable file property of the ScriptSource#resource in order to handle all those cases while letting the Gradle runtime make the decisions wrt. optional scripts and eventual failures.

Member

eskatos commented Apr 13, 2017

When it comes to the settings scripts there actually are three cases:

  1. specified settings script file exists
  2. specified settings script file does not exists
  3. using empty settings, there's no File involved, see SettingsLocation and BuildLayout in GBT

In the 1st case the ScriptSource passed to the ScriptPluginFactory is a UriScriptSource, already properly handled.

In the 2nd case it is a NonExistentFileScriptSource whose TextResource returns a null File. We need to accommodate here.

In the 3rd case the ScriptSource passed to the ScriptPluginFactory is a StringScriptSource that use its class name as fileName. In that case we can't distinguish the "file" path and name.

So, the KotlinScriptPluginFactory must not use the nullable file property of the ScriptSource#resource in order to handle all those cases while letting the Gradle runtime make the decisions wrt. optional scripts and eventual failures.

@eskatos eskatos changed the title from Setting non-existent Kotlin build script in settings.gradle results in unhelpful error message to Setting non-existent Kotlin build script in settings.gradle should not fail Apr 13, 2017

@bamboo

This comment has been minimized.

Show comment
Hide comment
@bamboo

bamboo Apr 13, 2017

Member

About case 3, why should Gradle even call the ScriptPluginFactory?

Member

bamboo commented Apr 13, 2017

About case 3, why should Gradle even call the ScriptPluginFactory?

@bamboo

This comment has been minimized.

Show comment
Hide comment
@bamboo

bamboo Apr 13, 2017

Member

With regards to my question above, I clarify: why should Gradle even call the ScriptPluginFactory to compile an empty Settings plugin?

Member

bamboo commented Apr 13, 2017

With regards to my question above, I clarify: why should Gradle even call the ScriptPluginFactory to compile an empty Settings plugin?

@eskatos

This comment has been minimized.

Show comment
Hide comment
@eskatos

eskatos Apr 13, 2017

Member

@bamboo it's calling the ScriptPluginFactory with an "empty text" ScriptSource to give it a chance to eventually enhance it. See my #302 (comment) above

Member

eskatos commented Apr 13, 2017

@bamboo it's calling the ScriptPluginFactory with an "empty text" ScriptSource to give it a chance to eventually enhance it. See my #302 (comment) above

eskatos added a commit that referenced this issue Apr 13, 2017

Do not fail on non-existing or “empty” script compilation
Do not consume ScriptSource TextResource nullable file property.
Rework the multi-kotlin-project-config-injection sample to demonstrate
a subproject with no build script.

See #302
@bamboo

This comment has been minimized.

Show comment
Hide comment
@bamboo

bamboo Apr 13, 2017

Member

Ok. I guess it makes sense to treat all plugin targets homogeneously in that regard.

Member

bamboo commented Apr 13, 2017

Ok. I guess it makes sense to treat all plugin targets homogeneously in that regard.

eskatos added a commit that referenced this issue Apr 13, 2017

Do not fail on non-existing or “empty” script compilation
Do not consume ScriptSource TextResource nullable file property.
Rework the multi-kotlin-project-config-injection sample to demonstrate
a subproject with no build script.

See #302

@eskatos eskatos removed their assignment Apr 13, 2017

eskatos added a commit that referenced this issue Apr 13, 2017

Do not fail on non-existing or “empty” script compilation
Do not consume ScriptSource TextResource nullable file property.
Rework the multi-kotlin-project-config-injection sample to demonstrate
a subproject with no build script.

See #302

@eskatos eskatos self-assigned this May 5, 2017

@eskatos eskatos closed this May 10, 2017

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment