-
-
Notifications
You must be signed in to change notification settings - Fork 360
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 Scala Native Examples #3657
Conversation
Hi @lihaoyi something like this or I have to use scalatags and mainargs? |
Let's use the same scalatags/mainargs example as we did in You can get rid of most of the docs that are already part of |
Alright, it's noted |
//build.mill
package build
import mill._, scalalib._, scalanativelib._
object `package` extends RootModule with ScalaNativeModule {
def scalaVersion = "2.13.11"
def scalaNativeVersion = "0.5.5"
def nativeLinkingOptions = pathToWhereTheTargetWasSaved
object test extends ScalaNativeTests {
def ivyDeps = Agg(ivy"com.lihaoyi::utest:0.8.4")
def testFramework = "utest.runner.Framework"
}
} #makefile
all :
gcc -m64 -shared -c native-src/HelloWorld.c -o target/libstub.so @lihaoyi good day, I've some few questions to ask Is it possible for me to run the commands in When I replace the That's why I asked whether it's possible to run the comments in Lastly, is this what you want for the |
Sure,
Make an upstream task generate the SO file and return a
Yes the |
@lihaoyi what you think |
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.
Took another pass at reviewing this. I think we're getting there! There's still some changes necessary, but it's already in a much better state than it was before
@lihaoyi I've done the changes |
@lolgab could you take a pass at reviewing this? Since you have much more experience with scala native than I do |
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.
Gave a quick look and found some possible improvements
def generateHtml(text: String): CString = { | ||
val colored = Console.RED + "<h1>" + text + "</h1>" + Console.RESET | ||
|
||
implicit val z: Zone = Zone.open | ||
val cResult = toCString(colored) | ||
z.close() | ||
cResult | ||
} |
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 point of this code?
it seems just scala code converted to C and also used after free
. You can't use the CString after closing the Zone.
A better example would be using an open source html templating c library and then convert the C result to a Scala string
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.
It was meant to be a strawman hello-world Scala-Native application, so intentionally overly simple. Is it easy to integrate open source C libraries into scala native? I've never done it before
Thanks @lolgab! @c0d33ngr if we can just pass in the C sources via I think for the suggestion to add a third-party C library to use in the example, let's leave that to a follow-up bounty/PR, since it's a bit out of scope of the original ticket that was intentionally pretty minimal #3647 |
@lolgab is so right!! As long as the C codes are in |
Any more changes needed @lihaoyi ? |
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.
One nit and then I think we're good, @c0d33ngr once you fix the merge conflicts and we get a green CI run I'll merge this and close out the bounty
Alright.. It's all greeny At first, it wasn't but all of a sudden it restarted and ran successfully. Thought, the errors doesn't seem to be about scala-native examples |
@c0d33ngr just a flaky test, not related to your PR. Thanks again for your efforts! Email me your international bank transfer details and I will close out the bounty |
|
||
implicit val z: Zone = Zone.open() | ||
val cResult = toCString(html) | ||
cResult |
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.
Doesn't cResult
escape here the Zone
where it was allocated?
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.
Alright... Noted
I'm into it
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.
1-simple/src/Foo.scala
object Foo {
def generateHtml(text: String) (using Zone) = {
val html = "<h1>" + text + "</h1>\n"
val cResult = toCString(html)
cResult
}
@main
def main(text: String) = Zone {
stdio.printf(generateHtml(text))
}
...
3-multi-module/foo/src/Foo.scala
...
object Foo {
@main
def main(@arg(name = "foo-text") fooText: String,
@arg(name = "bar-text") barText: String): Unit = Zone {
val cFooText = toCString(fooText)
val cBarText = toCString(barText)
...
3-multi-module/bar/src/Bar.scala
...
object Bar {
def main(args: Array[String]): Unit = Zone {
println("Running HelloWorld function")
val result = toCString(args(0))
val barValue = HelloWorldBar.stringLength(result)
...
4-common-config/src/Foo.scala
...
object Foo {
def generateHtml(text: String) (using Zone) = {
val colored = Console.RED + "<h1>" + text + "</h1>" + Console.RESET
val cResult = toCString(colored)
cResult
}
def main(args: Array[String]): Unit = Zone {
val value = generateHtml("hello")
stdio.printf(c"Foo.value: %s\n", value)
}
}
Hello @sideeffffect Here's is the updated code. Are they good to go?
Also please do a review on the scala-native codes I wrote just in case there are some changes needed to be made. It came to my notice that doc should be as good it can be cause future readers would depend on it
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.
@c0d33ngr please open a new PR with these changes, it's easier to discuss individual code changes on the PR, and when we're done we can just click the merge button :)
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.
Alright..
This is a PR to close issue #3647