Enable `buildInfoStaticCompiled` on ScalaJSModule and ScalaNativeModule #2562

lolgab · 2023-06-01T11:38:48Z

Status quo fails linking with a cryptic error message where previous version was working out of the box.
This sets the default to the static behavior on Scala.js and Scala Native so users don't need to care.

lefou

IMHO, we should make static compilation the general default, on all platforms. After all, this feature is some kind of optimization of the developer experience and not without runtime costs.

lihaoyi · 2023-06-01T22:09:25Z

The issue with making static compilation the default is that with common practice - setting versions based on git dirty sha, including the version as a buildinfo field, and using buildinfo in some upstream util module - it ends up invalidating all compilation caches everywhere if you touch any file in the repo. Even an unrelated, unused readme.md file

It's really problematic, and made working with the com-lihaoyi/mill repo very annoying for me. It totally breaks the incremental nature of Mill. I don't think we should make that the common-case default for users running Scala on the JVM

FWIW we hit the same problem at work. With Bazel instead of Mill, in C++ rather than Scala. But the fundamental problem is the same: compiled buildinfo + version in buildinfo + git dirty sha used in version = everything is invalidated all the time. The solution was the same too. So this does seem like a fundamental issue, and not a quirk of our current implementation

lefou · 2023-06-02T05:30:13Z

I see it differently, but it is configurable after all. I think, we should add the implications (pros, cons) to the docs of the setting, as these are far from obvious to a user. If you both agree, I'm not against merging this as-is.

IMHO, a project should test the same artifacts it delivers, and using a fine grained classpath in (integration) tests but a jar in production always has some potential to fail due to the mismatch. E.g. in one of my client projects OSGi is used, which has enriched metadata in the jar manifest and it is key to use the full jars downstream when testing. This has an impact to the developer experience, but instead of compromising accuracy, we just changed the way we format dirty git hashes in version numbers (we keep the "dirty", but not the hash). And as another side note: I still have to fix mill-osgi plugin for Mill 0.11.0-M10 because of that initial change. Apparently I need to override a lot more targets now, to restore the old behavior.

lolgab · 2023-06-02T09:08:13Z

Another possibility is to ship it without a default and let users decide. It's not that nice fox UX but allows users to take informed decisions.

lefou · 2023-06-02T12:24:20Z

Another possibility is to ship it without a default and let users decide. It's not that nice fox UX but allows users to take informed decisions.

I think a sane default is better than non, in this case. I'd assume, most projects don't include a dirty git hash (which changes with every file change) in their versions, and are not affected at all. Those projects will profit from a static config default. Projects that have some highly dynamic content in their build info like Mill, might want to flip that setting. My own projects also have dynamic build info content, yet I still prefer the safer setting, as it can't be the cause of any trouble. The safer settings also makes the implementation simpler, as we don't need to match based on the module type, but which was needed to implement this PR. Also, there are already a lot of settings to override when using the BuildInfo plugin. That was the main reason I typically just wrote the generated files by hand instead of using the BuildInfo plugin.

lihaoyi · 2023-06-02T13:39:25Z

I think this is fine to merge as is, or with some of the discussion captured in the scaladoc

The added complexity isn't too bad. In future, we could even bundle up some JSON with the Scala.js Javascript to support out-of-band buildinfo bundling there, and come up with some solution for Scala-Native: not sure what, but definitely possible. People have been bundling data with their executables since forever.

I think "version string includes dirty without the SHA" isn't a good compromise. The SHA is useful. If I'm running publishLocal, or my own internal Maven server, or distributing jars, I want the SHA. Even for quick hacks with un-committed code, the SHA helps ensure I can distinguish distinct builds of the code, avoiding confusion about artifacts that look identical but are not. Furthermore, it does not solve the problem: I don't see why running git commit should require me to re-compile all my code. Why should we punish users for "commit early, commit often"? Even though Zinc is meant to skip compiling things it doesn't need to, this easily adds 1+ seconds per module, which adds up in any non-trivial project

It's true that compiling things into bytecode is simpler, but it's too simple: given the common-usage-pattern of mill-vcs-version we demonstrate throughout the com-lihaoyi projects, and the common case of BuildInfo being used to propagate the version, it ends up totally disabling a major feature of Mill.

People with advanced use cases would already be equipped with the skills for dealing with libraries requiring auxiliary non-classfile classpath entries, so it won't come as too big a surprise: SPI files, conf files, manifests, etc.. They would also have larger projects that would suffer most from the lack of incrementality, which might not matter that much to you, but does matter to many.

Let's keep the flag for the people who prefer it, and this PR flipping the default for JS/Native is fine since we don't have an alternative for now. But for the bulk of people with JVM code with uncomplicated classpaths, or with complicated classpaths but the skills to manage them, leaving the flag off by default is the right thing to do.

lefou · 2023-06-04T19:28:20Z

build.sc

@@ -729,8 +729,8 @@ object contrib extends Module {
  }

  object buildinfo extends ContribModule {
-    def compileModuleDeps = Seq(scalalib, scalajslib, scalanativelib)
-    def testModuleDeps = super.testModuleDeps ++ Seq(scalalib, scalajslib, scalanativelib)
+    def moduleDeps = Seq(scalalib, scalajslib, scalanativelib)


This means, if somebody includes this module into his project, it will also transitively include the Mill dependendencies. And if the versions do not match exactly, we end up with two different versions in the classpath.

Mill plugins typically always keep dependencies to Mill as compileIvyDeps or compileModuleDeps.

But as a contrib module, wouldn't the versions always line up? Since contrib modules are co-versioned with Mill

Yeah, if used as we suggest. But I've seen also other constructs in the wild. And sometimes, these are even valid. We have no guard against it. The error are typically some strange compiler error messages and aren't helpful.

Got it. I'll open a follow up to move back to compileIvyDeps

And if the versions do not match exactly, we end up with two different versions in the classpath.

Doesn't the version get evicted to the newer one?

Now, that we control the whole build process of build.sc scripts with Mill, we could probably enhance our dependency management. E.g. we could apply some forceVersion to all Mill dependencies and such things.

Yeah that works too. Or just skip them entirely during resolution (is that possible???) since they'll all be bundled anyway

Doesn't the version get evicted to the newer one?

It wouldn't, because the version bundled in the Mill assembly isnt resolved through ivy/maven and thus can't be evicted nor can it evict the other

Doesn't the version get evicted to the newer one?

It wouldn't, because the version bundled in the Mill assembly isnt resolved through ivy/maven and thus can't be evicted nor can it evict the other

Also, in case we include a newer transitive Mill, it could still fail.

Maybe, we should try to resolve them from coursier and reduce the size of our bundle further. If we only come with our runner and coursier, we could load everything else via coursier on demand.

Opened a PR to go back to the earlier behavior via #2568

The non-transitive nature of compileModuleDeps is definitely unfortunate; it means that anyone who depends on the buildInfo plugin needs to manually depend on scalajslib and scalanativelib as well. This isn't a problem for user code, since the Mill assembly always contains both. But can be an annoyance for plugins, e.g. in my PR we needed to add the dependencies to contrib,scoverage rather than having them be pulled in automatically

I think the status quo is probably fine for 0.11.0. After that, we can consider other options, e.g. making use of exclude to allow normal dependencies while automatically removing the Mill jars from dependency resolution.

Shrinking the bundle is possible. With the current bootstrapping mechanism, we would need to keep at least scalalib, since that's required for MillBootstrapRootModule. With an alternative bootstrapping mechanism, we could cut it further, though it's unclear whether that's worth the complexity: for small size, the current bootstrap bash/bat script already wins. We can leave that debate for later

…eps` (#2568) Follow up from #2562, to avoid potentially having duplicate Mill classfiles on the classpath

Enable buildInfoStaticCompiled on ScalaJSModule and ScalaNativeModule

945605b

lolgab requested a review from lihaoyi June 1, 2023 11:38

lolgab added the need-more-info More information is needed to reproduce or fix the issue label Jun 1, 2023

lolgab requested a review from lefou June 1, 2023 11:39

lolgab removed the need-more-info More information is needed to reproduce or fix the issue label Jun 1, 2023

lihaoyi approved these changes Jun 1, 2023

View reviewed changes

Add scalajslib and scalanativelib to buildinfo test deps

eb6f2c5

lefou reviewed Jun 1, 2023

View reviewed changes

lihaoyi added 2 commits June 3, 2023 20:03

use normal moduleDeps

06bf031

.

ba1f366

lihaoyi merged commit 87cd32a into com-lihaoyi:main Jun 4, 2023

lolgab deleted the buildInfoStaticCompiled-js-native branch June 4, 2023 13:50

lefou reviewed Jun 4, 2023

View reviewed changes

lefou added this to the 0.11.0-M11 milestone Jun 4, 2023

lihaoyi mentioned this pull request Jun 4, 2023

Make contrib.buildinfo use compileModuleDeps rather than moduleDeps #2568

Merged

lihaoyi added a commit that referenced this pull request Jun 5, 2023

Make contrib.buildinfo use compileModuleDeps rather than `moduleD…

1535a7b

…eps` (#2568) Follow up from #2562, to avoid potentially having duplicate Mill classfiles on the classpath

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Enable `buildInfoStaticCompiled` on ScalaJSModule and ScalaNativeModule #2562

Enable `buildInfoStaticCompiled` on ScalaJSModule and ScalaNativeModule #2562

lolgab commented Jun 1, 2023 •

edited

Loading

lefou left a comment

lihaoyi commented Jun 1, 2023 •

edited

Loading

lefou commented Jun 2, 2023 •

edited

Loading

lolgab commented Jun 2, 2023

lefou commented Jun 2, 2023

lihaoyi commented Jun 2, 2023 •

edited

Loading

lefou Jun 4, 2023 •

edited

Loading

lihaoyi Jun 4, 2023

lefou Jun 4, 2023

lihaoyi Jun 4, 2023

lolgab Jun 4, 2023

lefou Jun 4, 2023

lihaoyi Jun 4, 2023

lihaoyi Jun 4, 2023

lefou Jun 4, 2023

lihaoyi Jun 4, 2023 •

edited

Loading

Enable buildInfoStaticCompiled on ScalaJSModule and ScalaNativeModule #2562

Enable buildInfoStaticCompiled on ScalaJSModule and ScalaNativeModule #2562

Conversation

lolgab commented Jun 1, 2023 • edited Loading

lefou left a comment

Choose a reason for hiding this comment

lihaoyi commented Jun 1, 2023 • edited Loading

lefou commented Jun 2, 2023 • edited Loading

lolgab commented Jun 2, 2023

lefou commented Jun 2, 2023

lihaoyi commented Jun 2, 2023 • edited Loading

lefou Jun 4, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lihaoyi Jun 4, 2023 • edited Loading

Choose a reason for hiding this comment

Enable `buildInfoStaticCompiled` on ScalaJSModule and ScalaNativeModule #2562

Enable `buildInfoStaticCompiled` on ScalaJSModule and ScalaNativeModule #2562

lolgab commented Jun 1, 2023 •

edited

Loading

lihaoyi commented Jun 1, 2023 •

edited

Loading

lefou commented Jun 2, 2023 •

edited

Loading

lihaoyi commented Jun 2, 2023 •

edited

Loading

lefou Jun 4, 2023 •

edited

Loading

lihaoyi Jun 4, 2023 •

edited

Loading