Fix incremental compilation of SPI in Java helper libraries #8129

radeusgd · 2023-10-21T19:10:18Z

Pull Request Description

Fixes the issue that sometimes occurred on CI where old services configuration was not cleaned and SPI definitions were leaking between PRs, causing random failures:

❌ should allow selecting table rows based on a boolean column
	An unexpected panic was thrown: java.util.ServiceConfigurationError: org.enso.base.file_format.FileFormatSPI: Provider org.enso.database.EnsoConnectionSPI not found

The issue is fixed by detecting unknown SPI classes before the build, and if such classes are detected, cleaning the config and forcing a rebuild of the given library to ensure consistency of the service config.

Important Notes

Checklist

Please ensure that the following checklist has been satisfied before submitting the PR:

The documentation has been updated, if necessary.
Screenshots/screencasts have been attached, if there are any visual changes. For interactive or animated visual changes, a screencast is preferred.
All code follows the
Scala,
Java,
and
Rust
style guides. In case you are using a language not listed above, follow the Rust style guide.
All code has been tested:
- Unit tests have been written where possible.
- If GUI codebase was changed, the GUI was tested when built using ./run ide build.

radeusgd · 2023-10-21T19:13:19Z

To test this (since this cannot be unit tested), I've created a Python script:

import os

clazz = """
package org.enso.database;

import org.enso.base.file_format.FileFormatSPI;

@org.openide.util.lookup.ServiceProvider(service = FileFormatSPI.class)
public class TestFormatSPI extends FileFormatSPI {
  @Override
  protected String getModuleName() {
    return "Standard.Database.Connection.Connection";
  }

  @Override
  protected String getTypeName() {
    return "Connection";
  }
}
"""

path = "std-bits/database/src/main/java/org/enso/database/TestFormatSPI.java"

enso_script = """
from Standard.Base import all
from Standard.Database import all

import Standard.Base.System.File_Format as File_Format_Module

main =
    IO.println "First pass:"
    IO.println <| Panic.recover Any <| File_Format_Module.format_types

    IO.println ""
    IO.println "Second pass:"
    File_Format_Module.format_types.each IO.println
"""

with open("load-formats.enso", "w") as f:
    f.write(enso_script)

with open(path, "w") as f:
    f.write(clazz)

print("Wrote file: " + path)

os.system("sbt buildEngineDistribution")
os.system("enso run load-formats.enso")

os.remove(path)
print("Deleted file: " + path)

os.system("sbt buildEngineDistribution")
os.system("enso run load-formats.enso")

os.remove("load-formats.enso")

It creates a new File_Format type in std-database, compiles Enso and then removes it and compiles again. The helper script load-formats.enso will load the formats and detect any SPI config issues.

Running that script on current develop yields:

Wrote file: std-bits/database/src/main/java/org/enso/database/TestFormatSPI.java
[info] welcome to sbt 1.9.0 (GraalVM Community Java 17.0.7)
(...)
[INFO] [2023-10-21T21:02:07+02:00] [enso.org.enso.interpreter.runtime.DefaultPackageRepository] Found library Standard.Table @ 0.0.0-dev at [***\0.0.0-dev].
First pass:
[Bytes, JSON_Format, Plain_Text_Format, XML_Format, SQLite_Format, Connection, Delimited_Format, Excel_Format]

Second pass:
Bytes
JSON_Format
Plain_Text_Format
XML_Format
SQLite_Format
Connection
Delimited_Format
Excel_Format
Deleted file: std-bits/database/src/main/java/org/enso/database/TestFormatSPI.java
[info] welcome to sbt 1.9.0 (GraalVM Community Java 17.0.7)
(...)
[INFO] [2023-10-21T21:04:49+02:00] [enso.org.enso.interpreter.runtime.DefaultPackageRepository] Found library Standard.Table @ 0.0.0-dev at [***\0.0.0-dev].
First pass:
(Error: java.util.ServiceConfigurationError: org.enso.base.file_format.FileFormatSPI: Provider org.enso.database.TestFormatSPI not found)

Second pass:
Bytes
JSON_Format
Plain_Text_Format
XML_Format
SQLite_Format
Delimited_Format
Excel_Format

As we can see, currently the script reproduces the CI issue - after removing the added new class - the first pass of loading the formats fails with ServiceConfigurationError not being able to find TestFormatSPI anymore.

…emoved too late and required 2 recompilations to work

radeusgd · 2023-10-21T20:05:18Z

After applying the patch from this PR and re-running the repro Python script, we get:

Wrote file: std-bits/database/src/main/java/org/enso/database/TestFormatSPI.java
[info] welcome to sbt 1.9.0 (GraalVM Community Java 17.0.7)
(...)
[info] Generating index for built-distribution\enso-engine-0.0.0-dev-windows-amd64\enso-0.0.0-dev\lib\Standard\Table
[info] Engine package created at built-distribution\enso-engine-0.0.0-dev-windows-amd64\enso-0.0.0-dev
[success] Total time: 174 s (02:54), completed 21 paź 2023, 21:58:10
[INFO] [2023-10-21T21:58:14+02:00] [enso.org.enso.interpreter.runtime.DefaultPackageRepository] Found library Standard.Base @ 0.0.0-dev at [***\0.0.0-dev].
[INFO] [2023-10-21T21:58:14+02:00] [enso.org.enso.interpreter.runtime.DefaultPackageRepository] Found library Standard.Database @ 0.0.0-dev at [***\0.0.0-dev].
[INFO] [2023-10-21T21:58:14+02:00] [enso.org.enso.interpreter.runtime.DefaultPackageRepository] Found library Standard.Table @ 0.0.0-dev at [***\0.0.0-dev].
First pass:
[Bytes, JSON_Format, Plain_Text_Format, XML_Format, SQLite_Format, Connection, Delimited_Format, Excel_Format]

Second pass:
Bytes
JSON_Format
Plain_Text_Format
XML_Format
SQLite_Format
Connection
Delimited_Format
Excel_Format
Deleted file: std-bits/database/src/main/java/org/enso/database/TestFormatSPI.java
[info] welcome to sbt 1.9.0 (GraalVM Community Java 17.0.7)
(...)
[warn] No Java sources detected for classes: List(TestFormatSPI.class).
[warn] Removing C:\NBO\enso\std-bits\database\target\scala-2.13\classes\META-INF\services\org.enso.base.file_format.FileFormatSPI and forcing recompilation of List(SQLiteFormatSPI.class) to ensure that the SPI definition is up-to-date.
(...)
[info] Generating index for built-distribution\enso-engine-0.0.0-dev-windows-amd64\enso-0.0.0-dev\lib\Standard\Table
[info] Engine package created at built-distribution\enso-engine-0.0.0-dev-windows-amd64\enso-0.0.0-dev
[success] Total time: 167 s (02:47), completed 21 paź 2023, 22:01:28
[INFO] [2023-10-21T22:01:32+02:00] [enso.org.enso.interpreter.runtime.DefaultPackageRepository] Found library Standard.Base @ 0.0.0-dev at [***\0.0.0-dev].
[INFO] [2023-10-21T22:01:33+02:00] [enso.org.enso.interpreter.runtime.DefaultPackageRepository] Found library Standard.Database @ 0.0.0-dev at [***\0.0.0-dev].
[INFO] [2023-10-21T22:01:33+02:00] [enso.org.enso.interpreter.runtime.DefaultPackageRepository] Found library Standard.Table @ 0.0.0-dev at [***\0.0.0-dev].
First pass:
[Bytes, JSON_Format, Plain_Text_Format, XML_Format, SQLite_Format, Delimited_Format, Excel_Format]

Second pass:
Bytes
JSON_Format
Plain_Text_Format
XML_Format
SQLite_Format
Delimited_Format
Excel_Format

As we can see, this time the missing file gets immediately detected and a rebuild of std-database is triggered:

[warn] No Java sources detected for classes: List(TestFormatSPI.class).
[warn] Removing C:\NBO\enso\std-bits\database\target\scala-2.13\classes\META-INF\services\org.enso.base.file_format.FileFormatSPI and forcing recompilation of List(SQLiteFormatSPI.class) to ensure that the SPI definition is up-to-date.

then, the first pass succeeds without the issue that used to happen on the repro before:

First pass:
[Bytes, JSON_Format, Plain_Text_Format, XML_Format, SQLite_Format, Delimited_Format, Excel_Format]

meaning that the problem should be fixed and our incremental CI builds should no longer suffer from this issue! 🎉

radeusgd · 2023-10-21T20:16:11Z

This is what SBT logs look like with debug enabled:

> buildStdLibAll
(...)
[debug] Scanning C:\NBO\enso\std-bits\image\target\scala-2.13\classes\META-INF\services for SPI definitions.
[debug] Jar uptodate: C:\NBO\enso\distribution\lib\Standard\Base\0.0.0-dev\polyglot\java\std-base.jar
[debug] Scanning C:\NBO\enso\std-bits\table\target\scala-2.13\classes\META-INF\services for SPI definitions.
[debug] Processing service definitions: C:\NBO\enso\std-bits\image\target\scala-2.13\classes\META-INF\services\org.enso.base.file_format.FileFormatSPI
[debug] No missing classes detected in C:\NBO\enso\std-bits\image\target\scala-2.13\classes\META-INF\services\org.enso.base.file_format.FileFormatSPI.
[debug] Processing service definitions: C:\NBO\enso\std-bits\table\target\scala-2.13\classes\META-INF\services\org.enso.base.file_format.FileFormatSPI
[debug] The source file [C:\NBO\enso\std-bits\table\src\main\java\org\enso\table\read\TestFormatSPI.java] for class [org.enso.table.read.TestFormatSPI] does not exist.
[warn] No Java sources detected for classes: List(TestFormatSPI.class).
[warn] Removing C:\NBO\enso\std-bits\table\target\scala-2.13\classes\META-INF\services\org.enso.base.file_format.FileFormatSPI and forcing recompilation of List(DelimitedFormatSPI.class, ExcelFormatSPI.class) to ensure that the SPI definition is up-to-date.
(...)

4e6 · 2023-10-21T20:36:06Z

May be related #8006

mwu-tow

Looks promising!

JaroslavTulach

Nice attempt to fix the problem that once forced us to split runtime into runtime-service-id-instrument, etc. Good to see the sbt limitations can be overcome when one understands the sbt design.

JaroslavTulach · 2023-10-23T04:35:21Z

project/SPIHelpers.scala

+            )
+          }
+
+          (classFilePath, hasSource)


Iterates the current project to find all .class files in META-INF/services/serviceConfig and checks if they have .java source.

JaroslavTulach · 2023-10-23T04:39:41Z

project/SPIHelpers.scala

+          s"to ensure that the SPI definition is up-to-date."
+        )
+        IO.delete(serviceConfig)
+        kept.foreach { case (path, _) => IO.delete(path) }


Deletes the META-INF/services file and deletes the .class files for the kept ones. Great idea.

JaroslavTulach · 2023-10-23T04:40:37Z

build.sbt

@@ -2122,6 +2122,9 @@ lazy val `std-base` = project
  .settings(
    frgaalJavaCompilerSetting,
    autoScalaLibrary := false,
+    Compile / compile / compileInputs := (Compile / compile / compileInputs)
+      .dependsOn(SPIHelpers.ensureSPIConsistency)


Could we do this for all projects that have Compile / compile /compileInputs?

As noted in the docs - I'm not sure if this will work well with mixed-Scala projects - since as you noticed - I check for existence of a .java file corresponding to .class.

In case of mixed projects - that file may not exist if the service is written in Scala.

If we can guarantee we won't have such a case, I guess we could try to set it as a global setting, if I recall correctly it should be possible.

I'd like to merge the current PR, because it solves the most immediate issue and got through CI.

But if we decide in the discussion that we want to amend how we 'register' this 'hook', I'm happy to do a followup PR updating that.

hubertp

Note that this wouldn't work if service was defined on a mixed project that has both Java and Scala service providers. For that to work you would have to add 33a06c9 as well.

But... it looks like we no longer have that case, so this is fine.

project/SPIHelpers.scala

radeusgd added 3 commits October 21, 2023 20:37

checkpoint: test processor

86340c6

prototype for the fix

02fc7cf

removing tmp file

d711e90

radeusgd self-assigned this Oct 21, 2023

radeusgd added the CI: No changelog needed Do not require a changelog entry for this PR. label Oct 21, 2023

radeusgd added 5 commits October 21, 2023 21:28

move to separate module, document, proper logs

2be9d40

add to other std- projects

336060e

check for source files instead of class files - as class files were r…

e5cf150

…emoved too late and required 2 recompilations to work

add debug log

04cc1d5

doc

ae2fad5

radeusgd marked this pull request as ready for review October 21, 2023 20:05

radeusgd requested review from 4e6, JaroslavTulach, hubertp and Akirathan as code owners October 21, 2023 20:05

radeusgd requested a review from mwu-tow October 21, 2023 20:05

4e6 approved these changes Oct 21, 2023

View reviewed changes

4e6 mentioned this pull request Oct 21, 2023

Transient issues in backend tests #7775

Open

25 tasks

mwu-tow approved these changes Oct 22, 2023

View reviewed changes

JaroslavTulach approved these changes Oct 23, 2023

View reviewed changes

hubertp approved these changes Oct 23, 2023

View reviewed changes

project/SPIHelpers.scala Show resolved Hide resolved

radeusgd added the CI: Ready to merge This PR is eligible for automatic merge label Oct 23, 2023

mergify bot merged commit 1114a9b into develop Oct 23, 2023
33 of 35 checks passed

mergify bot deleted the wip/radeusgd/try-cleaning-spi branch October 23, 2023 09:14

This was referenced Oct 23, 2023

Avoid Inexact_Type_Coercion when downloading a database table #8049

Closed

Simplify code to run in the browser #8092

Merged

hubertp mentioned this pull request Nov 8, 2023

Separate compilation does not take into account Enso files #8262

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix incremental compilation of SPI in Java helper libraries #8129

Fix incremental compilation of SPI in Java helper libraries #8129

radeusgd commented Oct 21, 2023 •

edited

Loading

radeusgd commented Oct 21, 2023

radeusgd commented Oct 21, 2023

radeusgd commented Oct 21, 2023 •

edited

Loading

4e6 commented Oct 21, 2023

mwu-tow left a comment

JaroslavTulach left a comment

JaroslavTulach Oct 23, 2023

JaroslavTulach Oct 23, 2023

JaroslavTulach Oct 23, 2023

radeusgd Oct 23, 2023

radeusgd Oct 23, 2023

hubertp left a comment

Fix incremental compilation of SPI in Java helper libraries #8129

Fix incremental compilation of SPI in Java helper libraries #8129

Conversation

radeusgd commented Oct 21, 2023 • edited Loading

Pull Request Description

Important Notes

Checklist

radeusgd commented Oct 21, 2023

radeusgd commented Oct 21, 2023

radeusgd commented Oct 21, 2023 • edited Loading

4e6 commented Oct 21, 2023

mwu-tow left a comment

Choose a reason for hiding this comment

JaroslavTulach left a comment

Choose a reason for hiding this comment

JaroslavTulach Oct 23, 2023

Choose a reason for hiding this comment

JaroslavTulach Oct 23, 2023

Choose a reason for hiding this comment

JaroslavTulach Oct 23, 2023

Choose a reason for hiding this comment

radeusgd Oct 23, 2023

Choose a reason for hiding this comment

radeusgd Oct 23, 2023

Choose a reason for hiding this comment

hubertp left a comment

Choose a reason for hiding this comment

radeusgd commented Oct 21, 2023 •

edited

Loading

radeusgd commented Oct 21, 2023 •

edited

Loading