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

Rework recent changes to dependency verification #1563

Merged
merged 3 commits into from Jun 22, 2018

Conversation

cbeams
Copy link
Member

@cbeams cbeams commented Jun 5, 2018

See individual commit comments for details, but in short, this reverts a recent commit that made our dependency arrangement unnecessarily complex, and updates the instructions for how to regenerate dependency verification hashes to avoid similar problems in the future.

This does the work that was intended to be done in commit
175e11d, but was done by first removing
the dependencyVerification block entirely, and then replacing it with
the output of the `calculateChecksums` task.

The entire process went like this:

 1. Remove existing dependencyVerification block from build.gradle
 2. Run `./gradlew -q calculateChecksums | grep -v network.bisq:bisq- >> build.gradle`
 3. Run `git diff` to see that only the checksums we expect to have
    changed have in fact changed (libdohj and bitcoinj in this case).
 4. Commit the changes (in this commit)

I'll update the instructions for the dependencyVerification block in a
subsequent commit to make this clearer in the future.
@cbeams cbeams requested a review from ManfredKarrer June 5, 2018 14:53
@cbeams cbeams self-assigned this Jun 5, 2018
@cbeams
Copy link
Member Author

cbeams commented Jun 5, 2018

It doesn't really come through in this pull request, but the best way to understand how much simpler this change is, is to run the following command to compare build.gradle now (in this pull request) to how build.gradle looked before the changes in commit 175e11d. You can see that by running the following command (with this pull request branch checked out):

$ git diff 175e11d81cfd353307af62d5c931cc25a7cc763f^ build.gradle

And here's that diff, just to make it easy:

$ git diff 175e11d81cfd353307af62d5c931cc25a7cc763f^ build.gradle
diff --git build.gradle build.gradle
index e1ce3fc19..1505817c5 100644
--- build.gradle
+++ build.gradle
@@ -67,7 +67,15 @@ dependencies {
 build.dependsOn installDist
 installDist.destinationDir = file('build/app')

-// generated with `./gradlew -q calculateChecksums | grep -v network.bisq:bisq-`
+// To update the `dependencyVerification` block below:
+//
+// 1. Remove the block entirely
+// 2. Replace the block with the following command:
+//
+//    ./gradlew -q calculateChecksums | grep -v network.bisq:bisq- >> build.gradle
+//
+// 3. Run `git diff` to verify that expected hashes have changed
+// 4. Commit the changes
 dependencyVerification {
     verify = [
         'org.controlsfx:controlsfx:b98f1c9507c05600f80323674b33d15674926c71b0116f70085b62bdacf1e573',
@@ -96,7 +104,7 @@ dependencyVerification {
         'com.google.code.findbugs:jsr305:c885ce34249682bc0236b4a7d56efcc12048e6135a5baf7a9cde8ad8cda13fcd',
         'com.google.guava:guava:36a666e3b71ae7f0f0dca23654b67e086e6c93d192f60ba5dfd5519db6c288c8',
         'com.google.inject:guice:9b9df27a5b8c7864112b4137fd92b36c3f1395bfe57be42fedf2f520ead1a93e',
-        'network.bisq.libdohj:libdohj-core:797a65f00e5ddde1b119748a00c6d75bd6e0c60b65b9cfcdb2cff94bfb5c9abc',
+        'network.bisq.libdohj:libdohj-core:cef7db8a2032ffbc229ccacc29b7d13e8ee1daf1cd60ee8f988cdfa60a041d8e',
         'com.github.JesusMcCloud.netlayer:tor:3896950c56a41985f901ff9475524ac162cba18b2d5a0ed39810b20ddaf5128a',
         'org.jetbrains.kotlin:kotlin-stdlib-jdk8:841b021d62fc007ce2883963ff9440d5393fb1f6a0604ed68cd016afcaf02967',
         'com.github.MicroUtils:kotlin-logging:7dbd501cc210d721f730d480c53ee2a6e3c154ae89b07dc7dee224b9c5aca9eb',
@@ -118,7 +126,7 @@ dependencyVerification {
         'commons-logging:commons-logging:daddea1ea0be0f56978ab3006b8ac92834afeefbd9b7e4e6316fca57df0fa636',
         'javax.inject:javax.inject:91c77044a50c481636c32d916fd89c9118a72195390452c81065080f957de7ff',
         'aopalliance:aopalliance:0addec670fedcd3f113c5c8091d783280d23f75e3acb841b61a9cdb079376a08',
-        'com.github.bisq-network.bitcoinj:bitcoinj-core:ad39183983285d00082cb01638428913db976fa8fd7494c7e223fc81c74d578a',
+        'com.github.bisq-network.bitcoinj:bitcoinj-core:05c94cc68f1524ed08f5aa815f5f09d99a3aae5ab277527d7729ee74d0aece13',
         'com.lambdaworks:scrypt:9a82d218099fb14c10c0e86e7eefeebd8c104de920acdc47b8b4b7a686fb73b4',
         'com.google.zxing:core:11aae8fd974ab25faa8208be50468eb12349cd239e93e7c797377fa13e381729',
         'com.cedricwalter:tor-binary-geoip:7fc7b5ebf80d65ec53d97dd8d3878b8d2c85dc04f3943e5e85e7ba641655492b',

Copy link
Member

@ManfredKarrer ManfredKarrer left a comment

Choose a reason for hiding this comment

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

I tested it with setting libdohj to the prev. commit version (dbc23c67) in common.
It would be expected that the hash of libdohj and btcj change but they stay the same.

The desktop build would fail as well then due wrong hashes.

Tried different variants also with including the libdohj with version dbc23c67 in desktop but no luck. Any idea? Probably some stupid mistake from my side...

@cbeams
Copy link
Member Author

cbeams commented Jun 5, 2018

Did you get the bit about removing the entire block first? The problem is that dependency verification happens every time you run any Gradle command. So even if you're trying to regenerate the dependency verification hashes after a dependency upgrade, the build fails because the old hashes are still in place. This is why blowing away the dependencyVerification block first, and then replacing it with a newly-generated one avoids the problem you were having. Unless I'm really missing something with your original problem. But everything works as expected in this pull request branch. Read through the commit comments (if you haven't already), hopefully it makes sense.

@ManfredKarrer
Copy link
Member

ManfredKarrer commented Jun 5, 2018

Yes I deleted it.

If you change locally back to latest libdohj version and then try to make the hashes again you see the problem. It does not change the hash and a desktop build afterwards fails.

I used that on common for testing:

compile('network.bisq.libdohj:libdohj-core:dbc23c67') {
        exclude(module: 'jsr305')
        exclude(module: 'slf4j-api')
        exclude(module: 'guava')
        exclude(module: 'protobuf-java')
    }

@ManfredKarrer
Copy link
Member

I have in my BSQ branch now a similar problem and I think the reason is that the resolving of core is done via jitpack, so we would need to first commit and push the new libdohj version in core. Then once desktop is resolving the dependencies it gets the correct version.

For development I would prefer to have a local resolve.
Is adding mavenLocal() to the repo the best approach? If so could we add mavenLocal as preferred resolve strategy?

@ManfredKarrer
Copy link
Member

I will merge that one now, my questions was going outside of the scope of that PR anyway...

@ManfredKarrer ManfredKarrer merged commit 84b346f into bisq-network:master Jun 22, 2018
@cbeams cbeams deleted the dependency-verification branch July 30, 2018 19:09
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants