fix: Make sure scala-native doesn't use cached native-lib when nativeConfig changes

[tanishiking]

Fix: #3567

Previously, the scala-native would not start a new, clean build when switching from multithreadingSupport=false -> true while having incremental compilation enabled.
This was because it would reuse the already compiled cached native-lib nir files. However, when nativeConfig (especially multithreadingSupport) changes, which affects the native-lib code scala-native shouldn't reuse the cached native-lib. This led to compilation errors until doing a manual cleanup.

This commit fixes this issue by removing stale native-config-* directories when the nativeConfig changes. To do that, we added a hashCode of nativeConfig to the native-lib's directories.

❯ ls -l sandbox/.3/target/scala-3.3.1/native | grep native-code
... native-code-clib_native0.5.0-SNAPSHOT_3-0.5.0-SNAPSHOT--1554901494-1/
... native-code-javalib_native0.5.0-SNAPSHOT_3-0.5.0-SNAPSHOT--1554901494-2/
... native-code-nativelib_native0.5.0-SNAPSHOT_3-0.5.0-SNAPSHOT--1554901494-0/
... native-code-posixlib_native0.5.0-SNAPSHOT_3-0.5.0-SNAPSHOT--1554901494-3/
... native-code-windowslib_native0.5.0-SNAPSHOT_3-0.5.0-SNAPSHOT--1554901494-4/

[WojciechMazur]

I'm not sure if it's enough to fix the underlying issue of changing native config. It would work handling changes in native glue code, but it would not force emitting new LLVM IR and especially the thread-safe Op.Module lowering (in singlethreaded mode it's a simple get or else init in multithreading mode is uses methods defined in loadModule.c to define a state machine waiting for the initialization of module by other thread.

[tanishiking]

it would not force emitting new LLVM IR

This change actually makes scala-native emitting new LLVM IR 👍

Reproduction

• Apply sandbox to use withMultithreadingSupport(false)

diff --git a/project/Build.scala b/project/Build.scala
index 54890226..49564101 100644
--- a/project/Build.scala
+++ b/project/Build.scala
@@ -703,6 +703,11 @@ object Build {
       .enablePlugins(MyScalaNativePlugin)
       .withNativeCompilerPlugin
       .withJUnitPlugin
+      .settings(
+        nativeConfig ~= { c =>
+          c.withMultithreadingSupport(false)
+        },
+      )
       .dependsOn(scalalib, javalib, testInterface % "test")

 // Testing infrastructure ------------------------------------------------

• clean build, reload sbt settings, and run nativeLink on sandbox3

$ sbt
sbt> clean; reload
sbt> sandbox3 / nativeLink

• Checking if there's __scalanative_loadModule call / declare in LLVM IR

$ cd sandbox/.3/target/scala-3.3.1/native
$ grep -nr "__scalanative_loadModule" *.ll
# no matches

• Now, enable multithreadingSupport in sandbox

diff --git a/project/Build.scala b/project/Build.scala
index 54890226..49564101 100644
--- a/project/Build.scala
+++ b/project/Build.scala
@@ -703,6 +703,11 @@ object Build {
       .enablePlugins(MyScalaNativePlugin)
       .withNativeCompilerPlugin
       .withJUnitPlugin
+      .settings(
+        nativeConfig ~= { c =>
+          c.withMultithreadingSupport(true)
+        },
+      )
       .dependsOn(scalalib, javalib, testInterface % "test")

 // Testing infrastructure ------------------------------------------------

• Reload (do not clean!), and nativeLink again.

sbt> reload
sbt> sandbox3 / nativeLink

• Checking if the LLVM IR are regenerated -> yes!

$ grep -nr "__scalanative_loadModule" *.ll | head -n 3
11b9624d.ll:76:declare i8* @"__scalanative_loadModule"(i8*, i8*, i64, i8*)
11b9624d.ll:2994:  %_5 = call i8* @"__scalanative_loadModule"(i8* %_4, i8* bitcast ({ { i8*, i8*, i32, i32, i8* }, i32, i32, { i8* }, [4 x i8*] }* @"_SM56scala.scalanative.runtime.ieee754tostring.ryu.RyuDouble$G4type" to i8*), i64 32, i8* bitcast (void (i8*)* @"_SM56scala.scalanative.runtime.ieee754tostring.ryu.RyuDouble$RE" to i8*)), !dbg !420
11b9624d.ll:5111:  %_5 = call i8* @"__scalanative_loadModule"(i8* %_4, i8* bitcast ({ { i8*, i8*, i32, i32, i8* }, i32, i32, { i8* }, [4 x i8*] }* @"_SM55scala.scalanative.runtime.ieee754tostring.ryu.RyuFloat$G4type" to i8*), i64 56, i8* bitcast (void (i8*)* @"_SM55scala.scalanative.runtime.ieee754tostring.ryu.RyuFloat$RE" to i8*)), !dbg !744

[WojciechMazur]

The other issue here is fact that dest dir would not be removed when it's no longer needed. So after frequent changeConfig; reload;nativeLink loops we would end up with garbage in the target dir.

The nativeConfig hash might be a good start, but maybe we should store it in some file in the workdir directory as a form of persistent store between sbt reloads, and check if the current hash has changes. If so, we should perform a cleanup of workdir from all previous artifacts.

[ekrich]

The nativeConfig hash might be a good start, but maybe we should store it in some file in the workdir directory as a form of persistent store between sbt reloads, and check if the current hash has changes. If so, we should perform a cleanup of workdir from all previous artifacts.

I think this might be a good idea.

[tanishiking]

The other issue here is fact that dest dir would not be removed when it's no longer needed.

Not actually, they will be deleted when they are no longer needed

      Files
         // List all the files / dirs in `workDir` (somewhere under `target/scala-$version/native`)
        .list(workDir)

        // List dirs that contains native-lib NIRs such as `native-code-clib_native...`
        .filter(_.getFileName().toString() matches nativeCodePattern)

        // # List stale native-lib dirs that has different name than than current `workDir.resolve(s"$nativeCodePrefix-$name-$confHash-$index"`
        //(if `confHash` has different value than previous build, the stale dir will alive here)
        .filter(p => !expectedPaths.contains(p.toAbsolutePath())) 

        // Delete all stale directories
        .forEach(IO.deleteRecursive(_)) 

scala-native/tools/src/main/scala/scala/scalanative/build/NativeLib.scala

Lines 215 to 219 in bb8aef7

	Files
	.list(workDir)
	.filter(_.getFileName().toString() matches nativeCodePattern)
	.filter(p => !expectedPaths.contains(p.toAbsolutePath()))
	.forEach(IO.deleteRecursive(_))

[ekrich]

I am wondering if there is a more manual way to deal with this. If we hash the config and the settings change maybe we could prompt the user to clean for the situations that require a full clean or something like this. It seems that we are trying to find an automated solution that is not worth it in complexity or time.