-
Notifications
You must be signed in to change notification settings - Fork 50
C interop build integration support #29
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
base: main
Are you sure you want to change the base?
Conversation
|
|
||
| class CinteropSettings : SchemaNode() { | ||
| @SchemaDoc("A list of .def files for cinterop generation.") | ||
| var defs: List<String> by value(emptyList()) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's use List<Path> here, since those are files. It will provide better diagnostics and IDE completion.
|
|
||
| val cinteropTasks = fragment.settings.native?.cinterop?.defs.orEmpty().map { defFile -> | ||
| // Create a unique task name for each def file. | ||
| val taskNameSuffix = defFile.substring(defFile.lastIndexOf('/') + 1) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ideally we should rely on the Path type and use name (or nameWithoutExtension) instead of this substring
| val cinteropTaskName = NativeTaskType.Cinterop.getTaskName(module, platform, isTest, buildType) | ||
| .let { TaskName(it.name + "-" + taskNameSuffix) } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why not directly use the suffix parameter of getTaskName?
|
|
||
| val modulePath = moduleDir.toNioPath() | ||
| val discoveredDefPaths = discoveredDefFiles.map { | ||
| it.toNioPath().relativeTo(modulePath).toString().replace('\\', '/') |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please use Path.invariantSeparatorsPathString instead of string manipulation to better convey the intent
| |----------------|-------------------------------------------|---------| | ||
| | `defs: list` | A list of `.def` files for cinterop generation. | (empty) | | ||
|
|
||
| By convention, Amper automatically discovers all `.def` files located in the `resources/cinterop` directory of a native fragment. The `defs` property can be used to include `.def` files from other locations. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we need some design here. Could you please describe the use cases for this approach? Why do we need to support explicit and implicit def files, respectively?
I'm not very fond of browsing the fs tree during model parsing in general (for perf reasons, because the auto-sync in the IDE should be super fast), so it's important to understand why we're doing this (cc @Jeffset, please feel free to jump in).
Also, from what I see in the interop docs, there seems to be more parameters that users should be able to customize when registering definition files. In Gradle it can look like this:
cinterops {
val libcurl by creating {
definitionFile.set(project.file("src/nativeInterop/cinterop/libcurl.def"))
packageName("com.jetbrains.handson.http")
compilerOpts("-I/path")
includeDirs.allHeaders("path")
}
}
|
Hey! I'd like to thank you again for the effort and the reactivity. I'm not using a lot of sugar in my PR comments, so if something sounds harsh or direct at first glance, please read the text while imagining a gentle and polite tone 😄 I haven't finished reviewing by any means, but I figured it was worth doing a first pass quickly. |
No description provided.