-
-
Notifications
You must be signed in to change notification settings - Fork 649
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
Add support for the check
goal for javac
#12859
Conversation
379dc54
to
cf2d675
Compare
# Copyright 2021 Pants project contributors (see CONTRIBUTORS.md). | ||
# Licensed under the Apache License, Version 2.0 (see LICENSE). | ||
|
||
java_library( | ||
dependencies=[ | ||
":lockfile", | ||
], | ||
) | ||
|
||
coursier_lockfile( | ||
name = "lockfile", | ||
maven_requirements = [], | ||
sources = [ | ||
"coursier_resolve.lockfile", | ||
], | ||
) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Echoing feedback I received on a previous review: is testprojects not now discouraged?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm still not a big fan because it results in brittle tests, multiple of them depending on the same fixture.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There are no tests depending on it. I could move it under examples
? I find it easier in many cases to iterate on fixing issues outside of the test harness (...should probably invest time in fixing that, but).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah I do agree there, I've found it useful for Go too. But then we get into a situation if we don't have tests where we risk bit rot 🤔
Maybe the solution is have them, but don't rely on them for intricate testing like we did in v1?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Will leave this here, and have added a test.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM in terms of the javac model changes
[ci skip-rust] [ci skip-build-wheels]
def level(self) -> LogLevel: | ||
return LogLevel.ERROR if self.exit_code != 0 else LogLevel.INFO | ||
|
||
def message(self) -> str: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Known issue: until #12877 has landed, this will only render the first time it fails, and not if it hits cache.
[ci skip-rust] [ci skip-build-wheels]
# Rust tests and lints will be skipped. Delete if not intended. [ci skip-rust] # Building wheels and fs_util will be skipped. Delete if not intended. [ci skip-build-wheels]
cf2d675
to
eb56cea
Compare
Add a `cacheable` flag to `EngineAwareReturnType` to ease forcing the sideeffects of `EngineAware` types. #12859 will use this to force re-rendering of failed (but not successful) compile outputs. Additionally: clarify the meaning of "can_modify_workunit" by renaming it to `engine_aware_return_type`, and remove redundancy of `impl`s. [ci skip-build-wheels]
Add support for the
check
goal by making the result ofjavac
compilation aFallibleCompiledClassfiles
type so that results are streamed, and the compilation graph can (optionally) complete without raising an exception.Successful check:

Failed check:
