mill codegen dependancies are "provided" #1552
Conversation
|
@kubukoz This is the "I tried to read the manual" version. It's been a long time since I used SBT! It appears, that I have not broken anything. I'm not sure though, how to test if I have "made something better"? Does this kind of thing need a test / validation? |
can you reproduce the earlier problem in CI? If so, doing that + then applying this fix should be a good protection against regressions. I guess we'd have to invoke Mill via the CLI, which is currently not being done here. |
I'm honestly not sure quite how / where would be the best place to go about trying that. I've publishLocal'd and validated that it does clear up the original error.
Yes, I think this could end up being quite "heavyweight" for the risk / reward ratio. I'd follow advice... |
| @@ -407,21 +426,7 @@ lazy val codegen = projectMatrix | |||
| "alloyVersion" -> Dependencies.Alloy.alloyVersion | |||
| ), | |||
| buildInfoPackage := "smithy4s.codegen", | |||
| libraryDependencies ++= Seq( | |||
There was a problem hiding this comment.
I'm not sure I understand the rationale behind extracting this. In theory, you shouldn't have to.
There was a problem hiding this comment.
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 .
| @@ -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( | |||
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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 !
There was a problem hiding this comment.
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?
As ever, thank you for taking the time to review 🙏
Aims to fix #1544
PR Checklist (not all items are relevant to all PRs)