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

Add an option to disable incremental compilation with zinc #2851

Merged
merged 7 commits into from
Nov 2, 2023

Conversation

mrdziuban
Copy link
Contributor

This was sparked by a conversation in gitter about forcing a full recompile for a given module.

The change to ZincWorkerApi is not binary compatible, but it could be if I added overloads for both compileJava and compileMixed -- let me know if you think I should.

@lolgab
Copy link
Member

lolgab commented Oct 31, 2023

We have def zincLogDebug: T[Boolean], maybe we can use some other name that is consistent with other targets we have in mill?
Maybe zincIncrementalCompilation: T[Boolean] or zincIncrementalCompilationEnabled: T[Boolean] like def debugEnabled: Boolean we have in Logger?

For sure I would avoid having disabled defaulting to false but something defaulting to true instead.

@mrdziuban mrdziuban force-pushed the disable-incremental-compilation branch from 1a39164 to 5979a7c Compare October 31, 2023 15:36
@mrdziuban
Copy link
Contributor Author

👍 thanks for the feedback @lolgab, updated to use zincIncrementalCompilation defaulting to true

Copy link
Member

@lefou lefou left a comment

Choose a reason for hiding this comment

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

Looks good to me. Thank you!

If we want to merge this into Mill 0.11.x, we need to add the backward-compatible overrides. Best is to mark those with @deprecated("Use other override instead", "Mill 0.11.6")

@mrdziuban
Copy link
Contributor Author

Thanks @lefou, I just tried that and mimaReportBinaryIssues still reports 2 issues:

Found 2 issue when checking against com.lihaoyi:mill-scalalib-api_2.13:0.11.5
 * abstract method compileJava(scala.collection.immutable.Seq,mill.api.AggWrapper#Agg,mill.api.AggWrapper#Agg,scala.collection.immutable.Seq,scala.Option,Boolean,Boolean,mill.api.Ctx#Dest)mill.api.Result in interface mill.scalalib.api.ZincWorkerApi is present only in current version
   filter with: ProblemFilter.exclude[ReversedMissingMethodProblem]("mill.scalalib.api.ZincWorkerApi.compileJava")
 * abstract method compileMixed(scala.collection.immutable.Seq,mill.api.AggWrapper#Agg,mill.api.AggWrapper#Agg,scala.collection.immutable.Seq,java.lang.String,java.lang.String,scala.collection.immutable.Seq,mill.api.AggWrapper#Agg,mill.api.AggWrapper#Agg,scala.Option,Boolean,Boolean,mill.api.Ctx#Dest)mill.api.Result in interface mill.scalalib.api.ZincWorkerApi is present only in current version
   filter with: ProblemFilter.exclude[ReversedMissingMethodProblem]("mill.scalalib.api.ZincWorkerApi.compileMixed")

I pushed up my change, let me know if anything stands out to you

@lefou
Copy link
Member

lefou commented Oct 31, 2023

@mrdziuban I think I fixed the Mima report. It would be nice, if you could add a section about this new option to the documentation. I think https://mill-build.com/mill/Scala_Module_Config.html is a good place.

@mrdziuban
Copy link
Contributor Author

@lefou docs added! I also tested the option in my project to confirm it worked and found that it didn't, but I was able to get it working by following the CacheBuster pattern that DockerModule uses. So when zincIncrementalCompilation is false, Math.random() is returned by zincIncrementalCompilationCacheBuster so the result is non-deterministic

@lefou
Copy link
Member

lefou commented Nov 1, 2023

@mrdziuban Can you please describe why you need the additional cacheBuster target?

To my understanding, this should not be necessary, as Mill already detects any relevant changes to the inputs of the compile target. In essence, even if we disable incremental compilation, we don't want to re-compile if nothing has changed. Any change, either to the sources or the classapth or to the new setting will result in a re-evaluation of the compile target.

@lefou
Copy link
Member

lefou commented Nov 1, 2023

If this is about "flakiness" in zinc, adding an integration test to establish trust is the way to go.

@mrdziuban
Copy link
Contributor Author

@lefou I realize the end goal in my project might not be accurately described by disabling incremental compilation. What I want/need is for this:

./mill my-project[3.3.1].compile

to always compile every file in my-project, regardless of whether any inputs to compile have changed. That's what the CacheBuster achieved.

I recognize that that might not be desirable, or might not align with everyone's understanding of disabling incremental compilation, and since I implemented the CacheBuster here I actually found a way to achieve this in my codebase without any changes to mill by doing:

def scalacOptions = T.input {
  super.scalacOptions ++ Seq(s"-Jdummy=${Instant.now.toEpochMilli}")
}

That said, I would be happy with any of these options:

  1. Scrap this whole change altogether since I've found a workaround that fits my needs
  2. Revert the CacheBuster change so zincIncrementalCompilation only takes effect if any inputs to compile have changed
  3. Keep the CacheBuster change and maybe rename the target to better reflect what it does?

I could add an integration test, but I'm juggling this with other work at my day job so can't guarantee that I'll get to it super quickly 🙂

Let me know what you think

@lefou
Copy link
Member

lefou commented Nov 1, 2023

  1. Revert the CacheBuster change so zincIncrementalCompilation only takes effect if any inputs to compile have changed

I'd prefer this option. I think there is some value in this PR, as it helps to easily work around zinc issues in rare cases.

I could add an integration test, but I'm juggling this with other work at my day job so can't guarantee that I'll get to it super quickly 🙂

Let me know what you think

The change is pretty straight forward, so IMHO a test is nice to have but not essential. If you roll back the cache buster, we can merge as is. Feel free, to open a follow up PR with a test, if you're in the mood.

@mrdziuban
Copy link
Contributor Author

👍 done

@lefou lefou merged commit c124843 into com-lihaoyi:main Nov 2, 2023
37 checks passed
@lefou lefou added this to the 0.11.6 milestone Nov 2, 2023
@lefou
Copy link
Member

lefou commented Nov 2, 2023

Thank you, @mrdziuban !

@mrdziuban mrdziuban deleted the disable-incremental-compilation branch January 5, 2024 17:09
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.

3 participants