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

Peculiar cache behavior for external modules #2419

Closed
lefou opened this issue Apr 5, 2023 · 4 comments · Fixed by #2421
Closed

Peculiar cache behavior for external modules #2419

lefou opened this issue Apr 5, 2023 · 4 comments · Fixed by #2421
Labels
bug The issue represents an bug
Milestone

Comments

@lefou
Copy link
Member

lefou commented Apr 5, 2023

In the process of fixing regressions in the BSP module (after the remove-ammonite) I noticed some strange behavior which is probably caused by some stale caches.

When I run a newer Mill version, the bspWorkerLibs target of the external mill.bsp.BSP module uses a stale cache value. I think, previously, it was re-computed. The default value of that target is computed inside of that target. As the external module is part of the classpath, a change was tracked by the classpath sig and resulted in an invalidation (I think). It looks like somehow we no longer track a change in the classpath.

@lihaoyi
Copy link
Member

lihaoyi commented Apr 5, 2023

Seems like it could be related to

// We want to use the grandparent buildHash, rather than the parent
// buildHash, because the parent build changes are instead detected
// by analyzing the scriptImportGraph in a more fine-grained manner.
nestedRunnerState
.frames
.dropRight(1)
.headOption
.map(_.runClasspath.map(p => (p.path, p.sig)).hashCode())
.getOrElse(0),

It seems to me that that logic relies on at least one meta-build to be present in mill-build/build.sc, in order to propagate the Mill executable classpath to the user-level evaluator in order for it to invalidate stuff. The Mill executable classpath is added to the unmanagedClasspath of all levels of meta-builds, so >1 meta build would work fine. However, with 0 meta builds, the getOrElse(0) would return 0, ignoring the Mill executable classpath entirely.

Maybe instead of getOrElse(0), it should be getOrElse(millBootClasspath.map(PathRef(_).sig) or similar?

@lefou
Copy link
Member Author

lefou commented Apr 5, 2023

Yeah, that seem to fix the issue for me. Any idea, how to test this?

I opened PR #2421 to address it.

@lihaoyi
Copy link
Member

lihaoyi commented Apr 5, 2023

Not sure about testing TBH. This misbehavior only happens when the Mill version changes, and we don't really have any precedence for using different Mill versions in our tests. All our tests use Mill build from HEAD

@lefou
Copy link
Member Author

lefou commented Apr 5, 2023

It's probably ok to merge the linked PR as is, as it appears to fix my concrete case.

Maybe, we can come up with some shell or Scala scripting doing the necessary version bumps and building two different mill versions, in the spirit of our bootstrap tests.

NB: We also have issues with handling a Mill version change in a running server, which we could approach this time with some tests. I fixed most of them manually in the past, but it was a time consuming process, and it partially regressed when we switched our rpc library to junixsocket.

@lefou lefou added this to the 0.11.0-M8 milestone Apr 5, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug The issue represents an bug
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants