PhilanthropyDataCommons / service

A project for collecting and serving public information associated with grant applications
GNU Affero General Public License v3.0
8 stars 2 forks source link

Use Java 17 and cache Gradle artifacts #1131

Closed jasonaowen closed 1 month ago

jasonaowen commented 1 month ago

The SDK build failed recently due to a network timeout downloading the Gradle binary.

Use the setup-gradle action to cache Gradle artifacts between runs, including both the Gradle binary and the dependencies it downloads. Caching should improve performance, protect against transitory network failures, and help us be a better neighbor by downloading less.

The setup-gradle action recommends the setup-java action. Previously, we were defaulting to Java 11, the version provided by the runner. Specify Java 17, which is the most recent LTS supported by the version of Gradle used by the SDK.

codecov[bot] commented 1 month ago

Codecov Report

All modified and coverable lines are covered by tests :white_check_mark:

Project coverage is 88.57%. Comparing base (fdceafd) to head (da0cb8f). Report is 14 commits behind head on main.

Additional details and impacted files ```diff @@ Coverage Diff @@ ## main #1131 +/- ## ======================================= Coverage 88.57% 88.57% ======================================= Files 129 129 Lines 1725 1725 Branches 220 220 ======================================= Hits 1528 1528 Misses 182 182 Partials 15 15 ```

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

jasonaowen commented 1 month ago

As a bonus, the setup-gradle plugin also generates a nice build summary! Compare a previous run that does not have a summary.

bickelj commented 1 month ago

I guess another option is to have it build multiple times: once with 11, once with 17, and once with 21. That would provide maximum feedback of any potential problems for latest/greatest folks (which we want to encourage) and also for the slower ones.

jasonaowen commented 1 month ago

I think a simpler solution is to target Java 11 here. That way we don't have to change the SDK repo at all and we verify that Java 11 can compile, test, etc., and result in Java 11 classes.

As you mention a few times, @bickelj, we're not testing the SDK here, nor are we exporting any Java artifacts; the purpose of this workflow is to make sure that our OpenAPI specification can successfully be built into a TypeScript SDK, and to publish that SDK. That is, in this repo we are a client of the SDK, and we trust that it works, and so should not concern ourselves here with validating the SDK.

For this reason alone, I'd like to be on the latest LTS that we can support. Currently that's 17; with https://github.com/PhilanthropyDataCommons/sdk/pull/30 that will hopefully be 21.

...a breaking change for Java 11 users...

...updating from one major Java version to the next takes years (decades) in many organizations...

...another option is to have it build multiple times: once with 11, once with 17, and once with 21.

The SDK repo already claims that it requires Java 18, and has from the start. To the best of my knowledge, nobody has complained yet about this requirement.

Rather than burden ourselves with the the responsibility of claiming and testing backwards support before anyone has asked for it, and enable bad behavior in large organizations by doing so, let's just stick to "latest LTS" until/unless we get a specific request for support.

I'm not advocating that we add any code that requires newer versions of Java, but there's only like 100 lines of Java code in the SDK repo, and our compatibility almost entirely depends on the libraries and plugins we're using.

I think before merging [this PR] we should ensure that the SDK project targets a Java 11 runtime.

Why before?

bickelj commented 1 month ago

@jasonaowen wrote:

As you mention a few times, @bickelj, we're not testing the SDK here, nor are we exporting any Java artifacts; the purpose of this workflow is to make sure that our OpenAPI specification can successfully be built into a TypeScript SDK, and to publish that SDK. That is, in this repo we are a client of the SDK, and we trust that it works, and so should not concern ourselves here with validating the SDK.

Oh, right. Thank you for clarifying this bit: "the purpose of this workflow is to make sure that our OpenAPI specification can successfully be built into a TypeScript SDK, and to publish that SDK." We are not generating any Java classes here, just using the SDK. I was thinking this was our only test that verifies the SDK can build and also that we were building Java classes (not TypeScript code).

@jasonaowen wrote:

For this reason alone, I'd like to be on the latest LTS that we can support. Currently that's 17; with PhilanthropyDataCommons/sdk#30 that will hopefully be 21.

Yes, I agree. In this repository that makes perfect sense.

@jasonaowen wrote:

The SDK repo already claims that it requires Java 18, and has from the start. To the best of my knowledge, nobody has complained yet about this requirement.

Rather than burden ourselves with the the responsibility of claiming and testing backwards support before anyone has asked for it, and enable bad behavior in large organizations by doing so, let's just stick to "latest LTS" until/unless we get a specific request for support.

I'm not advocating that we add any code that requires newer versions of Java, but there's only like 100 lines of Java code in the SDK repo, and our compatibility almost entirely depends on the libraries and plugins we're using.

I would guess the reason no one has asked for it is no one has tried it yet. A less likely possibility is that those who only have Java 11 would be quietly warded off by the 18 requirement. In any case, my point about maximizing reach by minimizing Java version stands for the SDK repository. We absolutely should support the latest LTS and prioritize making that work. But if all the dependent libraries work on Java 11 we shouldn't bump the minimum up to 17, for example. Anyway, I'm OK with not explicitly testing 11, 17, 21 and sticking with testing the one LTS for now. Perhaps when changing the readme over in the SDK repo @slifty you can note that it may work with both earlier and later Java versions, to try it out and let us know. Something like that.

I think before merging [this PR] we should ensure that the SDK project targets a Java 11 runtime.

Why before?

Assuming we were testing Java 11 before, my point was to continue testing Java 11 before additionally testing Java 17.

In summary, I withdraw my objection here. This is a build step for TypeScript and not a direct test of the SDK. If there are SDK users in the future, testing the SDK and Java compatibility will become more important in that repository.

jasonaowen commented 1 month ago

Thanks for your thoughtful reviews, @bickelj and @slifty!

I will rebase and update to Java 21 now that the gradle upgrade PR is merged.