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

Add support for the check goal for javac #12859

Merged
merged 3 commits into from
Sep 14, 2021

Conversation

stuhood
Copy link
Sponsor Member

@stuhood stuhood commented Sep 12, 2021

Add support for the check goal by making the result of javac compilation a FallibleCompiledClassfiles type so that results are streamed, and the compilation graph can (optionally) complete without raising an exception.

Successful check:
Screen Shot 2021-09-13 at 18 18 07

Failed check:
Screen Shot 2021-09-13 at 18 17 27

@stuhood stuhood force-pushed the stuhood/check-java branch 2 times, most recently from 379dc54 to cf2d675 Compare September 14, 2021 01:05
@stuhood stuhood marked this pull request as ready for review September 14, 2021 01:20
Comment on lines +1 to +16
# 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",
],
)
Copy link
Member

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?

Copy link
Contributor

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.

Copy link
Sponsor Member Author

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).

Copy link
Contributor

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?

Copy link
Sponsor Member Author

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.

Copy link
Member

@patricklaw patricklaw left a 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:
Copy link
Sponsor Member Author

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]
@stuhood stuhood enabled auto-merge (squash) September 14, 2021 05:12
@stuhood stuhood merged commit 75d0de7 into pantsbuild:main Sep 14, 2021
stuhood added a commit that referenced this pull request Sep 14, 2021
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]
@stuhood stuhood deleted the stuhood/check-java branch September 14, 2021 05:59
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants