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

Kotlin2Js Gradle plugin #524

Closed
wants to merge 3 commits into
base: master
from

Conversation

Projects
None yet
3 participants
@abesto
Contributor

abesto commented Nov 6, 2014

No description provided.

@bashor bashor self-assigned this Nov 6, 2014

@bashor

This comment has been minimized.

Member

bashor commented Nov 11, 2014

Sorry for delay!
I'll see it today.

@bashor

This comment has been minimized.

Member

bashor commented Nov 11, 2014

Looks like all commits should be merged in one. Thoughts?

@abesto

This comment has been minimized.

Contributor

abesto commented Nov 11, 2014

Can do. ETA 1 hour.
On Nov 11, 2014 5:52 PM, "Zalim Bashorov" notifications@github.com wrote:

Looks like all commits should be merged in one. Thoughts?


Reply to this email directly or view it on GitHub
#524 (comment).

@bashor

This comment has been minimized.

Member

bashor commented Nov 11, 2014

or maybe two -- impl and tests

abstract protected fun createBlankArgs(): T
public var kotlinOptions: T = createBlankArgs()
public var kotlinDestinationDir : File? = getDestinationDir()

This comment has been minimized.

@bashor

bashor Nov 11, 2014

Member

Unnecessary space before colon(:)

protected fun isJava(it: File): Boolean = FilenameUtils.getExtension(it.getName()).equalsIgnoreCase("java")
private fun getKotlinSources(): ArrayList<File> = getSource()

This comment has been minimized.

@bashor

bashor Nov 11, 2014

Member

The call chain may be written in one line

This comment has been minimized.

@bashor

bashor Nov 11, 2014

Member

Why just not List?

This comment has been minimized.

@abesto

abesto Nov 11, 2014

Contributor

Good point :)

@abesto

This comment has been minimized.

Contributor

abesto commented Nov 11, 2014

Fixed the two code style comments, squash coming up soon.

args.freeArgs = sources.map { it.getAbsolutePath() }
abstract protected fun populateTargetSpecificArgs(args: T, sources: ArrayList<File>)

This comment has been minimized.

@bashor

bashor Nov 11, 2014

Member

looks like we don't need sources in populateTargetSpecificArgs

This comment has been minimized.

@bashor

bashor Nov 11, 2014

Member

I think will be better if move up populateTargetSpecificArgs and afterCompileHook declarations, because it's part of API of this class

This comment has been minimized.

@abesto

abesto Nov 11, 2014

Contributor

Absolutely.

@abesto abesto force-pushed the abesto:gradle-js branch from e554509 to 73794d1 Nov 11, 2014

.filterNot { isJava(it) }
.toArrayList()
protected fun populateCommonArgs(args: T, sources: ArrayList<File>) {

This comment has been minimized.

@bashor

bashor Nov 11, 2014

Member

should be it private?

This comment has been minimized.

@abesto

abesto Nov 11, 2014

Contributor

In an ideal world, yes. I don't know enough to confidently say no-one will ever need to override it. If you say go, I'll change it.

This comment has been minimized.

@bashor

bashor Nov 11, 2014

Member

let's do it :)

protected fun isJava(it: File): Boolean = FilenameUtils.getExtension(it.getName()).equalsIgnoreCase("java")
private fun getKotlinSources(): ArrayList<File> = getSource()
.filterNot { isJava(it) }

This comment has been minimized.

@bashor

bashor Nov 11, 2014

Member

I think more properly to check that it's kotlin file.

P.S. I see that it's legacy code

This comment has been minimized.

@bashor

bashor Nov 11, 2014

Member

You can get file extension by file.extension.
And You can use JetFileType.EXTENSION instead of magic string.

This comment has been minimized.

@abesto

abesto Nov 11, 2014

Contributor

FYI, I'll rebase to the latest master to get JetFileType.EXTENSION. Would have to do it anyway. I'll squash the commits again once we're done with the review.

This comment has been minimized.

@bashor

This comment has been minimized.

@abesto

abesto Nov 11, 2014

Contributor

JavaFileType has slightly different constants, so I used the common interface to make the symmetry obvious.

private fun getJavaSourceRoots(): HashSet<File> = getSource()
.filter { isJava(it) }
.map { findSrcDirRoot(it) }
.filter { it != null }

This comment has been minimized.

@bashor

bashor Nov 11, 2014

Member

May be replaced with filterNotNull

This comment has been minimized.

@abesto

abesto Nov 11, 2014

Contributor

Great! I spent a minute or so looking for this method but couldn't find it for some reason.

ExitCode.INTERNAL_ERROR -> throw GradleException("Internal compiler error. See log for more details")
else -> {}
}
private fun getJavaSourceRoots(): HashSet<File> = getSource()

This comment has been minimized.

@bashor

bashor Nov 11, 2014

Member

May be List?

This comment has been minimized.

@abesto

abesto Nov 11, 2014

Contributor

It needs to be a set because generally there will be several files in each source root, leading to multiple entries for each source root. But I've generalized it from HashSet to just Set.

This comment has been minimized.

@bashor
fun outputFile(): String {
return if (StringUtils.isEmpty(kotlinOptions.outputFile)) {
"${kotlinDestinationDir}/app.js"

This comment has been minimized.

@bashor

bashor Nov 11, 2014

Member

I think outputFile can not be omitted and if it omitted compilation should be failed.

This comment has been minimized.

@abesto

abesto Nov 11, 2014

Contributor

Isn't this the same case as destination for the JVM task? There the plugin defaults to kotlinDestinationDir. This is basically the same logic. Of course we don't need to keep symmetry between the plugins.

I don't have a strong preference, it's up to you :)

This comment has been minimized.

@bashor

bashor Nov 11, 2014

Member

They are similar but not the same.
In k2js compiler right now file name will be used as module name, so I think this parameter should be provided explicitly. (Probably in the future it will be replaced with moduleName)

This comment has been minimized.

@bashor

bashor Nov 11, 2014

Member

As far as i understand JVM task, by default, use project's build dir.

This comment has been minimized.

@abesto

abesto Nov 11, 2014

Contributor

Ok, that makes sense. I'm convinced.

@abesto abesto force-pushed the abesto:gradle-js branch from a364d64 to faa76ba Nov 11, 2014

[TaskAction] fun rewrite() {
val file = File(sourceMapPath())
val text = file.readText("UTF-8")

This comment has been minimized.

@bashor

bashor Nov 11, 2014

Member

charset parameter may be omitted -- UTF-8 will by use by default

@abesto

This comment has been minimized.

Contributor

abesto commented Nov 11, 2014

Sorry for the delay, I shot myself in the foot by upgrading my default java to 8, causing strange gradle internal errors. Almost done with "fail if outputFile not set".

@abesto

This comment has been minimized.

Contributor

abesto commented Nov 11, 2014

... and then proceeded to shoot myself in the foot repeatedly with various tools. Anyway, that's it for me today. Next time I'll have more than 10 minutes to hack on this will probably be Thursday. Thanks for the review so far!

@bashor

This comment has been minimized.

Member

bashor commented Nov 11, 2014

Thanks for your efforts!

@abesto abesto force-pushed the abesto:gradle-js branch from 6462fba to 158e73e Nov 12, 2014

compilerClass = javaClass<Kotlin2JsCompile>()) {
val copyKotlinJsTaskName = sourceSet.getTaskName("copy", "kotlinJs")
val clean = project.getTasks().findByName("clean")
val compileJava = project.getTasks().findByName(sourceSet.getCompileJavaTaskName()) as AbstractCompile?

This comment has been minimized.

@bashor

bashor Nov 12, 2014

Member

Why we need javaTask?

This comment has been minimized.

@abesto

abesto Nov 15, 2014

Contributor

Because a bunch of tasks are added to its dependencies, so that they run on gradle build.

This comment has been minimized.

@bashor

bashor Nov 15, 2014

Member

Could we avoid to use it?

This comment has been minimized.

@abesto
}
}
public open class RewritePathsInSourceMap() : DefaultTask() {

This comment has been minimized.

@bashor

bashor Nov 12, 2014

Member

Ideally it should be fixed in compiler as part of KT-4078. You can just add TODO comment that this hack should be dropped after KT-4078 will be fixed or fix it :)

This comment has been minimized.

@abesto

abesto Nov 15, 2014

Contributor

Ah, nice to know this is something you've already thought about. I'd prefer not to attack 4078, added TODOs in abesto@c50a32e

override fun createBlankArgs(): K2JSCompilerArguments = K2JSCompilerArguments()
public fun addLibraryFiles(vararg fs: String) {

This comment has been minimized.

@bashor

bashor Nov 12, 2014

Member

I think we should extract all libraries from dependencies instead.

This comment has been minimized.

@abesto

abesto Nov 15, 2014

Contributor

Agree, thanks for pointing it out. Done in abesto@99df661 and abesto@fe3060d

clean?.dependsOn("clean" + kotlinTaskName.capitalize())
createCopyKotlinJsTask(jsLibraryJar)
createCopyKotlinSourcesForSourceMapTask()

This comment has been minimized.

@bashor

bashor Nov 13, 2014

Member

I think we should not be able to copy source files.

This comment has been minimized.

@abesto

abesto Nov 15, 2014

Contributor

I'm not exactly sure what you mean. Let me explain the use-case I tried to solve here, maybe that's an answer; if not, please clarify.

There are two things you may want to do with source maps. One, use them locally. In this case it doesn't matter where the Kotlin sources are, the browser will find them over the file:// protocol. Two, distribute the source-map and Kotlin sources with the project; either as a debug build of the application, or as an example of kotlin2js. In this case the Kotlin sources need to be copied to a place the sourcemap can reference; also it needs to be edited to point to paths relative to the sourcemap (that's what createRewritePathsInSourceMapTask does).

This comment has been minimized.

@bashor

bashor Nov 15, 2014

Member

I think that this gradle task should not be able to copy source files because user can do it in plain gradle if need, and how he want. And maybe it can lead to accidental leakage of sources.
But we should provide better way to setup source map generation(KT-4078).

This comment has been minimized.

@abesto

abesto Nov 15, 2014

Contributor

Fair enough. Removed them in abesto@dbeacf0

@bashor

This comment has been minimized.

Member

bashor commented Nov 15, 2014

You can find some my minor changes in https://github.com/JetBrains/kotlin/commits/rr/z/pr/524__4.
It includes:

  • fixes of windows specific issues;
  • temporary disable assert for source map which fails on windows(build log;
  • temporary drop copying source files(AFAIR something fails on windows).

You can see build results for this branch here

@abesto

This comment has been minimized.

Contributor

abesto commented Nov 15, 2014

Thanks for linking your changes, they look good to me. Should I merge / cherry-pick them into this branch, or will you take care of that after the next squash?

@bashor

This comment has been minimized.

Member

bashor commented Nov 15, 2014

Would be nice if You cherry-pick my changes.

@abesto abesto force-pushed the abesto:gradle-js branch from 3e1461c to 5c5519d Nov 15, 2014

@abesto

This comment has been minimized.

Contributor

abesto commented Nov 15, 2014

Done. Also: does adab044 mean that I shouldn't add kotlin-js-library as a dependency (and thus add the jar to libraryFiles) anymore? Edit: atm I can't try it out because my build against master fails with [error] /Users/abesto/playground/kotlin/libraries/stdlib/src/kotlin/io/ReadWrite.kt: (156, 37) Type mismatch: inferred type is java.util.stream.Stream<kotlin.String!>! but kotlin.Stream<kotlin.String> was expected, not yet sure if I messed up my local env or there's really a bug.

@bashor

This comment has been minimized.

Member

bashor commented Nov 15, 2014

does adab044 mean that I shouldn't add kotlin-js-library as a dependency (and thus add the jar to libraryFiles) anymore?

I think in maven and in gradle user should declare kotlin-js-library as dependency explicitly.
Probably You already saw that I added args.noStdlib = true in your code

@abesto

This comment has been minimized.

Contributor

abesto commented Nov 15, 2014

Ok, I'll make it so. I'm curious: is there a reason besides "let's not surprise the developer by adding dependencies"?

@bashor

This comment has been minimized.

Member

bashor commented Nov 15, 2014

What version of the compiler you?
Did you rebase on current master?

@bashor

This comment has been minimized.

Member

bashor commented Nov 15, 2014

They(maven, gradle) have own dependency manager and probably will be proper to use it.

@abesto

This comment has been minimized.

Contributor

abesto commented Nov 15, 2014

Agree with using Gradle to define the dependency, that's how it's implemented currently. The plugin uses the Gradle dependency manager to set kotlin-js-library as a compile dependency. I take it you don't think we should do this, the user should specify it explicitly; I'll remove the code from the plugin that adds the dependency.

@bashor

This comment has been minimized.

Member

bashor commented Nov 15, 2014

For developing Kotlin we use plugin from https://teamcity.jetbrains.com/guestAuth/repository/download/bt345/bootstrap.tcbuildtag/updatePlugins.xml

You can find more information here

@abesto

This comment has been minimized.

Contributor

abesto commented Nov 15, 2014

Wrt/ the build error: rebased on latest master (321e758). I'm using CLI tools to build. Did ant -f update_dependencies.xml, then ant -f build.xml dies with the above error. AFAICT I don't even have a working Kotlin compiler since I can't build the stdlib.

@abesto

This comment has been minimized.

Contributor

abesto commented Nov 15, 2014

./dependencies/bootstrap-compiler/Kotlin/kotlinc/bin/kotlinc-jvm -version                                                                                                                                                           22:29:22
INFO: Kotlin Compiler version 0.9.686
@bashor

This comment has been minimized.

Member

bashor commented Nov 15, 2014

0.9.686 is OK, it's latest bootstrap compiler. So, I'll try build your branch locally.

@bashor

This comment has been minimized.

Member

bashor commented Nov 15, 2014

I got it -- it's conflict between kotlin's Stream and java's Stream, try to run build on older runtime.

@abesto

This comment has been minimized.

Contributor

abesto commented Nov 15, 2014

Bah. This again.

Nice, thanks for figuring it out, build works with Java 7. Will now fix if any tests are broken and squash the commits.

@bashor

This comment has been minimized.

Member

bashor commented Nov 15, 2014

I take it you don't think we should do this, the user should specify it explicitly; I'll remove the code from the plugin that adds the dependency.

Yes, for maven and gradle we prefer this way.

Anyway we should provide a way to build with and without stdlib.

abesto and others added some commits Nov 15, 2014

Code quality, bugfixes
Conflicts:
	libraries/tools/kotlin-gradle-plugin-core/src/main/kotlin/org/jetbrains/kotlin/gradle/plugin/KotlinPlugin.kt

@abesto abesto force-pushed the abesto:gradle-js branch from a326c3d to 55cce01 Nov 15, 2014

@abesto

This comment has been minimized.

Contributor

abesto commented Nov 19, 2014

Quick check, are you waiting for some changes, or just didn't get around to moving on with this?

@bashor

This comment has been minimized.

Member

bashor commented Nov 19, 2014

Hi, Zoltán!

Sorry for delay!

Right now I waiting some changes in master, I hope it'll happen in some days.
This changes allow to avoid your "hack" in kotlin-javascript-library and allow to use kotlin2js gradle project from IDEA.

Additionally we don't sure that our buildtools(ant, maven, gradle) should copy js files from libraries to out dir. Maybe user should do it himself(by own tools)?
Feel free to share your thoughts.

Thanks!

@abesto

This comment has been minimized.

Contributor

abesto commented Nov 19, 2014

No problem at all, thanks for the prompt response.

Avoiding hacks is great, I'm fine with waiting for it.

About not copying the library to the out dir: as a user of the plugins, I have no idea where to find the kotlin.js for the version of Kotlin I'm using in my project. OTOH, I can see how automatically copying the file is not clean. How about providing a task type, but not creating a task by default? That way the user can manage what happens explicitly, without having to think about Kotlin internals.

@bashor

This comment has been minimized.

Member

bashor commented Nov 21, 2014

Good points, thanks! Let's continue discussion here.

@bashor

This comment has been minimized.

Member

bashor commented Dec 24, 2014

Hi, @abesto!

I pushed PR commits manually with minor fixes.

@bashor

This comment has been minimized.

Member

bashor commented Dec 24, 2014

Thanks a lot for your contributions!

@bashor

This comment has been minimized.

Member

bashor commented Dec 24, 2014

Additionally I turn off now copy js kotlin.js from jar, it's current common behavior of all our build tools. we'll continue to think how simplify build process.

@bashor bashor closed this Dec 24, 2014

@abesto

This comment has been minimized.

Contributor

abesto commented Dec 24, 2014

\o/ best 🎄 🎁 :)

@yanex

This comment has been minimized.

Files with ".kts" extension are Kotlin source files as well.

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