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

tools and codegen-sbt compiles in scala3 making compile-time plugin available #1415

Merged
merged 8 commits into from
Jul 30, 2022

Conversation

pmeheut
Copy link
Contributor

@pmeheut pmeheut commented Jul 23, 2022

This is a solution to #1239 and a Caliban 2 version of PR #1248.
The tests compile. I've also used it in a real project and it works for its scope.

Except for the Scala 3 changes, the problem was that the plugin generates the cached files in streams, without the usual scala version based directory structure.
It messed with +compile.
So I changed that.

In tools, a new problem is that the current zio.config.magnolia scala 3 implementation has a very different interface. So I had to add files specific to 2.12, 2.13 and 3.

Copy link
Owner

@ghostdogpr ghostdogpr left a comment

Choose a reason for hiding this comment

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

Can you run fmt to format the code and make the CI go to the next steps?
Also can you resolve the merge conflicts?

@ghostdogpr
Copy link
Owner

I guess we also need to modify .circleci/config.yml to make sure the scripted tests are working with scala 2.13 and 3?

@pmeheut
Copy link
Contributor Author

pmeheut commented Jul 23, 2022

About .circleci/config.yml, I guess so but I'm not familiar with this part.

@ghostdogpr
Copy link
Owner

About .circleci/config.yml, I guess so but I'm not familiar with this part.

So basically here we create a build step that runs sbt ++2.12.16 codegenSbt/scripted, which runs the scripted tests under scala 2.12. We probably don't need 2.13 but we need something similar for scala 3.

@pmeheut
Copy link
Contributor Author

pmeheut commented Jul 23, 2022

Ok. It does not work even for 2.13.8 right now because the test script is written with 2.12 hard dependencies. I'll have a look at it but because I'm gonna need to understand it first, it may take a few days (or months if my real work catches me up).

@pmeheut
Copy link
Contributor Author

pmeheut commented Jul 25, 2022

It can be done and I've been able to pass the test manually but it is not simple as far as I can tell. We have several problems:

  1. the scripted test file contains reference to the scalaVersion. So we would need to duplicate each entry for 3.1.3 and change them each time with upgrade the scala 3 compiler version because this file cannot be parameterized
  2. when running with ++3.1.3, we do not generate the 2.12 jar nor the plugin we need as far as I can tell. So sbt ends with an error

To run it, I need to fix the version in the main build.sbt file, (i.e. version := "sbt-test-SNAPSHOT"), build and publish the jars for 2.12 and then I can run the scripted test for 3.1.3 after editing the test file

The process can be automated and changing temporarily build.sbt and test files during a CI test should be ok.

This leads to two questions:

  • is it worth it? The scala3 code is the basically the same as the scala212 code. So it would be as efficient to test only the difference, i.e. DescriptorUtils.scala
  • is there a better, simpler solution, something I missed?

@ghostdogpr
Copy link
Owner

Hmm, maybe a simpler alternative for now could be just to add tools/test to the test3_jdk CI step to run tests on scala 3, and add a test that covers DescriptorUtils if it's not already there. It wouldn't test the whole thing but that's acceptable to me.

@pmeheut
Copy link
Contributor Author

pmeheut commented Jul 25, 2022

Ok, I'll write such tests based on the use we make of it in `Options.scala``.

@guizmaii
Copy link
Contributor

guizmaii commented Jul 25, 2022

Guys, I was successfully running the scripted tests on all the Scala versions thanks to this change in the CI: https://github.com/ghostdogpr/caliban/pull/1264/files#diff-78a8a19706dbd2a4425dd72bdab0502ed7a2cef16365ab7030a5a0588927bf47R53-R56

IIRC, The issue is that when we're publishing Caliban locally, it generates 3 different version numbers: 1 per version of Scala. The fix is to force the version in the build.sbt before to publish locally and to use this forced version number in the test projects

(Sorry if my response is a bit out of the blue, I only quickly read your conversation. Too quickly maybe)

@pmeheut
Copy link
Contributor Author

pmeheut commented Jul 25, 2022

The fix is to force the version in the build.sbt before to publish locally and to use this forced version number in the test projects
Yes, this is what I've done to test the scala3 modifications and what I suggested above. I'll see if I can implement it without spending to much time on it because if we go this route, we also need to edit the testfile from inside sbt to replace 2.12 by the current scalaVersion value.

@ghostdogpr
Copy link
Owner

Since this is very useful and I will do a release, I will merge the PR already. With existing tests we know it doesn't break 2.x support, so it can only be better than before 😄 Feel free to add tests later if you have time.

@ghostdogpr ghostdogpr merged commit f3b9210 into ghostdogpr:series/2.x Jul 30, 2022
ghostdogpr added a commit that referenced this pull request Aug 1, 2022
* tools and codegen-sbt now support scala3

* Moved sttp version to 3.7.1

* Merged scala-2.12 & 2.13 into scala-2, removed generated files from git added by mistake

* fmt

* Ran fmt on DescriptorUtils.scala in scala-3

* tools and codegen-sbt now support scala3

* Merged scala-2.12 & 2.13 into scala-2, removed generated files from git added by mistake

* fmt

* Now using scala 2.13.8

* now using the correct version, i.e 2.13 instead of 2.13.8 in test

* testing back with version 2.12, not working

* Preparing for merge

* Now reading scala3 from crossScalaVersions, removed the useless setting used to do so before

* Almost ready for push and PR

* scalaFmt

* Added tests for scala3 codegen sbt compile time plugins

* Removed scripted3_jdk filter

Co-authored-by: Pierre Ricadat <[email protected]>
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.

3 participants