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

Update dependencies, add Scala Native support for circe integration, add jsoniter-scala integration #7

Merged
merged 23 commits into from
Sep 30, 2022

Conversation

plokhotnyuk
Copy link
Contributor

@plokhotnyuk plokhotnyuk commented Sep 19, 2022

Close #6 by adding jsoniter-scala module

@ybasket @satabin Can I use code from this PR in a tutorial for jsoniter-scala?

@ybasket
Copy link
Collaborator

ybasket commented Sep 19, 2022

@plokhotnyuk thank you for contributing! I unfortunately don't have the time right now to dive deep into how the codecs work, but it looks good at first glance and I'll try to find some time soon-ish if @satabin isn't faster.

As for using the code in a tutorial, no objections from my side, looking forward to read it. A link back to geo-scala would of course be appreciated :)

@plokhotnyuk
Copy link
Contributor Author

plokhotnyuk commented Sep 19, 2022

@ybasket Please approve the workflow run, I hope the latest commit will fix CI build failures

@ybasket
Copy link
Collaborator

ybasket commented Sep 19, 2022

@ybasket Please approve the workflow run, I hope the latest commit will fix CI build failures

Done. Sorry, we probably should have updated after the last release (still learning the details of sbt-typelevel).

Edit: Seems as it wants x.y, just changed that 🤞

build.sbt Outdated Show resolved Hide resolved
@plokhotnyuk
Copy link
Contributor Author

plokhotnyuk commented Sep 19, 2022

@ybasket Thanks for the help! Please approve the workflow run again, I hope it will show the green mark finally.

@plokhotnyuk
Copy link
Contributor Author

Ups, it failed again... Now looking a way how to generate CI scripts with Java 11 for the jsoniterScala module...

build.sbt Outdated Show resolved Hide resolved
build.sbt Outdated Show resolved Hide resolved
build.sbt Outdated Show resolved Hide resolved
@armanbilge
Copy link

A workaround to build Java 8 bytecode using Java 11

Just to clarify—it not only builds Java 8 bytecode, it also restricts compilation to use only JDK 8 APIs and nothing newer.

@plokhotnyuk
Copy link
Contributor Author

@armanbilge Thank you a lot! It looks like a clear solution (not a workaround) isn't it?

@armanbilge
Copy link

Yes, I would consider it to be a solution. In fact there is a proposal to make a configuration like this the default for the next major release of sbt-typelevel.

@plokhotnyuk
Copy link
Contributor Author

Thank you, @ybasket! I missed to run the githubWorkflowGenerate task again

@armanbilge
Copy link

Btw, I think this can be removed:

geo-scala/build.sbt

Lines 30 to 32 in 876e75c

// disable MiMa until we have proper version released on gnieh / all platforms
// once removed, the tlBaseVersion has to adjusted as well
mimaPreviousArtifacts := Set.empty

@armanbilge
Copy link

Note: you should never set mimaPreviousArtifacts := Set.empty. This is wrong. You always fall into one of two scenarios:

  1. You are not publishing that module. So .enablePlugins(NoPublishPlugin).
  2. It is a brand new artifact, not yet published. So set
  tlVersionIntroduced := Map("2.12" -> "1.2.3", "2.13" -> "1.2.3", "3" -> "1.2.3")

Assuming that 1.2.3 is the first version it will be published in.

@plokhotnyuk
Copy link
Contributor Author

plokhotnyuk commented Sep 20, 2022

@ybasket @satabin Should I fix "issues" reported by Codacy Static Code Analysis?

Some of them like in README.md are not fixable, others will lead to less efficient or less readable and more error prone code...

@satabin
Copy link
Member

satabin commented Sep 20, 2022

Hi @plokhotnyuk You can disregard Codacy, their profile is not tuned yet for our usage.

@plokhotnyuk
Copy link
Contributor Author

@ybasket @satabin This PR is ready for final review.

I propose make other improvements for binary compatibility checks and replacement of List by ArraySeq in subsequent PRs

@armanbilge
Copy link

I propose make other improvements for binary compatibility checks

Just to clarify, the "improvement" is deleting one line :)

geo-scala/build.sbt

Lines 30 to 32 in 876e75c

// disable MiMa until we have proper version released on gnieh / all platforms
// once removed, the tlBaseVersion has to adjusted as well
mimaPreviousArtifacts := Set.empty

@plokhotnyuk
Copy link
Contributor Author

@armanbilge I have tried sbt +mimaReportBinaryIssues without mimaPreviousArtifacts := Set.empty line and it doesn't work.

All modules are still unchecked due to empty mimaPreviousArtifacts.

We should set it explicitly, isn't it?

@satabin
Copy link
Member

satabin commented Sep 28, 2022

Is the base version correctly set?

@armanbilge
Copy link

@plokhotnyuk yes, that is the correct, expected behavior. Because you have bumped the base version from 0.2 -> 0.3 you have signified that you are making a breaking change, and therefore sbt-typelevel stops checking against previous versions.

You should not update the base version if you want to continue checking against previous versions.

I highly recommend you read the material that I linked you to before :) it explains this in detail, with examples.

