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.

Sign up for GitHub

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

Jump to bottom

Support Scala Native #340

Merged
merged 3 commits into from
Nov 14, 2021
Merged

Support Scala Native #340

merged 3 commits into from
Nov 14, 2021

Conversation

lolgab
Copy link
Contributor

@lolgab lolgab commented May 16, 2021 •
edited
Loading

@lolgab lolgab marked this pull request as ready for review May 16, 2021 17:22
@rossabaker
Copy link
Member

Thanks for this.

I'm not sure whether any of the other Jawn maintainers use Scala Native, and I'm a little worried about adding build complexity in a key project. At the same time, without doing so, we can't begin to support downstream projects in Scala Native.

It doesn't complicate the code. I'm willing to give it a shot, as long as we get help if it breaks or can throw it overboard if it blocks the JVM use case.

@lolgab
Copy link
Contributor Author

lolgab commented May 17, 2021

@rossabaker Thank you!
I added Scala Native because I was trying to port Circe which (uses it on the JVM) is central to any typelevel project and key to Scala Native usefulness/adoption.
I'm happy to help in case this addition creates problems in the future.

@rossabaker
Copy link
Member

Sounds good to me. Let's try it.

Looks like we broke the Scala 3 build somehow. I'm confused, because it says it's the JVM project, but I see native things happening.

@lolgab
Copy link
Contributor Author

lolgab commented May 17, 2021

Something was going on with making available the dummy Claim.scala implementation for dotty.
I moved the code to src/test/scala-3 which is automatically detected by Sbt, so it works with no special treatment.

@lolgab
Copy link
Contributor Author

lolgab commented May 17, 2021

I made a silly mistake which made all the tests be skipped on Scala Native. The scalacheck dependency needs the %%% operator to be passed as a Scala Native dependency.
This reminded me that I need typelevel/claimant#132 and even after that there is some missing java.file api that needs to be added upstream on Scala Native.
I'm changing this PR to draft until I get all the requirements done.
I'll ask another review when I'm done, sorry for wasting your time.

@rossabaker
Copy link
Member

No worries. I approved the CI run, to see if it uncovers any more surprises.

@lolgab lolgab marked this pull request as draft May 17, 2021 21:15
@larsrh
Copy link
Contributor

larsrh commented May 30, 2021

Claimant 0.2.0 on its way to Maven Central.

@larsrh
Copy link
Contributor

larsrh commented May 30, 2021

After giving it a bit of thought I'm going to deprecate Claimant (typelevel/claimant#110). We should remove the dependency here.

@rossabaker
Copy link
Member

Would we then port this to expecty, or just drop the fancy assertions?

@larsrh
Copy link
Contributor

larsrh commented Jun 1, 2021

I vote drop fancy assertions. Jawn is stable; for all I care the tests just say "yes" or "no" 😉

@larsrh larsrh mentioned this pull request Jun 3, 2021
Merged
@larsrh
Copy link
Contributor

larsrh commented Jun 5, 2021

#348 is merged so we don't need the claimant stuff anymore

@armanbilge armanbilge mentioned this pull request Jul 6, 2021
Merged
@lolgab
Copy link
Contributor Author

lolgab commented Oct 15, 2021 •
edited
Loading

I rebased the branch on top of the current mainbranch.
About platform-specific files, I copy-pasted the Scala.js files. So I don't even depend on the missing Channel not implemented function in Scala Native 0.4.0.

I can, alternatively, put them in a js-native source folder if you don't like the copy.

Let me know.

@lolgab lolgab marked this pull request as ready for review October 16, 2021 09:31
armanbilge
armanbilge reviewed Oct 19, 2021
View reviewed changes
build.sbt
Comment on lines +83 to +85
lazy val jawnSettingsNative = Seq(
crossScalaVersions := crossScalaVersions.value.filterNot(ScalaArtifacts.isScala3)
)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How does this interact with the ++3.0.0 used in CI?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It seems to me that runs only the tests in JVM and JS projects since the native project doesn't have that version.
If you run:

> ++3.0.0
> astNative/test

It runs the 2.12 tests instead.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmm. It's probably fine for tests, but what happens with +publish. Does it try to publish 2.12 artifacts for native twice?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I tried +publishLocal and it seems fine actually. Go figure?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, I wasn't able to reproduce the problems that a platform with less scala versions create..

@lolgab
Copy link
Contributor Author

lolgab commented Oct 20, 2021

@rossabaker Can you approve the CI run again?

@lolgab
Copy link
Contributor Author

lolgab commented Oct 21, 2021

@rossabaker Thanks for the approval!
I forgot to disable MiMa for Scala Native as it is in Scala.js. Now I added the setting to jawnJSNativeSettings.
Hopefully, this will be the time when it passes 🤞

Sorry to disturb you again :/

@rossabaker
Copy link
Member

No worries. Bummer we're on different time zones. 😄

@lolgab
Copy link
Contributor Author

lolgab commented Oct 21, 2021

CI is green now 🥳

@lolgab
Copy link
Contributor Author

lolgab commented Oct 30, 2021

@rossabaker Can this be merged now? :)

@armanbilge
Copy link
Member

@lolgab your goal is to use this to cross circe for native, correct? Or do you see stand-alone uses?

@lolgab
Copy link
Contributor Author

lolgab commented Oct 30, 2021

@armanbilge
It is for Circe, yes, why?

@armanbilge
Copy link
Member

Do you know if Circe is open to a native cross rn? I guess it would depend on the build complexity, but lately seems to be trying to reduce the crosses: circe/circe#1818

It should be possible to cross Circe core without this, I think, to test the waters there maybe :)

@lolgab
Copy link
Contributor Author

lolgab commented Oct 30, 2021

@armanbilge You're right, I need to ask there first.
About porting Circe without jawn I'm not sure. The JS version uses native javascript parsing capabilities while the JVM version uses jawn, from what I saw.

@armanbilge
Copy link
Member

The circe-core module just defines Json, Encoder, Decoder, etc. but doesn't do any parsing, the circe-parser module is the one you are thinking of I think.

We all love Jawn but it's wholly possible that plugging circe-core with some non-Scala native lib for JSON parsing would be much more performant 😆

@armanbilge
Copy link
Member

Btw, I'm generally for this, esp. since Jawn is needed for http4s-native (watch out Ross!) regardless of what circe uses for native parsing ... but we are still quite a ways off from that.

rossabaker
rossabaker approved these changes Nov 14, 2021
View reviewed changes
Copy link
Member

@rossabaker rossabaker left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't know anything about scala-native, but it looks like this passes without unduly complicating the other builds. What is the compatibility story? When scala-native-0.5 is released, will another jawn release be required? Would it be okay to handle that in a minor jawn bump?

Like I said in the spring, I want to support this without putting the predominant platform at any extra risk, but it looks like you've accomplished it!

@armanbilge
Copy link
Member

When scala-native-0.5 is released, will another jawn release be required? Would it be okay to handle that in a minor jawn bump?

The artifact suffix will change (see how cats-native is published: https://repo1.maven.org/maven2/org/typelevel/cats-core_native0.4_2.12/) so wouldn't need anything special with jawn versioning: introduce a new cross-build for it (and most likely drop the old one).

@rossabaker
Copy link
Member

Okay, we have two people on this thread who know what they're doing. We'll keep it if you can keep it running or recruit more people who can. 😄

@rossabaker rossabaker merged commit 7684202 into typelevel:main Nov 14, 2021
@lolgab lolgab deleted the support-scala-native branch November 14, 2021 08:29
@sideeffffect sideeffffect mentioned this pull request Nov 16, 2021
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@armanbilge armanbilge armanbilge left review comments

@rossabaker rossabaker rossabaker approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@lolgab @rossabaker @larsrh @armanbilge