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

Do not fail on non-existing or “empty” script compilation #331

Closed
wants to merge 1 commit into
from

Conversation

Projects
None yet
2 participants
@eskatos
Member

eskatos commented Apr 13, 2017

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 added this to the 0.9.0 milestone Apr 13, 2017

@eskatos eskatos self-assigned this Apr 13, 2017

@eskatos eskatos requested a review from bamboo Apr 13, 2017

@bamboo

I think we should try to capture the different script source semantics in a data type and use that type instead of File when talking to CachingKotlinCompiler so it can decide how to best create cache keys.

Show outdated Hide outdated samples/hello-world/settings.gradle
@@ -1 +1 @@
rootProject.buildFileName = 'build.gradle.kts'
rootProject.buildFileName = 'buildZog.gradle.kts'

This comment has been minimized.

@bamboo

bamboo Apr 13, 2017

Member

What's the purpose of this change?

@bamboo

bamboo Apr 13, 2017

Member

What's the purpose of this change?

This comment has been minimized.

@eskatos

eskatos Apr 13, 2017

Member

Oops, fixed 😄

@eskatos

eskatos Apr 13, 2017

Member

Oops, fixed 😄

subprojects {
apply {

This comment has been minimized.

@bamboo

bamboo Apr 13, 2017

Member

We should make sure cli/build.gradle.kts can still be edited with the correct classpath when this sample is imported in IntelliJ.

@bamboo

bamboo Apr 13, 2017

Member

We should make sure cli/build.gradle.kts can still be edited with the correct classpath when this sample is imported in IntelliJ.

val scriptFile = scriptResource.file!!
val script = convertLineSeparators(scriptResource.text!!)
val scriptFilePath = scriptSource.fileName!!
val scriptFile = File(scriptFilePath)

This comment has been minimized.

@bamboo

bamboo Apr 13, 2017

Member

From what I understood, scriptSource.fileName doesn't necessarily point to an existing file, right?

If that's the case we shouldn't be creating a File out of it as it might be used in cache keys later on (see compilePluginsBlockOf).

@bamboo

bamboo Apr 13, 2017

Member

From what I understood, scriptSource.fileName doesn't necessarily point to an existing file, right?

If that's the case we shouldn't be creating a File out of it as it might be used in cache keys later on (see compilePluginsBlockOf).

This comment has been minimized.

@eskatos

eskatos Apr 13, 2017

Member

The File instance here is just a vehicule for "path" and "filename". It isn't used as a file.
When the file doesn't exists, it carries its intended path.
For the very special useEmptySettings() case the path will be ${cwd}/script_${content_hash}.

Given that we require Java 8 we could use Path instead to better convey the intention.

@eskatos

eskatos Apr 13, 2017

Member

The File instance here is just a vehicule for "path" and "filename". It isn't used as a file.
When the file doesn't exists, it carries its intended path.
For the very special useEmptySettings() case the path will be ${cwd}/script_${content_hash}.

Given that we require Java 8 we could use Path instead to better convey the intention.

additionalSourceFiles: List<File>,
classPath: ClassPath,
parentClassLoader: ClassLoader): CompiledScript {
val scriptFileName = scriptFile.name
return compileScript(cacheKeyPrefix + scriptFileName + scriptFile, classPath, parentClassLoader) {
return compileScript(cacheKeyPrefix + scriptFileName + script, classPath, parentClassLoader) { cacheDir ->

This comment has been minimized.

@bamboo

bamboo Apr 13, 2017

Member

This behaves slightly different as File and String have different cache key semantics.

I imagine it might have a (very small) performance impact when the same build script file is reused many times in a single build.

Just an observation.

@bamboo

bamboo Apr 13, 2017

Member

This behaves slightly different as File and String have different cache key semantics.

I imagine it might have a (very small) performance impact when the same build script file is reused many times in a single build.

Just an observation.

This comment has been minimized.

@eskatos

eskatos Apr 13, 2017

Member

Yep, performance wise the difference is that before we hashed the file from disk and now we hash its content, that was already loaded in any case, from memory.

@eskatos

eskatos Apr 13, 2017

Member

Yep, performance wise the difference is that before we hashed the file from disk and now we hash its content, that was already loaded in any case, from memory.

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

This comment has been minimized.

Show comment
Hide comment
@eskatos

eskatos Apr 13, 2017

Member

After discussing this with @bamboo, we decided step back and fix that once we migrated to the new Kotlin embedded compiler API.

Member

eskatos commented Apr 13, 2017

After discussing this with @bamboo, we decided step back and fix that once we migrated to the new Kotlin embedded compiler API.

@eskatos eskatos closed this Apr 13, 2017

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