pkl icon indicating copy to clipboard operation
pkl copied to clipboard

Update to Gradle 8.6

Open translatenix opened this issue 11 months ago • 3 comments

This PR/commit builds on top of #227 to start from a stable Gradle 7.5 build.

  • Fix and clean up the pkl-commons-test build script.
  • Change tests to read test packages/certs directly from the file system instead of packaging and reading them from the class path, which is both slower and more complicated.
  • Update expected checksums of some test packages. (Not sure why they changed.)
  • Work around a conflict between Pkl's and Gradle's Kotlin libraries in the pkl-gradle project.
  • Fix build deprecation warnings.
  • Ensure Gradle distribution integrity with distributionSha256Sum.
  • Manually verify integrity of Gradle wrapper added by this commit.

Result: gw clean build buildNative succeeds with Gradle 8.6.

Hunting down the remaining build deprecation warnings shown by --warning-mode all is worthwhile but not urgent. Most (possibly all) of them are caused by Gradle plugins, which I decided not to update in this commit.

translatenix avatar Feb 24 '24 23:02 translatenix

If you can figure out why the hashes changed, I'm up against the same issue in #204. I wonder if hashes broke in the same way for both of us?

sgammon avatar Feb 25 '24 20:02 sgammon

If you can figure out why the hashes changed

Test package zip files are created with Gradle's Zip task, so perhaps something changed there. (I think it would be better to use the same code as pkl project package.) But I didn't have the nerve to investigate further, updated the hashes and moved on.

translatenix avatar Feb 25 '24 22:02 translatenix

Thanks! Will review this after #227 is merged.

Test package zip files are created with Gradle's Zip task, so perhaps something changed there. (I think it would be better to use the same code as pkl project package.) But I didn't have the nerve to investigate further, updated the hashes and moved on.

Gradle must have changed something about how their zip balls are produced.

But, as long as these hashes are reproducible, it doesn't really matter that much.

bioball avatar Feb 29 '24 02:02 bioball

@translatenix can you rebase again? Thanks!

bioball avatar Mar 13 '24 17:03 bioball

@bioball done

translatenix avatar Mar 13 '24 23:03 translatenix

Instead of using a hardcoded string for the build directory we could also use project.layout.buildDirectory

StefMa avatar Mar 14 '24 05:03 StefMa

@StefMa What’s the benefit? It’s quite verbose.

translatenix avatar Mar 14 '24 09:03 translatenix

Well, its unlikey to happen, but you can also change the buildDirectory 😬 https://docs.gradle.org/8.6/javadoc/org/gradle/api/Project.html#setBuildDir-java.io.File-

In this case hardcoding build would fail 😁

StefMa avatar Mar 14 '24 09:03 StefMa

Doesn’t sound compelling. I think the real benefit is automatic dependency tracking for tasks with Property inputs/outputs. Perhaps refactoring the existing code could eliminate a ‘dependsOn’ here and there.

translatenix avatar Mar 14 '24 10:03 translatenix

There's also the need to parse the strings, and of course string paths can interfere on Windows...

sgammon avatar Mar 14 '24 17:03 sgammon

Hm?

