Upgrade to Gradle 7.3 #5824
Upgrade to Gradle 7.3 #5824
Conversation
|
Could one of the @bisq-network/pricenode-operators volunteer to build and run and test this PR? I'd like to get a sanity check that everything is OK with the big Spring Boot upgrade underneath. |
This commit does what is necessary to upgrade from Gradle 6.6.1 to 7.3, including: - generating the new Gradle wrapper - replacing uses of 'compile' with 'implementation' - replacing uses of 'testCompile' with 'testImplementation' Moving from *compile to *implementation results in many more duplicated dependency declarations throughout the file. These will be tidied up in a subsequent commit. Several dependencies needed to be upgraded in order to support this change. One of them was Spring Boot, from 1.5.1 to 2.5.6. This is a major upgrade that contained some breaking changes to the Spring Boot Actuator. These changes required the removal of the pricenode's /getVersion endpoint. The Gradle Witness plugin has been disabled in this commit, because it uses the now-removed 'compile' configuration. Use of the Witness plugin will be removed entirely in a subsequent commit in favor of using Gradle's new built-in dependency verification feature.
This commit removes the use of our fork of the gradle-witness plugin in
favor of Gradle's relatively new built-in dependency verification
feature [1].
The gradle/verification-metadata.xml file was produced using the
following command:
./gradlew --write-verification-metadata sha256 build
Where `build` is the usual `gradle build` command. All dependency
configurations are resolved this way, and written out to the file.
The resulting file contains 273 unique dependency declarations, as
compared to just 64 in our now-removed gradle-witness.gradle file. This
means that the coverage of dependencies verified is much more complete.
The new file contains the same sha256 checksums for each dependency as
the old file. This was manually spot-checked for a significant number of
the dependencies.
Like with gradle-witness, builds will break when dependencies are
upgraded (and now also when they are added). To fix these breakages, the
`--write-verification-metadata sha256` option must be provided to the
build. Note that new entries will be added for upgraded depedencies, but
old entries are not removed automatically from the file. These must be
removed manually.
[1]: https://docs.gradle.org/current/userguide/dependency_verification.html
|
Update: the Gradle Witness plugin has been removed entirely in favor of using Gradle's built-in dependency verification. See commit 6f8197b for details. |
CI build failed after the previous commit because the new dependency verification file was generated on a Mac and therefore did not include linux-specification artifact variants. This same process will need to be done under Windows as well.
This commit updates the verification-metadata.xml file to include osx-x86_64 variants of protoc dependencies.
|
@ripcurlx, could you do the following on your Windows VM?
This will (a) ensure the changes in this PR build on windows as expected, and (b) will update the new |
This fixes the IllegalAccessError problem documented at https://stackoverflow.com/a/66981165/622403
|
Update: @Emzy has agreed to take the pricenode through its paces tomorrow. |
This fixes the 'Unsupported class file major version 60' problem documented at mockito/mockito#2065 to allow building Bisq on JDK 16+.
|
Update: following the previous two commits, Bisq can now be built under JDK 16 and 17. Previously, it could only be built up to JDK 15. Ideally, we should update our GitHub workflow to support building under all the JDKs we want to support. Currently we only build under JDK 11. |
Prior to this commit, IDEA would fail to build the project because it downloads javadoc and source jars that do not have entries in the verification file. These artifacts are now trusted by default as documented at https://docs.gradle.org/current/userguide/dependency_verification.html#sec:skipping-javadocs
As mentioned in a prior commit, the upgrade to Gradle 7.x results in many more dependency declarations in the file, many of which are effectively duplicates. This change does not attempt to eliminate those duplications in any clever way, but rather just tidies up and organizes all dependency declarations by sorting them alphabetically.
|
I believe this PR is now ready to go, at least I intend to make no further changes to it. What's needed now is testing. As mentioned above, @Emzy will test out the pricenode tomorrow. I have been unable to build and test out the desktop myself because I'm running on an M1 machine. I'll reach out to a few people directly to see if they'd be willing to just build and run this PR to sanity check it. |
This fixes the CI build failure at https://github.com/bisq-network/bisq/runs/4198811212?check_suite_focus=true#step:6:306 by adding missing entries for findbugs, jsr305 and various netty artifacts. It is not clear why these artifacts were required under linux and not under MacOS.
These entries showed up as missing when @jmacxx ran this PR branch on his local Linux machine under JDK 11. It is not clear why these dependencies were required there and not elsewhere, e.g. under CI or on my own Mac.
|
Not starting after build. Reproduce: After that worked I checked out this PR and started as bisq user: As root: Error in LOG is: Starting manuel als user bisq: |
How shall I handle changed lib versions? Only commit the artifact entries with newer versions I guess (e.g. com.google.api.grpc, com.faster.xml.jackson,...)? |
|
can you paste the diff here? I'll review and apply it as a patch on my side. |
This comment has been minimized.
This comment has been minimized.
Pasted the patch above ☝️ |
|
can you also paste the output of |
|
|
Also, @ripcurlx, with those changes to the verification metadata file in place, are you able to run a full build, i.e. |
|
Hm. I see you've got Gradle 6.6.1 there. Should be 7.3 on this branch. |
True - this is weird indeed, but I freshly checked out the branch. Let me check the git log. |
|
We're getting close on this PR. @ripcurlx has verified everything works on Windows as expected. @Emzy is verifying pricenode changes. I've seen everything build and run on (non-M1) macos. CI works fine on linux. Would be good to have a Linux desktop user verify that the app builds and runs successfully there. |
|
Worked after two builds after upgrade from master. Reproduce: After that worked I checked out this PR and started as bisq user: Build error: Second build: No error. As root: Works. |
|
I checked all hashes and version changes from the old build file with the new one with following findings: Library version updates
Changed hashescom.google.api.grpc:proto-google-common-protos @cbeams I guess all library version updates where necessary to make this build work? As we have a a very cautious approach on updating libraries, I'd like to get an ACK of more contributors (at least @chimp1984 or @sqrrm) |
Correct, each of these upgrades were strictly necessary in order to be able to build / run under Gradle 7+ |
|
Where is I think we should exclude the price node to an independent project as it has very little Bisq dependency which either could be added as gradle dependency there or just refactored away (I think it was just some utils for currency lists). @ripcurlx Have you tested that update for building the binaries?
Is the grpcVersion, lombokVersion and protobufVersion required? |
Yes, I've built macOS for now using JDK 15 and JDK 11
Yes, I think so. |
perfmark and opencensus are both transitive dependencies of grpc-core. I remember running across them before, quite a while ago, i.e. in an older version of grpc (they're not new). I would not recommend excluding them. Keep in mind that the move away from gradle-witness to Gradle's built-in dependency verification now captures all dependencies, around 273 as opposed to the just 64 dependencies we were tracking before. So it's not surprising they show up here for the first time.
Sounds like a reasonable change, but I'd recommend making it a subsequent change instead of doing it as part of this PR. I can commit to doing that.
Building and running against JDKs 11 -> 15 work in conjunction with this PR. JDK 16 and 17 may work, I can't recall whether I tested them. See also #5835.
I can double-check this; one or more of them may not be strictly necessary for the Gradle 7.3 upgrade here, but were rather required for supporting building on Apple M1 (#5835). So even if we didn't upgrade them here, we would definitely have to upgrade them in that PR. For that reason, I'd prefer to leave them as-is here to avoid reworking the PR, dependency verification metadata, etc.
I agree supply chain attacks are a real risk, but an overly conservative approach to using and upgrading dependencies is also a risk. This concern is something that comes up over and over again, and I think we'd be wise to come up with an 'official' and pragmatic policy on this matter. I suggest something like the following: that like all code changes, dependency updates cannot be committed without review. And that "review" means that the committer making the change needs to account for why the dependencies are being updated and that the reviewer needs to verify those claims. Both parties should consider the context for the changes, including how likely it is that an attack might be involved. For example, a likely scenario for a supply chain attack would be that some helpful-seeming new contributor comes along and informs us that one of our dependencies is out of date, and that we should really use the newest version of it. This would be a highly suspicious change for obvious reasons. On the other hand, the changes in this PR are by nature much lower risk. We're updating dependencies we've been using for years, and we're doing it for our own reasons on our own timeline. The chances of an attacker coordinating a supply chain attack in conjunction with this PR are not zero, but are very low. We should therefore do basic diligence on the dependencies being changed, but we shouldn't be afraid to make the changes that are necessary to keep our build system up to date, that ensure Bisq can be built on relevant architectures, etc. |
Yes that would be great. I guess you will know the right way how to do it without losing the GH history (but also if thats not feasible I think its worth it). Agree to the statements regarding supply chain attacks... |
See #5824. Keeping history won't be feasible. I've tried this before, it's not worth the trouble.
Roger that. Does this PR have your ACK at this point? |
|
Concept ACK |
@chimp1984 yeah I agree, actually we were thinking of doing this as part of the mempool project, but as a low priority task |
Problem: When merging bisq-network#5824, the absence of this entry caused a build failure at dependency verification time against JDK11 and JDK15 on Ubuntu-latest [1]. It may also cause failures on other JDK/OS combinations, but the GitHub workflow was aborted before those failures couldhave occurred. In any case, this omission did not create build failures on any of the local development machines that tested the aforementioned PR. Reasons for this discrepancy are unknown. Solution: manually fetch the pom from [2], run `sha256sum` on it locally and commit the result to the verification metadata file. [1]: https://github.com/bisq-network/bisq/runs/4249640611?check_suite_focus=true#step:6:33 [2]: https://repo1.maven.org/maven2/com/fasterxml/jackson/jackson-base/2.11.1/jackson-base-2.11.1.pom
Problem: When merging bisq-network#5824, the absence of this entry caused a build failure at dependency verification time against JDK11 and JDK15 on Ubuntu-latest [1]. It may also cause failures on other JDK/OS combinations, but the GitHub workflow was aborted before those failures couldhave occurred. In any case, this omission did not create build failures on any of the local development machines that tested the aforementioned PR. Reasons for this discrepancy are unknown. Solution: manually fetch the pom from [2], run `sha256sum` on it locally and commit the result to the verification metadata file. [1]: https://github.com/bisq-network/bisq/runs/4249640611?check_suite_focus=true#step:6:33 [2]: https://repo1.maven.org/maven2/com/fasterxml/jackson/jackson-base/2.11.1/jackson-base-2.11.1.pom
This PR successfully upgrades the build from Gradle 6.6.1 to 7.3, but with a number of caveats that need to be worked out:
compileconfiguration. The witness plugin has been disabled in the meantime, and it should be re-enabled when fixed.implementationdependency declarations, now that transitivecompiledeclarations are no longer supported. This may be fine in the end (the explicitness is a good thing), but it's worth considering if we want to tidy them up in some way./getVersionendpoint has been removed entirely. @bisq-network/pricenode-operators, is this endpoint something you actually use, or is it fine to remove?Background: the reason I set out on this upgrade was part of the process of getting Bisq to build on an M1 Mac. That still appears impossible due to a bug in JFoenix's progress meter. See cbeams@3f4fba9 for details. In the meantime, building on an M1 Mac does work so long as you pass
-x :desktop:test. Actually running the app, e.g. with./gradlew :desktop:run, however, does not and will not work until that bug is fixed. In any case, it will be easier to get Bisq building on M1 with this PR merged.