https://typelevel.org/sbt-typelevel/faq.html#how-do-i-introduce-breaking-changes-intended-for-my-next-version
https://typelevel.org/sbt-typelevel/faq.html#what-is-a-base-version-anyway

@plokhotnyuk
Copy link
Contributor Author

@armanbilge I don't understand how all the magic should work... Locally when I revert the base version back to 0.2 it doesn't validate any module.

@armanbilge
Copy link

Have you pulled the tags to your local clone? Otherwise it will be unable to detect previous versions locally (but tags are always present in CI).

@armanbilge
Copy link

Btw, I see that v0.3.0 is already released. So now I am confused what is happening in your PR.
https://github.com/gnieh/geo-scala/releases/tag/v0.3.0

@armanbilge
Copy link

armanbilge commented Sep 28, 2022

For the record: if I leave the tlBaseVersion := "0.3" and I remove the mimaPreviousArtifacts := Set.empty, then it is all configured correctly.

sbt:geo-scala> show mimaPreviousArtifacts
[info] jsoniterScalaJVM / mimaPreviousArtifacts
[info]  Set(org.gnieh:geo-scala-jsoniter-scala:0.3.0 (e:info.versionScheme=early-semver))
[info] coreJS / mimaPreviousArtifacts
[info]  Set(org.gnieh:geo-scala-core:0.3.0 (e:info.versionScheme=early-semver))
[info] circeNative / mimaPreviousArtifacts
[info]  Set(org.gnieh:geo-scala-circe:0.3.0 (e:info.versionScheme=early-semver))
[info] jsoniterScalaJS / mimaPreviousArtifacts
[info]  Set(org.gnieh:geo-scala-jsoniter-scala:0.3.0 (e:info.versionScheme=early-semver))
[info] jsoniterScalaNative / mimaPreviousArtifacts
[info]  Set(org.gnieh:geo-scala-jsoniter-scala:0.3.0 (e:info.versionScheme=early-semver))
[info] circeJVM / mimaPreviousArtifacts
[info]  Set(org.gnieh:geo-scala-circe:0.3.0 (e:info.versionScheme=early-semver))
[info] coreNative / mimaPreviousArtifacts
[info]  Set(org.gnieh:geo-scala-core:0.3.0 (e:info.versionScheme=early-semver))
[info] polylineJVM / mimaPreviousArtifacts
[info]  Set(org.gnieh:geo-scala-polyline:0.3.0 (e:info.versionScheme=early-semver))
[info] circeJS / mimaPreviousArtifacts
[info]  Set(org.gnieh:geo-scala-circe:0.3.0 (e:info.versionScheme=early-semver))
[info] polylineJS / mimaPreviousArtifacts
[info]  Set(org.gnieh:geo-scala-polyline:0.3.0 (e:info.versionScheme=early-semver))
[info] coreJVM / mimaPreviousArtifacts
[info]  Set(org.gnieh:geo-scala-core:0.3.0 (e:info.versionScheme=early-semver))
[info] polylineNative / mimaPreviousArtifacts
[info]  Set(org.gnieh:geo-scala-polyline:0.3.0 (e:info.versionScheme=early-semver))
[info] mimaPreviousArtifacts
[info]  Set()
sbt:geo-scala> 

@plokhotnyuk
Copy link
Contributor Author

plokhotnyuk commented Sep 28, 2022

It doesn't work for me... Have I missed to run some sbt task?

Details
andriy@notebook:~/Projects/com/github/plokhotnyuk/geo-scala$ sbt
[info] welcome to sbt 1.7.1 (Azul Systems, Inc. Java 17.0.2)
[info] loading global plugins from /home/andriy/.sbt/1.0/plugins
[info] loading settings for project geo-scala-build from plugins.sbt ...
[info] loading project definition from /home/andriy/Projects/com/github/plokhotnyuk/geo-scala/project
[info] loading settings for project root from build.sbt,publish.sbt ...
[info] resolving key references (15836 settings) ...
[info] set scmInfo to https://github.com/plokhotnyuk/geo-scala
[info] set current project to geo-scala (in build file:/home/andriy/Projects/com/github/plokhotnyuk/geo-scala/)
[info] sbt server started at local:///home/andriy/.sbt/1.0/server/0eabc375e6e88395df69/sock
[info] started sbt server
sbt:geo-scala> show mimaPreviousArtifacts
[info] coreJVM / mimaPreviousArtifacts
[info] 	Set()
[info] jsoniterScalaJVM / mimaPreviousArtifacts
[info] 	Set()
[info] circeJS / mimaPreviousArtifacts
[info] 	Set()
[info] coreJS / mimaPreviousArtifacts
[info] 	Set()
[info] polylineNative / mimaPreviousArtifacts
[info] 	Set()
[info] polylineJS / mimaPreviousArtifacts
[info] 	Set()
[info] circeNative / mimaPreviousArtifacts
[info] 	Set()
[info] jsoniterScalaJS / mimaPreviousArtifacts
[info] 	Set()
[info] circeJVM / mimaPreviousArtifacts
[info] 	Set()
[info] jsoniterScalaNative / mimaPreviousArtifacts
[info] 	Set()
[info] polylineJVM / mimaPreviousArtifacts
[info] 	Set()
[info] coreNative / mimaPreviousArtifacts
[info] 	Set()
[info] mimaPreviousArtifacts
[info] 	Set()
sbt:geo-scala> 
[info] shutting down sbt server
andriy@notebook:~/Projects/com/github/plokhotnyuk/geo-scala$ git diff
diff --git a/build.sbt b/build.sbt
index 59a5aa7..0432a9c 100644
--- a/build.sbt
+++ b/build.sbt
@@ -26,10 +26,7 @@ lazy val commonSettings = Seq(
     "org.scalatest" %%% "scalatest" % "3.2.13" % Test,
     "org.scalatestplus" %%% "scalacheck-1-16" % "3.2.13.0" % Test,
     "org.scalacheck" %%% "scalacheck" % "1.16.0" % Test
-  ),
-  // disable MiMa until we have proper version released on gnieh / all platforms
-  // once removed, the tlBaseVersion has to adjusted as well
-  mimaPreviousArtifacts := Set.empty
+  )
 )
 
 lazy val core = crossProject(JVMPlatform, JSPlatform, NativePlatform)

