Skip to content
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

Port to Scala Native #160

Merged
merged 6 commits into from
Aug 13, 2017
Merged

Port to Scala Native #160

merged 6 commits into from
Aug 13, 2017

Conversation

olafurpg
Copy link
Collaborator

@olafurpg olafurpg commented Jul 9, 2017

Opening this PR just to show progress, currently waiting for new release of scodec bits and Scala Native to fix remaining issues. Tests pass for fastparse/scalaparse/cssparse when using a local publish of scala native with scala-native/scala-native#902.

@olafurpg
Copy link
Collaborator Author

olafurpg commented Aug 8, 2017

Now blocked by scala-native/scala-native#925 (EDIT) for the fastparseByte module.

Also hit linking errors for classparseNative/test

[info] Linking (1939 ms)
[error] cannot link: @java.lang.Class::getDeclaredFields_scala.scalanative.runtime.ObjectArray
[error] cannot link: @java.lang.Class::getMethod_java.lang.String_scala.scalanative.runtime.ObjectArray_java.lang.reflect.Method
[error] cannot link: @java.lang.Class::getMethods_scala.scalanative.runtime.ObjectArray
[error] cannot link: @java.lang.reflect.Field::getName_java.lang.String
[error] cannot link: @java.lang.reflect.Field::getType_java.lang.Class
[error] cannot link: @java.lang.reflect.Method
[error] cannot link: @java.lang.reflect.Method::getDeclaringClass_java.lang.Class
[error] cannot link: @java.lang.reflect.Method::getName_java.lang.String
[error] cannot link: @java.lang.reflect.Method::getParameterTypes_scala.scalanative.runtime.ObjectArray
[error] cannot link: @java.lang.reflect.Method::getReturnType_java.lang.Class
[error] cannot link: @java.lang.reflect.Method::invoke_java.lang.Object_scala.scalanative.runtime.ObjectArray_java.lang.Object
[error] unable to link

@lihaoyi
Copy link
Member

lihaoyi commented Aug 8, 2017

@olafurpg
https://github.com/lihaoyi/fastparse/blob/3da22d6480746b1582353b4b157c7dbba2686ff3/classparse/shared/src/test/scala/classparse/TestUtils.scala#L26 is using Java reflection to load bytecode, in order to feed it through the parser as a sanity check (here too https://github.com/lihaoyi/fastparse/blob/3da22d6480746b1582353b4b157c7dbba2686ff3/classparse/jvm/src/test/scala/classparse/ProjectTests.scala#L29)

We are basically asserting that the data-model of the classfile parsed by ClassParse lines up with the data-model you can find in java-reflection.

I'd be happy just disabling that entire suite of tests of Scala-Native. Apart from these bulk classloader-load-classfile-and-parse tests, that logic is all reasonable covered by unit tests in https://github.com/lihaoyi/fastparse/blob/3da22d6480746b1582353b4b157c7dbba2686ff3/classparse/shared/src/test/scala/classparse/ClassTests.scala so I'm not too afraid of it getting silently broken in Scala-Native

@olafurpg
Copy link
Collaborator Author

I linked the wrong issue above, I meant fastparseByte is blocked by scala-native/scala-native#925 I disabled NativePlatform for fastparseByte and its dependents classparse and perftests.

The only remaining issue is that the following warning prints when loading sbt

Project defines platforms: js, jvm
project fastparse

  discards: native
Project defines platforms: js, jvm
project fastparse

  discards: native
project pythonparse

  discards: native
project scalaparse

  discards: native
project cssparse

  discards: native

even if native is enabled for those projects. I cancelled all the non-native builds on Travis to see if the CI can reproduce the warnings.

@olafurpg olafurpg changed the title [WIP] Port to Scala Native Port to Scala Native Aug 12, 2017
@olafurpg olafurpg mentioned this pull request Aug 12, 2017
@olafurpg
Copy link
Collaborator Author

olafurpg commented Aug 12, 2017

The CI failures seem to be unrelated to this PR, https://travis-ci.org/lihaoyi/fastparse/jobs/263232670#L1528 twitter/scalading is failing in ProjectTests

We will have to wait until scala-native/scala-native#925 is fixed to cross-build fastparseByte to native. utils/fastparse/scalaparse/pythonparse/cssparse are cross-built.

This PR is ready for review. Would be nice to merge #159 first. Only annoyance is the warnings on sbt load that are printed by sbt-crossproject portable-scala/sbt-crossproject#52 @densh mentioned the warning might be spurious in this case.

@lihaoyi
Copy link
Member

lihaoyi commented Aug 13, 2017

Looks fine to me. Feel free to take Scalding out of the test suite if it's misbehaving to make things green again

- scodec-bits has been ported by is still pending a release
  scodec/scodec-bits#72
- Scala Native 0.3.1 includes classfiles for javalib in the published
  jars, which cause problems with c.eval in macros. Fixed in
  scala-native/scala-native#902
Stuck on ??? errors in HeapByteBuffer

```
[info]     endianness        Success
[info]         short        Failure('scala.NotImplementedError: an implementation is missing')
[info]         int        Failure('scala.NotImplementedError: an implementation is missing')
[info]         long        Failure('scala.NotImplementedError: an implementation is missing')
[info]         float        Failure('scala.NotImplementedError: an implementation is missing')
[info]         double        Failure('scala.NotImplementedError: an implementation is missing')
```
The spurious "discards: native" warnings are still printed.
@olafurpg
Copy link
Collaborator Author

CI is green! 🎉 A sanity check in the logs shows native binaries are running https://travis-ci.org/lihaoyi/fastparse/jobs/264040997#L2488

I'll go ahead and merge this. Note that sbt still prints that annoying warning on startup, https://travis-ci.org/lihaoyi/fastparse/jobs/264040984#L865 I hope it doesn't bug you too much before it gets fixed in sbt-crossproject @lihaoyi 😜

@olafurpg olafurpg merged commit 04687c4 into com-lihaoyi:master Aug 13, 2017
@olafurpg olafurpg deleted the native branch August 13, 2017 13:17
@lihaoyi
Copy link
Member

lihaoyi commented Aug 13, 2017

Yeah I've never been very strict about warnings. You have my publishing credentials already IIRC, so feel free to publish when you're ready

@olafurpg
Copy link
Collaborator Author

fastparse 0.4.4 has been released with scala native support! changelog pending in #163

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants