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

KSPLogger.error does not fail compilation #122

Closed
yigit opened this issue Oct 12, 2020 · 4 comments
Closed

KSPLogger.error does not fail compilation #122

yigit opened this issue Oct 12, 2020 · 4 comments
Assignees
Labels
enhancement New feature or request P1 major features or blocking bugs
Milestone

Comments

@yigit
Copy link
Collaborator

yigit commented Oct 12, 2020

Calling KSPLogger should fail compilation after processing is done, otherwise, processors don't have a way to gracefully fail compilation.

@neetopia neetopia self-assigned this Oct 18, 2020
@neetopia neetopia added enhancement New feature or request P2 affects usability but not blocks users labels Oct 18, 2020
copybara-service bot pushed a commit to androidx/androidx that referenced this issue Oct 19, 2020
I couldn't add a test for KSP because right now logging
errors do not fail compilation in KSP.
If by the time we need to run room compiler's failure tests,
if it is still not implemented in KSP, I'll update the
abstraction to track messages and throw error at the end
of compilation.

google/ksp#122.

I've added a test to be enabled once that issue is fixed.

Bug: 160322705
Test: XProcessingEnvTest
Change-Id: Ied320bf329e026a71cc8c107f40e371e6bf8691e
@ZacSweers
Copy link
Contributor

To work around this, I wrap the logger in my own logger that throws a CompilationErrorException (which kotlinc looks for and is what Kapt uses)

// TODO temporary until KSP's logger makes errors fail the build
private class FailOnErrorKSPLogger(private val delegate: KSPLogger) : KSPLogger by delegate {
  private val hasErrors = AtomicBoolean(false)
  override fun error(message: String, symbol: KSNode?) {
    delegate.error(message, symbol)
    hasErrors.set(true)
  }

  fun reportErrors() {
    if (hasErrors.get()) {
      throw CompilationErrorException()
    }
  }
}

// Later in my processor
override fun init(
  options: Map<String, String>,
  kotlinVersion: KotlinVersion,
  codeGenerator: CodeGenerator,
  logger: KSPLogger,
) {
  this.logger = FailOnErrorKSPLogger(logger)
}

override fun finish() {
  logger.reportErrors()
}

@yigit
Copy link
Collaborator Author

yigit commented Dec 27, 2020

I think you should not need this anymore.
At least since we've separated the ksp task from the main compilation.
I even have a test for this in some random branch; https://github.com/yigit/ksp/blob/plugin-tests/gradle-plugin/src/test/kotlin/com/google/devtools/ksp/gradle/GradleCompilationTest.kt#L38

@ting-yuan ting-yuan added P1 major features or blocking bugs and removed P2 affects usability but not blocks users labels Dec 31, 2020
@ting-yuan ting-yuan added this to the alpha milestone Dec 31, 2020
@ting-yuan
Copy link
Collaborator

Still need a way to allow processors to fail and this seems a reasonable approach?

@neetopia
Copy link
Contributor

Actually this start to work now.. Not sure how, potentially upstream fixed some logic around message collector.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request P1 major features or blocking bugs
Projects
None yet
Development

No branches or pull requests

4 participants