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

Allow projects which don't use jdk-15 to resolve latest nullaway #2400

Merged
merged 7 commits into from
Sep 21, 2022

Conversation

carterkozak
Copy link
Contributor

Before this PR

Crashes due to bugs resolved in newer nullaway releases

After this PR

==COMMIT_MSG==
Allow projects which don't use jdk-15 to resolve latest nullaway
==COMMIT_MSG==

Possible downsides?

Bit of a hack, but likely better than alternatives?

@changelog-app
Copy link

changelog-app bot commented Sep 20, 2022

Generate changelog in changelog/@unreleased

Type

  • Feature
  • Improvement
  • Fix
  • Break
  • Deprecation
  • Manual task
  • Migration

Description

Allow projects which don't use jdk-15 to resolve latest nullaway

Check the box to generate changelog(s)

  • Generate changelog entry

project.getDependencies().addProvider("errorprone", project.provider(() -> {
// Cannot upgrade dependencies in this case because newer nullaway/checkerframework
// are not compatible with java 15, but fully support jdk11 and jdk17.
return anyProjectUsesJava15(project) ? null : dep;
Copy link
Contributor

@fawind fawind Sep 21, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I wonder if this should be the other way around? Per default, we use the latest versions but downgrade to the old compatible version when using j15? That way, j15 is the special case and usage of this hack will phase out naturally once projects roll to j17 (plus excavator will continue to update our used versions)?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How would you suggest doing that? Part of the issue is that it would bump errorprone deps as well to a version that’s no longer compatible with jdk15. Gcv makes upgrades much easier than downgrades :-/

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmm yeah, I was thinking of using resolutionStrategy.force("java-15-dep-coordinate") for all deps that we need to pin to a specific version when using j15. But yeah, we would have to enumerate all the error-prone deps that are also not compatible and this might cause issues when other deps depend on them.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmm yeah, this is becoming more complicated. I guess its fine to put this in for now, given that jdk 15 is EOL soon and we can remove the special casing.

// the current latest version.
"org.checkerframework:dataflow-errorprone:3.25.0",
"org.checkerframework:dataflow-nullaway:3.25.0",
"org.checkerframework:checker-qual:3.25.0");
Copy link
Contributor

@fawind fawind Sep 21, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We should probably also move nullaway and checkerframework to this section in versions.props to prevent excavator updating it directly:

# dependency-upgrader:OFF
# Don't upgrade, we will remove this in a future release.
# Users should depend on this plugin directly such that it will get updated by excavator.
com.palantir.javaformat:gradle-palantir-java-format = 1.1.0
# Newer spotless versions have issues resolving dependencies at configuration time
com.diffplug.spotless:spotless-plugin-gradle = 6.6.0
# dependency-upgrader:ON

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I could go either way, we do want to upgrade it eventually, and our test coverage will prevent version-bumps from going through (jdk-15 nullaway integration test won't work!)

Copy link
Contributor

@fawind fawind Sep 21, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah nice, just saw that you added regression tests in #2389

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Baseline-java-versions is super helpful for this style of testing :-]

@bulldozer-bot bulldozer-bot bot merged commit bcc1aab into develop Sep 21, 2022
@bulldozer-bot bulldozer-bot bot deleted the ckozak/newer_nullaway branch September 21, 2022 13:04
@svc-autorelease
Copy link
Collaborator

Released 4.174.0

bulldozer-bot bot pushed a commit to palantir/witchcraft-api that referenced this pull request Sep 27, 2022
###### _excavator_ is a bot for automating changes across repositories.

Changes produced by the roomba/latest-baseline-oss check.

# Release Notes
## 4.174.0
| Type | Description | Link |
| ---- | ----------- | ---- |
| Improvement | Allow projects which don't use jdk-15 to resolve latest nullaway | palantir/gradle-baseline#2400 |


## 4.175.0
| Type | Description | Link |
| ---- | ----------- | ---- |
| Fix | Fix GitHub issues navigation Idea config | palantir/gradle-baseline#2403 |


## 4.176.0
| Type | Description | Link |
| ---- | ----------- | ---- |
| Improvement | Error-prone is enabled in idea for uniformity with CLI compilation | palantir/gradle-baseline#2405 |


## 4.177.0
| Type | Description | Link |
| ---- | ----------- | ---- |
| Fix | baseline-immutables adds required exports to the java compiler for compatibility with jdk-17+ | palantir/gradle-baseline#2406 |


## 4.178.0
| Type | Description | Link |
| ---- | ----------- | ---- |
| Fix | Refaster compilation uses exports matching error-prone compilation | palantir/gradle-baseline#2407 |



To enable or disable this check, please contact the maintainers of Excavator.
This was referenced Sep 27, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants