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

[WIP]Implement reproducible out/ folder contents across different filesystem layouts #3765

Draft
wants to merge 28 commits into
base: main
Choose a base branch
from

Conversation

rahat2134
Copy link

@rahat2134 rahat2134 commented Oct 17, 2024

This PR addresses issue #3660 by making the out/ folder contents more reproducible and filesystem layout agnostic. These changes allow for re-using the out/ folder as a build cache between different machines, supporting both coarse-grained (e.g., zip file transfer) and fine-grained (via Bazel remote cache protocol) caching strategies.

Key Changes:

  • Updated PathRef to normalize paths relative to workspace, Coursier cache, and home directory
  • Implemented NonDeterministicFiles to handle non-deterministic file content
  • Updated JsonFormatters to use the new path normalization methods
  • Added integration test ReproducibleOutTest to verify reproducibility
  • Updated build.mill in the integration test to define the necessary project structure

Reproducibility is achieved by:

  1. Normalizing paths in serialized PathRefs
  2. Handling non-deterministic files (e.g., mill-profile.json, worker JSONs)
  3. Zeroing out modification times for zip and jar files

Testing:

  • Added new integration test ReproducibleOutTest
  • Existing tests have been updated and pass

Documentation:

  • Updated relevant comments in the code

Performance Impact:

  • Initial testing shows minimal impact on build times, but more extensive performance testing may be needed for large projects

@rahat2134 rahat2134 changed the title Implement reproducible out/ folder contents across different filesystem layouts [WIP]Implement reproducible out/ folder contents across different filesystem layouts Oct 17, 2024
@rahat2134
Copy link
Author

rahat2134 commented Oct 25, 2024

@lihaoyi, Can you take a top-level look at the changes and whether they are aligned with the solution or not in any way?
I am working on tests to pass.

(Also, If somehow you can tell why the tests are failing, it will be a great help!)
Thanks!

@lihaoyi
Copy link
Member

lihaoyi commented Oct 26, 2024

@rahat2134 the top level changes look reasonable. The CI logs seem to be overloaded with all the printlns you added and won't load for me, you should try removing them and try to run the failing tests locally to debug further

@lihaoyi
Copy link
Member

lihaoyi commented Oct 29, 2024

@rahat2134 FYI the test jobs are arranged roughly in dependency order, so the first failing job {main,scalalib,testrunner,bsp,testkit}.__.testCached is the one to look at, and the ones after are probably the same failure. Within that, main.eval.test mill.eval.JavaCompileJarTests.javac is the smallest failure in that job, so you can start by running that locally and debugging that first

@lihaoyi
Copy link
Member

lihaoyi commented Nov 2, 2024

@rahat2134 BTW feel free to ask if you have any questions or get stuck

@rahat2134
Copy link
Author

@rahat2134 BTW feel free to ask if you have any questions or get stuck

Ya, Sure. Thanks :)

@rahat2134
Copy link
Author

rahat2134 commented Nov 3, 2024

Hi @lihaoyi

I've made progress with two tests passing:

  1. mill.eval.JavaCompileJarTests.javac
  2. mill.api.PathRefTests (path normalization tests)

However, we're struggling with HelloWorldTests. When running ./mill -i scalalib.test "mill.scalalib.HelloWorldTests.compile.fromScratch", we're seeing:

scala.MatchError: Left(scala.reflect.internal.FatalError: 
  bad constant pool index: 0 at pos: 49842
  while compiling: <no file>
  during phase: globalPhase=<no phase>, enteringPhase=<some phase>
  library version: version 2.12.6
  compiler version: version 2.12.6
)

We've tried:

  1. Path normalization in PathRef.scala to handle compile.super -> compile.dest conversions
  2. Fixing classpath entries before compilation in ZincWorkerImpl
  3. Adding explicit Scala dependencies and proper compiler classpath setup
  4. Various levels of path fixing at both module and worker level

The core issue seems to be that despite our path fixes, the Scala compiler is still getting paths with "compile.super" in the classpath:

reconstructed args: -classpath /Users/.../compile.super/ScalaModule.dest/classes -bootclasspath ...

We think the problem might be deeper than just path normalization, possibly related to how Mill handles compilation paths in test environments.

Would appreciate any guidance on:

  1. The correct approach to handle test compilation paths
  2. Where in the Mill codebase test compilation paths are determined
  3. If there are any special considerations for Scala 2.12.6 compilation in tests

Thanks for your help!

Any other thing if you want to say!

p.s - Just saw right now that this test is passing on github actions -

[09925-10] + mill.scalalib.HelloJavaTests.failures 3460ms
[09925-10] Tests: 7, Passed: 6, Failed: 1
[09925-11] + mill.scalalib.HelloWorldTests.compile.fromScratch 10296ms

I don't know why they are failing on local....

@lihaoyi
Copy link
Member

lihaoyi commented Nov 3, 2024

Could it be a java version thing? CI runs Java 17 and 11, if you are on 8 or 24 you might get different behaviors

Your reported failure of mill.scalalib.HelloWorldTests.compile.fromScratch also appears to be passing in CI https://github.com/com-lihaoyi/mill/actions/runs/11649568853/job/32437259063?pr=3765

[09925-11] + mill.scalalib.HelloWorldTests.compile.fromScratch 10296ms  

You didn't mention which Java version you are using, but try using either 11 or 17 to reproduce the issues locally, and hopefully it will be more consistent with what you are seeing in CI and better able to reproduce the failures

@rahat2134
Copy link
Author

Could it be a java version thing? CI runs Java 17 and 11, if you are on 8 or 24 you might get different behaviors

Your reported failure of mill.scalalib.HelloWorldTests.compile.fromScratch also appears to be passing in CI https://github.com/com-lihaoyi/mill/actions/runs/11649568853/job/32437259063?pr=3765

[09925-11] + mill.scalalib.HelloWorldTests.compile.fromScratch 10296ms  

You didn't mention which Java version you are using, but try using either 11 or 17 to reproduce the issues locally, and hopefully it will be more consistent with what you are seeing in CI and better able to reproduce the failures

Ya, I saw that the test is passing in github, May be it is due to java version...
Currently I am using java 23

@rahat2134
Copy link
Author

Hi, @lihaoyi I was working on fixing the failing tests in the reproducible out/ folder implementation. We've noticed an interesting issue with HelloJavaTests:

  1. Test Status:
  • Passing locally but failing in GitHub Actions
  • Specific failure: MatchError: Right(Result((,List()),62)) in HelloJavaTests.scala:152
  • The test expects Left(Result.Failure(...)) (because MyCoreTests.msgTest should fail) but gets Right(Result) in CI
  1. What we've tried:
  • Added GitHub workspace path normalization to handle CI paths consistently
  • Enhanced test failure detection and logging in TestRunnerUtils
  • Verified Java version is correct (openjdk 17.0.13)
  • Also try some changes in the testRunner, testModule,etc (related) files to make the issue visible on local but no progess.

Debug logs from GitHub Actions show:
Debug: GITHUB_WORKSPACE detected: /home/runner/work/mill/mill
Debug: Input path: /home/runner/work/mill/mill/out/scalalib/test/testCached.dest/mill.scalalib.HelloJavaTests/sandbox/...

Could this be related to how test failures are captured/reported differently in the CI environment? Would appreciate any insights on why the test behavior differs between local and CI environments.

p.s - All the other failures are similar in local and GitHub actions.

@lihaoyi
Copy link
Member

lihaoyi commented Nov 8, 2024

@rahat2134 so it seems like the test is trying to evaluate the task HelloJava.core.test.test(), expects a Left representing a failure failure , but is receiving a Right representing a success.

The tests in question are at scalalib/test/resources/hello-java/core/test/src/MyCoreTests.java, with this assert assertEquals(Core.msg(), "Hello World!!") expecting to fail because Core.msg() is `"Hello World" without the exclamation marks

I'm guessing there is something wrong with the configuration of the source paths, causing the test sources to not be picked up.

To debug this, could you try applying the following patch

lihaoyi mill$ git diff
diff --git a/scalalib/test/src/mill/scalalib/HelloJavaTests.scala b/scalalib/test/src/mill/scalalib/HelloJavaTests.scala
index 7c8a046108..c6effa9679 100644
--- a/scalalib/test/src/mill/scalalib/HelloJavaTests.scala
+++ b/scalalib/test/src/mill/scalalib/HelloJavaTests.scala
@@ -150,6 +150,8 @@ object HelloJavaTests extends TestSuite {
       val eval = testEval()

       val Left(Result.Failure(ref1, Some(v1))) = eval.apply(HelloJava.core.test.test())
+      pprint.log(eval.apply(HelloJava.core.test.sources))
+      pprint.log(eval.apply(HelloJava.core.test.allSourceFiles))


       assert(
         v1._2(0).fullyQualifiedName == "hello.MyCoreTests.lengthTest",

This will print out the folder in which HelloJava.core is looking for the test source files. You can also pprint.log(os.list(...)) or pprint.log(os.walk(...)) that folder to see whats in it.

If you run that locally and in CI, hopefully it will give us a hint as to what is going wrong

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.

2 participants