User-side library generator. #3538
Conversation
| var target: String? = null | ||
| var index = 0 | ||
| var saveTemps = false | ||
| while (index < args.size) { |
| ArgType.String, "output-directory", "o", "Output directory").required() | ||
| val target by argParser.option( | ||
| ArgType.String, "target", "t", "Compilation target").required() | ||
| val saveTemps by argParser.argument( |
There was a problem hiding this comment.
Option has value, argument - does not.
There was a problem hiding this comment.
IMHO in this case a lot more convenient is optional option --save-temps
If present - save, if not - delete.
There was a problem hiding this comment.
As far as I understand, argument is something passed without a flag at all. So currently the tools saves temporary data if the following args are passed:
./generate-platform ... --target macos_x64 true
There was a problem hiding this comment.
For me it works as generate-platform ... --target mingw save-temps
There was a problem hiding this comment.
OK, reworked here a bit, indeed boolean option seems to be better fit.
| fun dependsOn(defFile: DefFile): Boolean { | ||
| if (defFile == this) return true | ||
| for (dependency in this.depends) { | ||
| if (defFile.dependsOn(dependency)) return true |
There was a problem hiding this comment.
Shouldn't it be dependency.dependsOn(defFile)?
There was a problem hiding this comment.
Actually not needed anyway.
| destinationDir distDir | ||
|
|
||
| from(project("platformLibs").file("src/platform")) { | ||
| into('klib/platformDef') |
There was a problem hiding this comment.
Abuses klib directory. Maybe konan/platformDef or konan/targets/$target/platformDef?
| @@ -472,6 +481,7 @@ task bundle(type: (isWindows()) ? Zip : Tar) { | |||
| } | |||
|
|
|||
| task bundleRestricted(type: Tar) { | |||
| dependsOn("distDef") | |||
There was a problem hiding this comment.
So bundle doesn't include these files on non-Apple hosts?
There was a problem hiding this comment.
Yeah, was unsure here as well. Added to bundle dependency.
|
|
||
| from(project("platformLibs").file("src/platform")) { | ||
| into('klib/platformDef') | ||
| } |
There was a problem hiding this comment.
Are .sh scripts included intentionally? I don't expect these to work properly.
| private class DefFile(val name: String, val depends: MutableList<DefFile>) { | ||
|
|
||
| override fun hashCode(): Int { | ||
| return name.hashCode() |
There was a problem hiding this comment.
Why not rely on identity-based equals/hashCode? Do you expect multiple DefFile objects for the same .def-file?
That would seem wrong, considering that there is mutable state in DefFile.
There was a problem hiding this comment.
Yeah, it was mostly for clearness of intentions.
| while (markBlack.size < defFiles.size) { | ||
| visit(defFiles[index++]) | ||
| } | ||
| return result |
There was a problem hiding this comment.
Isn't it actually a reverse topological order?
There was a problem hiding this comment.
Depends on what we believe to be an order, I guess. First goes ones with least deps.
There was a problem hiding this comment.
Exactly. And in topological order first element has no dependencies on it.
| // Now run interop tool on toposorted dependencies. | ||
| sorted.forEach { def -> | ||
| executorPool.execute { | ||
| // A bit ugly, we just block here until all dependencies are built. |
There was a problem hiding this comment.
Maybe use ForkJoinPool instead?
There was a problem hiding this comment.
Not sure if there's big difference here.
There was a problem hiding this comment.
It would help to make this code more simple and avoid blocking threads with tasks that aren't ready to start.
| "-target", target, | ||
| "-def", file.absolutePath, | ||
| "-compiler-option", "-fmodules-cache-path=$outputDirectory/clangModulesCache", | ||
| "-repo", "${outputDirectory.absolutePath}", |
There was a problem hiding this comment.
This one is likely not needed.
There was a problem hiding this comment.
Was needed for me, otherwise indirect dependencies were not resolved.
| "-no-default-libs", "-no-endorsed-libs", "-Xpurge-user-libs", | ||
| *def.depends.flatMap { listOf("-l", "$outputDirectory/${it.name}") }.toTypedArray()) | ||
| println("Processing ${def.name}...") | ||
| K2Native.mainNoExit(invokeInterop("native", args)) |
There was a problem hiding this comment.
Why not use
instead?There was a problem hiding this comment.
This way it's more clear, wrt intentions afterward.
There was a problem hiding this comment.
This way it requires more maintenance. I believe that calling the entry point of cinterop tool is clear enough.
intentions afterward.
Which ones?
| // A bit ugly, we just block here until all dependencies are built. | ||
| while (def.depends.any { built[it] == 0 }) { | ||
| Thread.sleep(100) | ||
| } |
There was a problem hiding this comment.
Doesn't it hang if a dependency is failed to build?
No description provided.