Upgraded gradle wrapper to 4.10.2 and fatjar to 0.3 #459
Conversation
…e composite build and upgraded fatjar to 0.3. The gradle wrapper was upgraded by running this command (a few times while issues were being worked out) : ./gradlew wrapper --gradle-version=4.10 --distribution-type=bin With fatjar still at 0.1.2, this error was showing up: > Failed to apply plugin [id 'fatjar'] > Could not find method add() for arguments [fatJarPrepareFiles, class eu.appsatori.gradle.fatjar.tasks.PrepareFiles] on task set of type org.gradle.api.internal.tasks.DefaultTaskContainer.
thinkingserious
added
the
status: code review request
|
|
thinkingserious
added
type: community enhancement
|
Thanks @RohanTalip! This has been added to our backlog for a code review. With Best Regards, Elmer |
|
@thinkingserious, thanks but I see you said the same thing on #436 in May and that one still hasn't been reviewed from what I can tell. Roughly how long should I/we expect to wait for a code review? Is this open source project still a priority for SendGrid? I'm not trying to be snarky, just expressing genuine interest, as I don't see that many recent commits and there are quite a few old open pull requests that I would expect to either be merged or closed as infeasible or not meeting the direction of the product, within N days of being opened (where N is some reasonable number like 7 or 30). I see that you added the difficulty:medium tag to this pull request. If you or someone else at SendGrid would like to replicate what I've done, you can start with running "./gradlew wrapper --gradle-version=4.10 --distribution-type=bin" and then upgrading the fatjar dependency (or alternatively migrating to https://github.com/johnrengelman/shadow ). I have no attachment to being the author for such a change (i.e. feel free to make this change yourselves), it would just be nice to have a recent version of Gradle embedded in the sendgrid-java project. Regards, Rohan |
|
Actually, I see that Gradle 4.10.1 has been released now, so if you were going to make this change yourself, I would upgrade to that instead of 4.10: https://github.com/gradle/gradle/releases/tag/v4.10.1 |
|
Hi @RohanTalip, Your PR is actually coming up soon for a review. This Java SDK (just finished Python, Node.js, PHP and C#) is next on our list to get the dynamic template helpers added and we hope to merge in many of these open PRs that have been sitting for a very long time as part of that release. If you would be so kind to update this PR with the updated 4.10.1 version or maybe 4.10.*, that would be awesome. With Best Regards, Elmer |
(Running "./gradlew wrapper --gradle-version=4.10.2 --distribution-type=bin" once updated the gradle/wrapper/gradle-wrapper.properties file; running it again downloaded the updated gradle/wrapper/gradle-wrapper.jar file -- not sure why it doesn't update the first time.)
|
Hi @thinkingserious, sorry, I had missed your update until now. I just updated the gradle version from 4.10 to 4.10.2. (Personally, if I was maintaining an open source project, I would be very cautious about accepting binary changes, such as .jar files, partly for the changes being opaque and partly for security issues. |
|
That makes sense @RohanTalip, thanks! |
thinkingserious
added
status: waiting for feedback
|
HI @marshallpierce, Do you mind giving this a quick look in comparison to your PR? Whoever's PR gets accepted in the end, both of you will be getting swag if you have not already :) With Best Regards, Elmer |
|
They're pretty similar; I tackled the fatjar -> shadow conversion mentioned here. As mentioned in the comments on mine, it's not clear what the original intent was for the fatjar, so maybe the conversion to use the shadow plugin needs additional config? It's pretty unusual for projects to generate a fat jar and put it in the project root 🤷♂️ |
RohanTalip
changed the title
|
I see that @marshallpierce also updated his PR #436 to use 4.10.2. I cloned his repository locally and switched to his build-on-recent-java branch: $ mkdir marshallpierce Comparing the 2 binary gradle-wrapper.jar files (my change was on the master branch of sendgrid-java, with my fork on GitHub as the default remote repository) : Double checking: Checking the scripts: $ shasum -a 256 sendgrid/sendgrid-java/gradlew* marshallpierce/sendgrid-java/gradlew* $ diff -s sendgrid/sendgrid-java/gradlew marshallpierce/sendgrid-java/gradlew $ diff -s sendgrid/sendgrid-java/gradlew.bat marshallpierce/sendgrid-java/gradlew.bat The build.gradle file is obviously going to be different between our 2 PRs. @marshallpierce or you @thinkingserious could reproduce these checks ... |
|
Thanks for the responses @marshallpierce and @RohanTalip! One of the goals is to produce a fat jar that folks can download and add to their project directly in case they don't want to use a package manager. In the end I move that fatJar out and publish for direct download. |
|
That's fine, but you don't have to do the janky "host a jar on GitHub" system. You can publish a fat jar with different coordinates via bintray, etc... |
|
Hi @marshallpierce, I'm definitely open to that idea, could you please elaborate on the steps or update your PR to help us migrate to a more sensible solution? Thanks! |
|
I don't have the time to implement the build changes at the moment, but here's the idea: currently, this library is published at the coordinates That said, instead of tinkering with that, you might be better off helping customers use a build tool, like with a sample project that has Gradle set up already and is designed for customers to use as a starting point, or something like that... Projects without build automation are sad projects. |
|
That's awesome feedback @marshallpierce, Would you open to helping craft a new issue to that end? |
|
Hello @RohanTalip, |
|
Hello @RohanTalip, |
|
Hello @RohanTalip, |
thinkingserious
added
status: code review request
|
This pull request is really old now and it looks like this project is now going to be using Maven rather than Gradle (see PR #599), so I'm going to close this PR. |
Checklist
Short description of what this PR does:
Upgrading to a recent gradle version allows for a gradle composite build.
The gradle wrapper was upgraded by running this command (a few times while issues were being worked out) :
./gradlew wrapper --gradle-version=4.10 --distribution-type=bin
With fatjar still at 0.1.2, this error was showing up:
Upgrading to fatjar 0.3 fixed this issue.
This pull request is similar to pull request #436, with a newer version of gradle, but is slightly smaller and also doesn't change the build.gradle file away from using the unmaintained Gradle FatJar plugin to the Gradle Shadow plugin yet (but instead upgrades it from 0.1.2 to 0.3).