twitter / scrooge

A Thrift parser/generator
http://twitter.github.io/scrooge/
Apache License 2.0
793 stars 247 forks source link

Unavoidable "[Importer$] Expected to find just 1 match" warnings in sbt sub-project builds #341

Open rtyley opened 3 years ago

rtyley commented 3 years ago

Describe the bug For an sbt multi-project build which uses the ScroogeSBT autoplugin on a subproject, an unavoidable warning message (warn [Importer$] Expected to find just 1 match for ..., introduced March 2020 with https://github.com/twitter/scrooge/commit/9191deba0311cae2cef2f0f59af8f4af07a278e4) will be shown for every single .thrift file found, with no possible way to configure the project to avoid that warning.

To Reproduce Steps to reproduce the behavior:

  1. Create a sbt multi-project build, with ScroogeSBT disabled on all projects, except the subproject containing .thrift files in src/main/thrift
  2. Execute sbt compile
  3. See a warning for every .thrift file:

2020-11-26 12:43:26.379Z warn [Importer$] Expected to find just 1 match for /Users/roberto_tyley/code/ophan/event-model/src/main/thrift/device.thrift, but found 2, in DirImporter(/Users/roberto_tyley/code/ophan), DirImporter(/Users/roberto_tyley/code/ophan/event-model/src/main/thrift) 2020-11-26 12:43:26.394Z warn [Importer$] Expected to find just 1 match for /Users/roberto_tyley/code/ophan/event-model/src/main/thrift/device.thrift, but found 2, in DirImporter(/Users/roberto_tyley/code/ophan), DirImporter(/Users/roberto_tyley/code/ophan/event-model/src/main/thrift)

Expected behavior

Warnings should not be shown, or it should be at least possible to configure the sbt project in such a way that there are not two DirImporters for the same file. The number of warnings shown at the moment is heavily noisy, at least if you have many .thrift files.

Environment

sbt v1.4.4, scrooge v20.10.0

Additional context

The warning was introduced with https://github.com/twitter/scrooge/commit/9191deba0311cae2cef2f0f59af8f4af07a278e4. It's probably valid, but the fact that there are two paths found (the project root, and the path of the subproject src/main/thrift dir) is because the root is hard-coded into the scrooge Compiler class, and there's no way to turn it off:

https://github.com/twitter/scrooge/blob/37edfe5bede6477c3b091e081e5a193c8ff125a7/scrooge-generator/src/main/scala/com/twitter/scrooge/Compiler.scala#L48

...I've played the scroogeThriftIncludes setting, and could not find any way to remove the warnings.

cc @mosesn

felixbr commented 3 years ago

Until the fix (#345) is merged and released, you can use the same workaround I used in our internal sbt-plugin to at least silence the warning:

  private val importerLogger = java.util.logging.Logger.getLogger("com.twitter.scrooge.frontend.Importer$")
  importerLogger.setLevel(java.util.logging.Level.OFF)

It's hacky but the best solution I could find for the time being 🙂

muuki88 commented 3 years ago

Next scrooge sbt version will have a setting to disable this

scroogeThriftIncludeRoot := false
jstultz commented 3 years ago

I am not sure that the issue here is really that the root folder is included by default, but rather with the way absolute paths are handled. Any DirImporter will successfully resolve an absolute path, and when compiling a thrift file, it is the first file that the importer(s) resolve, and is going to be an absolute path. So while not including the root directory as an importer will solve this for some projects, it will still result in spurious warnings any time that a project has scroogeThriftIncludes with more than one entry (which would, I believe, be any project that has any dependencies in the thrift configuration?)

I believe conditioning the warning on a check like this would suffice to solve the problem more generally:

https://github.com/twitter/scrooge/blob/76504bac5d696e126d0a06df506d4923fc7962e2/scrooge-generator/src/main/scala/com/twitter/scrooge/frontend/Importer.scala#L87-L88

jstultz commented 3 years ago

(if others agree, I'd be happy to submit a PR)

niodice commented 1 year ago

Also seeing this quite a bit. I tend to ignore it and I always see the correct behavior in the generated code. This makes me think that @jstultz is correct in that these are spurious warnings.

Anyone maintainers have more context here?

csaltos commented 10 months ago

I've added Compile / scroogeThriftIncludeRoot := false to the Thrift configuration subprojects and it's working for me ... thank you very much for the fix and workarounds.