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

Apply ZIO SBT Ecosystem Plugin #1091

Open
wants to merge 60 commits into
base: series/4.x
Choose a base branch
from

Conversation

khajavi
Copy link
Member

@khajavi khajavi commented Feb 28, 2023

No description provided.

@khajavi khajavi force-pushed the apply-zio-ecosystem-plugin branch from 757e449 to 96facd1 Compare March 1, 2023 18:25
@khajavi khajavi marked this pull request as ready for review March 4, 2023 15:46
@khajavi khajavi requested a review from a team as a code owner March 4, 2023 15:46
@khajavi
Copy link
Member Author

khajavi commented Mar 4, 2023

This work is ready for review.

The failing CI seems related to this issue #1095

@afsalthaj
Copy link
Collaborator

Thanks Milad. Have been waiting for this plugin and great addition :-)

@@ -1,89 +1,215 @@
name: CI
# This file was autogenerated using `zio-sbt-ci` plugin via `sbt generateGithubWorkflow`
# task and should be included in the git repository. Please do not edit it manually.
Copy link
Collaborator

Choose a reason for hiding this comment

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

👍

.scalafmt.conf Outdated
"core/src/test/scala/zio/config/ProductBuilderTest.scala"
"core/src/test/scala/zio/config/ProductBuilderTest.scala",
"core/shared/src/main/scala-3.x/*",
"magnolia/shared/src/main/scala-dotty/*"
Copy link
Collaborator

Choose a reason for hiding this comment

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

I think this can also go into a custom module within ecosystem plugin that wraps scalafmt
And for custom settings, such as exclude, I could simply configure with in each module setting, excludeFmt

Copy link
Member Author

Choose a reason for hiding this comment

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

Thank you! Added to the backlog.

build.sbt Outdated
developers := List(
name := "ZIO Config",
crossScalaVersions -= scala211.value,
ciEnabledBranches := Seq("series/4.x"),
Copy link
Collaborator

Choose a reason for hiding this comment

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

I think we should include master as well, since master works with latest zio

Copy link
Member Author

Choose a reason for hiding this comment

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

What about series/2.x? Doesn't it work for the latest version of ZIO?

Copy link
Collaborator

Choose a reason for hiding this comment

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

I don't think we need to support 2.x going forward

Copy link
Member Author

Choose a reason for hiding this comment

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

Done!

licenses := List("Apache-2.0" -> url("http://www.apache.org/licenses/LICENSE-2.0")),
developers := List(
name := "ZIO Config",
crossScalaVersions -= scala211.value,
Copy link
Collaborator

Choose a reason for hiding this comment

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

I would rather just rely on ecosystem plugin to decide the common scala versions that an ecosystem library should support. Obviously, ecosystem libraries can choose to exclude from this common list. This implies there will not be specific inThisBuild settings for crossScalaVersions. The exclusion, should be within each module.
Ecosystem Plugin can also choose to pre-built excluded list.

Copy link
Member Author

Choose a reason for hiding this comment

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

Currently, zio-sbt provides default scala versions, then we can add/remove and customize that for the whole modules or any submodule.
I'm not sure I got what you said, here I excluded scala 211 from all modules, although we can do that one by one.

build.sbt Outdated
)
)

addCommandAlias("fmt", "; scalafmtSbt; scalafmt; test:scalafmt")
addCommandAlias("fix", "; all compile:scalafix test:scalafix; all scalafmtSbt scalafmtAll")
addCommandAlias("compileAll", "; ++2.12.16; root2-12/compile; ++2.13.8!; root2-13/compile")
Copy link
Collaborator

Choose a reason for hiding this comment

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

I would delete this as well. CommandAlias can be a pain. I would either choose it as sbtSettings that are not based on String. Even better is within ecosystem plugin

Copy link
Member Author

Choose a reason for hiding this comment

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

Done!

build.sbt Outdated
)
)

addCommandAlias("fmt", "; scalafmtSbt; scalafmt; test:scalafmt")
addCommandAlias("fix", "; all compile:scalafix test:scalafix; all scalafmtSbt scalafmtAll")
addCommandAlias("compileAll", "; ++2.12.16; root2-12/compile; ++2.13.8!; root2-13/compile")
addCommandAlias("testAll", "; ++2.12.16; root2-12/test; ++2.13.8!; root2-13/test")
Copy link
Collaborator

Choose a reason for hiding this comment

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

I would rather choose the same Same as above

Copy link
Member Author

Choose a reason for hiding this comment

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

Done!

build.sbt Outdated
@@ -65,13 +67,14 @@ lazy val magnoliaDependencies =