@armanbilge
Copy link

@plokhotnyuk what is the output of git tag on your system?

@plokhotnyuk
Copy link
Contributor Author

plokhotnyuk commented Sep 28, 2022

@armanbilge Thanks for the tip! There were no tags in the local clone of my fork until I fetched them from the upstream.

Details
andriy@notebook:~/Projects/com/github/plokhotnyuk/geo-scala$ git tag
andriy@notebook:~/Projects/com/github/plokhotnyuk/geo-scala$ git remote add upstream https://github.com/gnieh/geo-scala
andriy@notebook:~/Projects/com/github/plokhotnyuk/geo-scala$ git remote -v
origin	git@github.com:plokhotnyuk/geo-scala.git (fetch)
origin	git@github.com:plokhotnyuk/geo-scala.git (push)
upstream	https://github.com/gnieh/geo-scala (fetch)
upstream	https://github.com/gnieh/geo-scala (push)
andriy@notebook:~/Projects/com/github/plokhotnyuk/geo-scala$ git fetch upstream
remote: Enumerating objects: 51, done.
remote: Counting objects: 100% (51/51), done.
remote: Compressing objects: 100% (9/9), done.
remote: Total 36 (delta 9), reused 35 (delta 9), pack-reused 0
Unpacking objects: 100% (36/36), 5.09 KiB | 473.00 KiB/s, done.
From https://github.com/gnieh/geo-scala
* [new branch]      feature/refresh-versions             -> upstream/feature/refresh-versions
* [new branch]      fix-polyline-on-small-negative-delta -> upstream/fix-polyline-on-small-negative-delta
* [new branch]      geohash                              -> upstream/geohash
* [new branch]      main                                 -> upstream/main
* [new branch]      switch-to-sbt-typelevel              -> upstream/switch-to-sbt-typelevel
* [new branch]      upgrade-to-2.13                      -> upstream/upgrade-to-2.13
* [new tag]         v0.1.0                               -> v0.1.0
* [new tag]         v0.1.0-2023c94f                      -> v0.1.0-2023c94f
* [new tag]         v0.1.0-907d7c6d                      -> v0.1.0-907d7c6d
* [new tag]         v0.1.0-fb77b3a3                      -> v0.1.0-fb77b3a3
* [new tag]         v0.1.1                               -> v0.1.1
* [new tag]         v0.1.2                               -> v0.1.2
* [new tag]         v0.2.0                               -> v0.2.0
* [new tag]         v0.3.0                               -> v0.3.0
andriy@notebook:~/Projects/com/github/plokhotnyuk/geo-scala$ git tag
v0.1.0
v0.1.0-2023c94f
v0.1.0-907d7c6d
v0.1.0-fb77b3a3
v0.1.1
v0.1.2
v0.2.0
v0.3.0

@plokhotnyuk
Copy link
Contributor Author

plokhotnyuk commented Sep 28, 2022

I bumped the base version to 0.4 due to changes in the circe module that replaced io.circe.Encoder#AsObject by io.circe.Encoder in some pubic methods which were required for passing added serialization assertions in tests.

@plokhotnyuk
Copy link
Contributor Author

@ybasket @satabin Please approve workflow running.

@ybasket
Copy link
Collaborator

ybasket commented Sep 30, 2022

scala/scala#9982 produces some warnings while compiling that are then turned into errors 😢

I downgraded to Scala 2.13.8 to avoid this. I didn't see anything in the release notes of 2.13.9 we would profit from…

@ybasket
Copy link
Collaborator

ybasket commented Sep 30, 2022

@plokhotnyuk thank you for contributing this and your patience with the CI 👍. Would be interested to read that blog article, maybe you throw a link here once ready?

And also thank you 👍, @armanbilge!

I'll release as 0.4.0.

@ybasket ybasket merged commit 1592087 into gnieh:main Sep 30, 2022
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.

Use jsoniter-scala instead of circe
4 participants