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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

Built-in gradle dependency verification #759

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

ialokim
Copy link
Collaborator

@ialokim ialokim commented Jun 1, 2021

As discussed in #693, this PR switches from the gradle witness plugin to the built-in gradle dependency verification. As before with the gradle witness approach, only the checksums of the dependencies are verified, not the GPG signatures. However, the metadata files (.pom) are now verified, too, and locally built dependencies are (should be) already automatically excluded. The only platform-dependent dependency is aapt2, I had to add the checksums for the windows and osx version manually. Let's see if the verification works properly in the CI As the verification worked in the CI, this should be ready to be merged 馃槃

@cla-bot cla-bot bot added the cla-signed 鉁旓笍 The Contributor Licence Agreement was signed by all contributors. label Jun 1, 2021
./gradlew -q calculateChecksums | grep -Ev "^(Skipping|Verifying)" | grep -Ev "files-2.1:|caches:transforms-3:|:build-tools:core-lambda-stubs.jar:|:platforms:android.jar:|-linux.jar:" > $WITNESS
# to clean up the file, remove ./gradle/verification-metadata.xml,
# run the command below and manually (re-)add checksums for missing operating systems windows, osx or linux for aapt2
# checksums can be computed after downloading the respective jars of https://maven.google.com/web/index.html?q=aapt2#com.android.tools.build:aapt2
Copy link
Owner

Choose a reason for hiding this comment

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

Its a pity we can't do that automatically. Would be nice to have that file reflect all changes and not grow bigger over time.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I agree, that would be better, but only adding new entries avoids removing manually added checksums as the ones for windows and osx for aapt. I think it will make sense anyway to look at the diff after running this command, so perhaps it is viable to remove old versions manually then. After all, this should be only updated after library upgrades.

Copy link
Owner

Choose a reason for hiding this comment

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

only adding new entries avoids removing manually added checksums as the ones for windows and osx for aapt.

but it won't help after we upgrade the gradle plugin as this will pull in new dependencies again that we would need to add manually, right?

perhaps it is viable to remove old versions manually then

maybe easier to re-add windows/mac versions if we even want to support those?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

only adding new entries avoids removing manually added checksums as the ones for windows and osx for aapt.

but it won't help after we upgrade the gradle plugin as this will pull in new dependencies again that we would need to add manually, right?

That's correct.

perhaps it is viable to remove old versions manually then

maybe easier to re-add windows/mac versions if we even want to support those?

We could certainly change the bash script to always remove the old verification data (as was done with the witness plugin before), but I would have to check if the order of the dependencies is deterministic to ensure sensible diffs.

Copy link

Choose a reason for hiding this comment

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

According to gradle/gradle#11664 the entries are sorted, so diffs should be fine

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cla-signed 鉁旓笍 The Contributor Licence Agreement was signed by all contributors.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants