-
-
Notifications
You must be signed in to change notification settings - Fork 116
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
Add Linux x64 Support #103
Conversation
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.
Can you please also add documentation for the linux part of kotlin-logging
- how to use it
- how it works (briefly)
- how to build it (the c parts)
You can simple add a readme file and later we will incorporate it with the main docs.
I reviewed it (except for the c files)
I will be glad if someone with native experience will review it as well. I will try to ask for help in kotlin slack.
*/ | ||
actual object KMarkerFactory { | ||
actual fun getMarker(name: String): Marker { | ||
TODO("not implemented") //To change body of created functions use File | Settings | File Templates. |
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 remove "To change body of ..." and change todo to UnsupportedOperationException (if you're not going to support it at the moment)
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.
What is the marker for. I saw the interfaces for it. But wasn't entirely sure it's use case?
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.
We used slf4j markers concept so I think quite a good explanation is here: https://stackoverflow.com/questions/16813032/what-are-markers-in-java-logging-frameworks-and-what-is-a-reason-to-use-them
If you want to take ideas from other implementations in kotlin-logging take a look at the js implementation as it is similar by the fact that it's not the original api the library was built with.
) : KLogger { | ||
|
||
override fun entry(vararg argArray: Any?) { | ||
TODO("not implemented") //To change body of created functions use File | Settings | File Templates. |
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.
same for TODO's in this file
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'll address those. First I want to clarify whether we should switch to platform snprintf or use the lib.
adding an upstream c++ lib to do "a thin wrapper around snprintf() function." seems like it could just be a kotlin lib that is "a thin wrapper around snprintf() function." with greater streamlining in terms of system complexity |
The big reason I chose this was it's linking for mobile. I'm relatively new to native, and have little iOs experience. so seeing a library with built in iOs support was what interested me. We can also do a simple
On a native only project. Add a
Compiling the C Library My local development command structure has been.
This results in the following m2 repo tree. The prior name
The linuxX64 contains several klibs which will recompile on any project pulling in depending on it. This is one of the reasons a lot of project also do host detection to determine how to compile the project. Multi Source Set Library If you are consuming it in a project with targets outside of
** Usage ** From a native project it's not much different than the existing implementation.
Notes If we stick with zf_log, I am totally ok with redoing this as a print function. There is a format string that needs to be set. How do we want to set those two variables. Environment variables, text file configuration, or something else. |
Deprecating zfLog. That should be fully removed. I introduced a new abstract class. Which in the The configuration was switched from an object -> data class. This is due to the invalid mutability exception. Recommended reading on the topic The message will be
The log level will always be the first item printed. The default format string is.
This approach allows us to easily add new elements as needed to native. Be it process name, thread, etc. A sample implementation
Corresponding logs
|
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.
Saw the comment about wanting a review.
build.gradle.kts
Outdated
val linuxX64 by sourceSets.creating { | ||
dependsOn(nativeMain) | ||
} | ||
targets.withType(KotlinNativeTarget::class.java) { |
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.
This can just be targets.withType<KotlinNativeTarget> {
.
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.
Switched
@@ -67,6 +79,16 @@ kotlin { | |||
} | |||
} | |||
} | |||
val linuxX64 by sourceSets.creating { |
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.
Creating this source set isn't necessary, since the target will create one already.
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.
The problem I had was referencing via dependsOn
. I was having trouble where this project wouldn't work in a multi platform build. Without creating a nativeMain, and setting dependsOn, which requires a source set. The presets
from konan I tried wasn't working. I realize it's duplicated code but couldn't find a work around.
val linuxX64 by sourceSets.creating {
dependsOn(nativeMain)
}
...
"linuxX64" -> compilations["main"].defaultSourceSet.dependsOn(linuxX64)
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.
Hmm, does auto-complete/intelli-sense work in the IDE as it is?
If it doesn't then compilations["main"].defaultSourceSet.kotlin.srcDir("src/nativeMain/kotlin")
should make it work. Otherwise it doesn't really matter I guess.
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.
KTS results in a bunch of errors in IntelliJ for me. I've tried wiping my settings a number of times but haven't gotten it to work. Essentially no auto complete, or type hinting. Running Intellij Community Latest.
nativeHelper.gradle.kts
Outdated
@@ -0,0 +1,11 @@ | |||
val hostOs = System.getProperty("os.name") |
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.
You can replace all of this with the HostManager
class, which comes with the kotlin gradle plugin. You can then do stuff like, HostManager.isMingw
and HostManager.isLinux
.
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.
Changed over thanks
|
||
object ConsoleAppender : IAppender { | ||
private val STDOUT = platform.posix.fdopen(1, "w") | ||
private val STDERR = platform.posix.fdopen(2, "w") |
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.
Instead of explicitly opening a descriptor, can you use platform.posix.stderr
and platform.posix.stdout
?
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.
Switched
* Gets a date struct, [upstream docs](http://www.cplusplus.com/reference/ctime/tm/) | ||
*/ | ||
fun getDateStruct() = memScoped { | ||
val t = alloc<time_tVar>() |
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.
val t = cValue<time_tVar> { value = time(null) }
is an option. Then you don't need memScoped
.
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 had seen a similar snippet at one point.
Using
val t = cValue<time_tVar> { value = time(null) }
// Gave
Type argument is not within its bounds: should be subtype of 'CStructVar'
// For <time_tVar>
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.
Ah! Nvm then.
@animusnull - whats is the status of the PR from your side? |
I had missed the other feed back by @Dominaezzz |
… built std[err|out] handlers.
Just pushed changes. I still couldn't knock away the explicit linuxX64 source set, and couldn't deprecate Removed native target helper, moving most remaining items into main |
@animusnull - from your perspective this should be merged? or is there anything else you want to add? |
Linux x64 works. I have it building via a fork and publishing artifacts. But there are a couple of catches.
From a code stand point, I think it's good. It's more CI/CD integration. Maybe take the time to build for other platforms. I can take on Windows, but I don't have a iOs, Mac Os device. |
This brings in support for linux x64. This is done via an upstream library zf log
This library will also work for android, iOs, mac os, etc. That being said I've only configured this for linux x64. I'm not familiar with Mac or mobile development.
Build Process Changes
As the library will not be present on most systems. It needs to be compiled and included. I have included a build script, and copied the source and header from the upstream repo with attribution to the initial authors.
Clarification Points