Conversation
raboof
force-pushed
the
fix-error-showing-protoc-logs
branch
from
e81754a to
2285b5d
Compare
|
Seems to work locally (tested with a locally-published plugin an the project at https://github.com/claudio-scandura/akka-grpc-gradle-plugin-issue ), but I'm not too experienced with Gradle, so would be good to have a review from someone more familiar with this stuff - e.g. @jasonxh @hithran @orendain @eshepelyuk ? |
| else if (line.startsWith("[debug]")) logger.debug(line.substring(7)) | ||
| else if (line.startsWith("[warn]")) logger.warn(line.substring(6)) | ||
| else if (line.startsWith("[error]")) logger.error(line.substring(7)) | ||
| if (Files.exists(logFile)) { |
There was a problem hiding this comment.
Looks like dependsOn line is the main fix. I wonder if there is still any legit case that'll leave logFile non-existent. If not, I think it's better to still fail here, or at least log a warning.
There was a problem hiding this comment.
It looks like when a subproject doesn't generate any sources based on the proto because of NO-SOURCE, it won't produce a logging file either.
The example project above didn't have any proto files, but typically it'd have some in api but not in impl. I think it's OK to just skip without further logging in impl in that case.
| else if (line.startsWith("[error]")) logger.error(line.substring(7)) | ||
| if (Files.exists(logFile)) { | ||
| Files.lines(logFile).forEach { line -> | ||
| if (line.startsWith("[info]")) logger.info(line.substring(7)) |
There was a problem hiding this comment.
Should be .substring(6)? Or if the intention is to also strip out the leading space, should +1 to the other levels as well.
| } | ||
| } | ||
| } | ||
| project.getTasks().getByName("printProtocLogs").dependsOn("generateProto") |
There was a problem hiding this comment.
Besides adding this task dependency, it's also a good idea to add logFile to the task outputs of generateProto, so that gradle can properly retrigger the task if the file is missing or stale. Be careful that generateProto was added as part of project.afterEvaluate, so you'll have to do the same here.
Taking a huge step back, since generateProto by default outputs everything under build/generated/source/proto/<sourceSet>, it might be a better idea to actually move the logFile to be over there, so we don't have to mess with its outputs and we also avoid overwrites from multiple source sets (which may be another bug right now?)
There was a problem hiding this comment.
That sounds like a sensible improvement, but I think I'd like to keep things less ambitious in this PR for now :)
raboof
force-pushed
the
fix-error-showing-protoc-logs
branch
from
23e162b to
3efd20d
Compare
raboof
force-pushed
the
fix-error-showing-protoc-logs
branch
from
3efd20d to
73b8b48
Compare
2 participants
No description provided.