lazy val refinedDependencies =
libraryDependencies ++= {
if (scalaBinaryVersion.value == "2.11") Seq.empty // Just to make IntelliJ happy
if (scalaVersion.value == scala211.value) Seq.empty // Just to make IntelliJ happy
Copy link
Collaborator

Choose a reason for hiding this comment

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

This can happen several times. One setting can be addDependency which takes simply takes a list of scala versions to be excluded. Otherwise, different projects may have different ways to handle dependency settings.

Also refinedDependencies is an unnecessary redirection, that it could be inlined as well

Copy link
Member Author

Choose a reason for hiding this comment

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

That's a good idea, but it is the first time I saw such a case. I'm doing the same for other ecosystem projects. If I saw that the common case, I'll make a general utility inside zio-sbt.

Copy link
Member Author

Choose a reason for hiding this comment

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

Just wrote these utility functions:

def addDependenciesFor(scalaBinaryVersions: String*)(dependencies: ModuleID*) =
  libraryDependencies ++= {
    if (scalaBinaryVersions.contains(scalaBinaryVersion.value)) dependencies else Seq.empty
  }

def addDependenciesExceptFor(scalaBinaryVersions: String*)(dependencies: ModuleID*) =
  libraryDependencies ++= {
    if (scalaBinaryVersions.contains(scalaBinaryVersion.value)) Seq.empty else dependencies
  }

def addScalacOptionsFor(scalaBinaryVersions: String*)(options: String*) =
  scalacOptions ++= {
    if (scalaBinaryVersions.contains(scalaBinaryVersion.value)) options else Seq.empty
  }

def addScalacOptionsExceptFor(scalaBinaryVersions: String*)(options: String*) =
  scalacOptions ++= {
    if (scalaBinaryVersions.contains(scalaBinaryVersion.value)) Seq.empty else options
  }

),
testFrameworks := Seq(new TestFramework("zio.test.sbt.ZTestFramework"))
"com.amazonaws" % "aws-java-sdk-ssm" % awsVersion
)
Copy link
Collaborator

@afsalthaj afsalthaj Mar 5, 2023

Choose a reason for hiding this comment

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

Much better for any new contributor, is to be able to say

lazy val zioConfigAws = jvmModule("aws").withDependencies().dependOn(zioConfig)

As mentioned above withDependencies can take care of weird cases, such as excluding if Scala-version is x etc.

The ecosystem plugin mandates the name of the root project (say zio-config), and any module definition simply append the file path name. This way every project becomes a standard

Copy link
Member Author

Choose a reason for hiding this comment

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

I agree, will work on this!

.settings(stdSettings("zio-config-scalaz"))
.settings(crossProjectSettings)
.settings(stdSettings(name = "zio-config-scalaz", enableCrossProject = true))
.settings(enableZIO())
Copy link
Collaborator

Choose a reason for hiding this comment

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

I think its mostly to help defining modules easily without relying on adhoc sbt settings. As mentioned above, as a maintainer I am far relieved if I can just simply define module(...) using the plugin, or if needed jvmModule, jsModule etc.

@afsalthaj
Copy link
Collaborator

afsalthaj commented Mar 5, 2023

I have put some comments, but all of it boils down to 1 suggestion, that being able to define module based on the plugin,

lazy val myModule = 
   module("aws")
    .withDependencies("dev.zio" %%% "izumi-reflect" % "2.2.2")
    .dependsOn(core)

Adding customisations becomes even simpler

lazy val myModule = 
   module("aws")
    .skipScala211
    .withDependencies("dev.zio" %%% "izumi-reflect" % "2.2.2")
    .dependsOn(core)

This is much easier for a maintainer. Example: In the above case, core can support 211, but aws cannot, and there is no need of an indirection such as having a big Map of each module and scala versions supported

@khajavi
Copy link
Member Author

khajavi commented Mar 5, 2023

@afsalthaj
That is an excellent DSL for writing builds without any boilerplate. I agree with such language. The first iteration aimed to factor out BuildHelper and make it maintainable.
I am excited to work on implementing your suggestions for the next phase.

@khajavi
Copy link
Member Author

khajavi commented Mar 5, 2023

In the above case, core can support 211, but aws cannot, and there is no need of an indirection such as having a big Map of each module and scala versions supported

The name of supportedScalaVersions is somehow confusing, I will rename it to something like ciScalaVersionMatrix. We just use that to create the test matrix. This is just a workaround because I lack deep knowledge of how to iterate through sbt submodules and create matrix map pragmatically.

So ideally I would like to have something like this:

val ciScalaVersionsMatrix: Map[String, Seq[String]] = 
  listOfAllModules.map { module =>
      (module / thisProject).value.id   -> (module / crossScalaVersions).value
  }

But it will not compile due to complicated macro-based sbt internals. We need to find sbt specific solution to traverse modules and pull out settings.

@khajavi khajavi force-pushed the apply-zio-ecosystem-plugin branch from 354e474 to 2ffa644 Compare March 7, 2023 11:21
@khajavi
Copy link
Member Author

khajavi commented Mar 9, 2023

Thrilled to see the geeeen color 😅
@afsalthaj Can you please have a look?

Error message:
[error] -- Error: /home/runner/work/zio-config/zio-config/magnolia/shared/src/test/scala-dotty/zio/config/magnolia/MarkdownSpec.scala:14:44
[error]  14 |        generateDocs(deriveConfig[RawConfig]).toTable.toGithubFlavouredMarkdown
[error]     |                                            ^
[error]     |   given instance derived is declared as `inline`, but was not inlined
[error]     |
[error]     |   Try increasing `-Xmax-inlines` above 32
To reproduce the following error, run `sbt zioConfigRefinedJVM/Test/compile`
[error] -- [E008] Not Found Error: /home/runner/work/zio-config/zio-config/refined/shared/src/test/scala/zio/config/refined/NumericSupportTest.scala:3:26
[error] 3 |import eu.timepit.refined.W
[error]   |                          ^
[error]   |                          value W is not a member of eu.timepit.refined
@khajavi khajavi requested a review from afsalthaj March 14, 2023 11:26
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