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

mill codegen dependancies are "provided" #1552

Open
wants to merge 1 commit into
base: series/0.18
Choose a base branch
from
+27 −16
Open

mill codegen dependancies are "provided" #1552

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
43 changes: 27 additions & 16 deletions build.sbt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -388,6 +388,25 @@ lazy val `aws-http4s` = projectMatrix
.jsPlatform(latest2ScalaVersions, jsDimSettings)
.nativePlatform(allNativeScalaVersions, nativeDimSettings)

// Seperated out to help the mill codegen plugin not require "Provided" dependencies in test
val codegenDeps = Def.setting {
Seq(
Dependencies.Cats.core.value,
Dependencies.Smithy.model,
Dependencies.Smithy.build,
Dependencies.Alloy.core,
Dependencies.Alloy.openapi,
Dependencies.Smithytranslate.proto,
"com.lihaoyi" %% "os-lib" % "0.9.3",
Dependencies.Circe.core.value,
Dependencies.Circe.parser.value,
Dependencies.Circe.generic.value,
Dependencies.collectionsCompat.value,
"org.scala-lang" % "scala-reflect" % scalaVersion.value,
"io.get-coursier" %% "coursier" % "2.1.9"
)
}

/**
* This module contains the logic used at build time for reading smithy
* models and rendering Scala (or openapi) code.
Expand All @@ -407,21 +426,7 @@ lazy val codegen = projectMatrix
"alloyVersion" -> Dependencies.Alloy.alloyVersion
),
buildInfoPackage := "smithy4s.codegen",
libraryDependencies ++= Seq(
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm not sure I understand the rationale behind extracting this. In theory, you shouldn't have to.

Copy link
Author

Choose a reason for hiding this comment

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

I couldn't figure out the right sbt incantation to reference the dependancies of the module directly, without extracting them into their own setting.

This could simply be weak sbt-foo .

Dependencies.Cats.core.value,
Dependencies.Smithy.model,
Dependencies.Smithy.build,
Dependencies.Alloy.core,
Dependencies.Alloy.openapi,
Dependencies.Smithytranslate.proto,
"com.lihaoyi" %% "os-lib" % "0.9.3",
Dependencies.Circe.core.value,
Dependencies.Circe.parser.value,
Dependencies.Circe.generic.value,
Dependencies.collectionsCompat.value,
"org.scala-lang" % "scala-reflect" % scalaVersion.value,
"io.get-coursier" %% "coursier" % "2.1.9"
),
libraryDependencies ++= codegenDeps.value,
libraryDependencies ++= munitDeps.value,
scalacOptions := scalacOptions.value
.filterNot(Seq("-Ywarn-value-discard", "-Wvalue-discard").contains),
Expand Down Expand Up @@ -507,7 +512,13 @@ lazy val millCodegenPlugin = projectMatrix
name := "mill-codegen-plugin",
crossVersion := CrossVersion
.binaryWith(s"mill${millPlatform(Dependencies.Mill.millVersion)}_", ""),
libraryDependencies ++= Seq(
libraryDependencies := codegenDeps.value ++ Seq(
Copy link
Contributor

Choose a reason for hiding this comment

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

I find it weird that you'd have to specify codegenDeps.value here considering this module depends on the codegen module, and therefore should benefit from its dependencies without having to add them here.

Copy link
Author

@Quafadas Quafadas May 31, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

My understanding, is that the sbt := operator discards and overwrites any inherited value to this point.

Which begs the question : why not stay with the ++= operator for the "non-provided" scope. The answer to that, was that the "logic" as to which scope contains what became hard for me to follow (weak sbt again, perhaps). My choice was then to be as explicit as possible that something quite deliberate and at least a little non-standard was going on, by using the := operator in both places.

It may not be the right approach, but it set out (for me) the logic as clearly as possible... I'm not a confident sbt user, so certainly willing to stand corrected.

Copy link
Contributor

Choose a reason for hiding this comment

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

No problem, I'll try and find the time to try it locally before making a decision of whether to approve as-is or not. Regardless, thank you very much for the contribution !

Copy link
Author

Choose a reason for hiding this comment

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

Yes, I have to say that I think this is a surprisingly risky, potentially high maintenance sort of change. In the interim, it appears that Tobias at mill has done something which might sort this out millside - maybe it would be preferable to see if the below mill PR sorts it out in preference to merging this?

com-lihaoyi/mill#3189

As ever, thank you for taking the time to review 🙏

Dependencies.Mill.main % Provided,
Dependencies.Mill.mainApi % Provided,
Dependencies.Mill.scalalib % Provided,
Dependencies.Mill.mainTestkit
),
Test / libraryDependencies := codegenDeps.value ++ Seq(
Dependencies.Mill.main,
Dependencies.Mill.mainApi,
Dependencies.Mill.scalalib,
Expand Down