-
-
Notifications
You must be signed in to change notification settings - Fork 323
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
Move BuildInfo from sources into resources, defer use of resources during compilation #2425
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Interesting approach. It swaps compiletime work with runtime work. Not really my taste.
I think this approach complicates things. We now do some work at runtime which can fail. To be performant, we also need to cache things a bit, to avoid loading the resources again and again. Also, in the current draft, we loose Scaladoc, but that could be changed, and in general the flexibility to provide more than just key-value pairs.
What I like about this approach. It is still better than using system properties for things, which are not meant to be changed.
What I wonder. How great is the impact of using generated code that changes with every git change? As we use zinc, which tracks binary changes and our generated classes does not change its API (only the values, which are not part of the ABI), downstream compilation targets should not really be affected. Only other targets like |
@lefou I updated the PR description with some rough benchmarks running My own specific motivation comes when running the The new mechanism is definitely more complicated, but I'd argue it better reflects the true dependencies of the system:
|
Yeah being limited to primitive key-values is a bit annoying, but I don't think it's the end of the world. We can always add back support for more complex data structures with the appropriate code-gen. But there's only one place in |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Currently, you put each property in a single resource file. Have you considered using java.util.Properties
for loading from a single file? This would also reduce complexity.
.map { | ||
case (k, v) => | ||
if (isScala) s"""val $k = this.readMillBuildInfo("$k")""" | ||
else s"""public static java.lang.String $k = readMillBuildInfo("$k");""" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This needs to be a method, not a field, otherwise the compiler is allowed to inline the value at the call side, which we need to avoid.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This needs to be a method, not a field, otherwise the compiler is allowed to inline the value at the call side, which we need to avoid.
I don't think that happens unless the RHS of the final
field is a literal constant or (constant-folded). So in this case, a method call loading a JVM resource, it won't get inlined
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In the buildInfoStaticCompiled = true
case, we need to make it a method, as the constants are candidates for inlining (https://docs.oracle.com/javase/specs/jls/se7/html/jls-15.html#jls-15.28), but that means we also need to make it a method in the other case, otherwise the generated code is not interchangeable.
I considered it, but some things I liked about separate files:
|
One thing of note is that I had to add I think that's probably a good thing that we've started dogfooding our own plugins, even if it takes a bit more setup to bootstrap. |
822f690
to
1ae58e4
Compare
all the feedback so far has been addressed and CI is green(ish) |
One thought, not a request: Isn't it now possible to directly include the |
It isn't, because Once we tag a milestone and rebootstrap, we'll be able to configure the
|
I was referring to the bootstrap tests, where we need to publishLocal the plugin, but instead could load it directly as source file. |
In another project we kept |
Oh, for the bootstrap tests, we might be able to. I don't see how |
How does the symlinks work with the package declaration st the top of the file? My understanding is that it's a blocker, since it's required to have one for |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Some files needs formatting. Otherwise, this looks good to go. And I'm happy about the outcome. The buildinfo plugin is now in a usable state (Java support and Java/ScalaDoc). Nice work!
Is this how you're meant to auto-format the files? It's the command you gave inhttps://github.com/com-lihaoyi/os-lib/pull/160, but seems to give me in an error
|
…2435) This should allow us to do debugging or iteration on example/integration in `.local` mode, which is a lot faster than `.fork` or `.server` Even after #2425 landed, the `.local` integration tests are still much faster to run than `.fork` and `.server`, with publishing taking ~20s down from ~60s before which is still a very annoying wait. Better if we can avoid that, using `.local` in the common development workflow and leaving `.fork` and `.server` for CI to catch rare edge-case bugs We basically needed to be a bit more robust in spawning subprocesses, allowing the `InputPumper` machinery to work for any redirected streams rather than being hardcoded to work with the `PipedInputStream` that `MillServerMain` uses. I had to fix an issue in `ScalaJSModule#run` to make `example.web[3-scalajs-module].local.test` pass with this additional enforcement. It appears we were not properly pumping the JSEnv subprocess stdout/stderr. This would cause the outputs to disappear when run repeatedly in client-server mode, since the output would be inherited by first MillClient process that spawned the server, and not subsequent clients that simply connect to it --------- Co-authored-by: Tobias Roeser <le.petit.fou@web.de>
This broke my Scala.js build. |
This adds support for Mill API 0.11. In Mill 0.11.0-M8, some internals have changed (com-lihaoyi/mill#2425), so the consumption of transitive module dependencies had to be changed to continue to generate proper OSGi Manifests. This feels a bit fragile in the sense, that if the customized targets and their interactions change upstream, we might need to follow these changes in this plugin. Pull request: #118
Instead of compiling the buildinfo values into the source code, we only compile the buildinfo keys into the source code and store the buildinfo values in the resources, to be loaded at runtime. This greatly reduces the amount of time spent compiling things when buildinfo changes. In this repo, some rough tests indicate that it makes the second
-i installLocal
after a one-line change in theLICENSE
file drop from 50s to 14s of runtimemill-profile.json
Before:mill-profile.txt
mill-profile.json
After:mill-profile.txt
Notes:
To really benefit from this, I had to make
compileClasspath
depend only on upstream module'scompileClasspath()
/compile().classes
, so it doesn't end up depending on their runtimeresources()
and forcing unnecessary recompiles when the buildinfo resources change. This is probably a change we want to do anyway, given the direction set by Don't include resources into compileClasspath. #1811I consolidated the BuildInfo implementation with
contrib/buildinfo/
.Extended
contrib/buildinfo/
to support Java modules, which we dogfood inmain.client
.The old behavior of statically compiling the values into the binary is still available under
def buildInfoStaticCompiled = true
, for anyone who needs it. That includes anyone usingBuildInfo
in Scala.js, which doesn't support JVM resources by defaultFor now, I copy-pasted the implementation into the root
build.sc
file, and updatedmill-bootstrap.patch
to remove it and make use of themill-contrib
version. When we re-bootstrap Mill on latest main, we can remove the copy-pastacontrib/buildinfo/
should be mostly source compatible with the previous implementation, except that I madebuildInfoPackageName
a required field to encourage the best practice of not putting code in the empty package