This option is meant for debugging, but I put it back. (At some point it didn't work.)

I think we should try to do the "right thing" and use layout.buildDirectory instead.

Can this be done in a separate PR? "build" is no worse than buildDir.


Some background:

https://github.com/gradle/gradle/issues/26234

Blindly replacing buildDir with layout.buildDirectory doesn't feel like the right solution. For example, replacing

args("--output-dir", "build/testConfigClasses")
args(fileTree("src/test/resources/codegenPkl"))

with

 argumentProviders.add(CommandLineArgumentProvider { 
   listOf("--output-dir", outputDir.get().asFile.absolutePath) +
     fileTree("src/test/resources/codegenPkl").map { it.absolutePath }
 })

doesn't improve anything because methods such as fileTree() are already lazy. It only adds unnecessary complexity and makes the build less readable.

translatenix avatar Mar 14 '24 23:03 translatenix

@translatenix From the issue, this:

  testResults.from("${layout.buildDirectory.get().asFile}/test-results/testIntegration/binary")

Can actually be:

  testResults.from(layout.buildDirectory.dir("test-results/testIntegration/binary"))

sgammon avatar Mar 14 '24 23:03 sgammon

@sgammon Yeah I wouldn't write such code. But my point still stands--just take a look at the suggested diff.

translatenix avatar Mar 15 '24 00:03 translatenix

@translatenix I was just trying to make a case that it can be slightly less verbose than expressed in that issue.

args("--output-dir", "build/testConfigClasses")
args(fileTree("src/test/resources/codegenPkl"))

and

argumentProviders.add(CommandLineArgumentProvider { 
   listOf("--output-dir", outputDir.get().asFile.absolutePath) +
     fileTree("src/test/resources/codegenPkl").map { it.absolutePath }
 })

Are not identical behavior, though, because the args in the former are computed immediately; the args are deferred as well as fileTree. layout.buildDirectory is deferred, so the CommandLineArgumentProvider is irrelevant anyway. Gradle, however, will output warnings for $buildDir/ and strings have the downside of parsing and path issues cross-platform; truthfully, I wish Gradle kept it less verbose, but here we are.

sgammon avatar Mar 15 '24 00:03 sgammon

In any case, I'm just another PR filer / user, my only intent is to offer a less verbose route in case it is less of a change / less painful to implement. 👍

sgammon avatar Mar 15 '24 00:03 sgammon

There's nothing to compute here unless you're trying to hypothetically save a microsecond. I'm also not aware of any cross-platform issues that layout.buildDirectory solves.

I think a separate PR is the way to go. Personally I believe there are more important things to improve (also in the build). For example, I'd love to update the libraries, and I'd LOVE to be able to clone the project on Windows.

translatenix avatar Mar 15 '24 00:03 translatenix

Can this be done in a separate PR? "build" is no worse than buildDir.

Fine by me; will submit that patch as a follow-up

bioball avatar Mar 15 '24 00:03 bioball

@sgammon Would it be possible, and make sense, to use buildless for the Pkl build? Sounds like direct competition for Develocity. And perhaps you could ask your web designer to give pkl-lang.org a facelift. :-)

translatenix avatar Mar 15 '24 00:03 translatenix

@translatenix Ha! Our web designer is a theme from Webflow but that is very kind of you. Buildless is open and can be self-hosted. We'd be happy to give Pkl a free instance if the team would like to try it.

It accelerates local builds and CI builds, especially in GHA and Circle.

The best part of Buildless is that you can open the cache for read-only traffic publicly. So contributors can gain the benefits, too. Develocity is a whole different beast: the reporting, test execution/avoidance, etc., are not things we cover. We are a rosetta stone of build caching protocols, so you can use Buildless with Bazel and SCCache, too.

The two are complementary because we can replicate writes to Develocity and we can front it with our CDN layer, which is Cloudflare Enterprise.

In any case, thank you for asking. I am a big fan of Pkl and here to help however I can.

sgammon avatar Mar 15 '24 00:03 sgammon

Sounds good. I'm not part of the team, just trying to help where I can.

translatenix avatar Mar 15 '24 00:03 translatenix

Well I will still give you an instance if you want one, just fill in the form and we can get you in the next beta wave 😎. I am using it on my fork and it works great in the Pkl build. But I'm a build caching nerd and I don't expect everyone to be 🤣

sgammon avatar Mar 15 '24 00:03 sgammon

I think you should demo buildless to the Pkl team!

translatenix avatar Mar 15 '24 00:03 translatenix

@bioball 👀 if you'd have us we'd be glad! But anyway, it's not right to talk about buildless on your PR 😄 thanks for the Gradle upgrade because it will let me rebase a lot away.

sgammon avatar Mar 15 '24 00:03 sgammon

@bioball Ready from my side.

translatenix avatar Mar 15 '24 21:03 translatenix

@bioball CI is failing due to some missing task dependencies on createTestPackages. Will send a follow-up PR shortly.

translatenix avatar Mar 16 '24 00:03 translatenix