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

Add version catalog support for Gradle #6249

Merged
merged 7 commits into from
Mar 13, 2023
Merged

Conversation

alcere
Copy link
Contributor

@alcere alcere commented Dec 2, 2022

Hello from the GitHub Android mobile team!

As part of a client apps hackathon we decided to give it a shot and add version catalog support for Gradle projects.

This functionality has been requested by some users here: #3121, and we are planning to start using version catalog as well as soon as Dependabot supports it :P.

We have added support for version catalogs defined using a toml file (more info), for libraries and plugins using value versions and referenced versions, so this changes add support for a toml file like:

[versions]
corektx = "1.7.0"
lifecycleRuntime = "2.3.1"
composeUi = "1.2.0"
jUnitVersion = "4.13.2"
androidxJunit = "1.1.3"
espresso = "3.5.0"

[libraries]
corektx = { module = "androidx.core:core-ktx", version.ref = "corektx" }
lifecycleRuntimeKtx = { module = "androidx.lifecycle:lifecycle-runtime-ktx", version.ref = "lifecycleRuntime" }

compose-activity = { module = "androidx.activity:activity-compose", version = "1.3.1" }
compose-ui = { module = "androidx.compose.ui:ui", version.ref = "composeUi" }
compose-ui-tooling = { module = "androidx.compose.ui:ui-tooling-preview", version.ref = "composeUi" }
compose-material = { module = "androidx.compose.material:material", version = { strictly = "[1.2.0, 1.3.1[", prefer="1.3.0" } }

test-junit = { module = "junit:junit", version.ref = "jUnitVersion" }
test-andoridx-junit = { module = "androidx.test.ext:junit", version.ref = "androidxJunit" }
test-compose-ui = { module = "androidx.compose.ui:ui-test-junit4", version.ref = "composeUi" }
test-compose-ui-tooling = { module = "androidx.compose.ui:ui-tooling", version.ref = "composeUi" }
test-compose-manifest = { module = "androidx.compose.ui:ui-test-manifest", version.ref = "composeUi" }
test-espresso-Core = { module = "androidx.test.espresso:espresso-core", version.ref = "espresso" }

[bundles]
compose = ["compose-activity", "compose-ui", "compose-ui-tooling", "compose-material"]

[plugins]
kotlinter = { id = "org.jmailen.kotlinter", version = "3.11.0" }

Testing

In order to guide our development we have used the steps defined in here to use the dry-run script pointing to this test repo we created.

Note

We realize that this code won't reach the quality bar to be considered as a candidate to be merged (Ruby is not our field of expertise), but we will be more than happy to react to your feedback and keep working on it!

BIG THANKS

To @jurre who has been helping us on this task, sharing advices, techniques and ideas!

Closes #3121.

@alcere alcere requested a review from a team as a code owner December 2, 2022 14:33
@alcere alcere marked this pull request as draft December 2, 2022 14:33
@alcere alcere marked this pull request as ready for review December 3, 2022 13:20
@alcere alcere marked this pull request as draft December 5, 2022 16:25
@alcere alcere marked this pull request as ready for review December 16, 2022 15:36
Copy link

@trevjonez trevjonez left a comment

Choose a reason for hiding this comment

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

As is this would cover the bulk of projects using version catalogs. But surely as things scale up and out we would see projects that have multiple catalogs or non conventional file layouts in use.

gradle/lib/dependabot/gradle/file_fetcher.rb Outdated Show resolved Hide resolved
@jeffwidman
Copy link
Member

👋 from the Dependabot team. Thanks for this great PR... it is definitely on our radar, although no one has had time to review it just yet. TBH, for myself it may be a few more weeks as I'm juggling a few things here at the beginning of the quarter and this is a larger change that will take a bit of time to review. But if no one else on the team gets to it first, I expect to be able to get to it within a few weeks.

Copy link
Contributor

@deivid-rodriguez deivid-rodriguez left a comment

Choose a reason for hiding this comment

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

This is pretty awesome! I made a first pass on reviewing.

My main concern is that when sharing version declarations between multiple dependencies, I think Dependabot may create a different PR for each dependency that uses the reference, all of them with the same contents? Is this correct?

gradle/lib/dependabot/gradle/file_fetcher.rb Outdated Show resolved Hide resolved
gradle/lib/dependabot/gradle/file_parser.rb Outdated Show resolved Hide resolved
gradle/lib/dependabot/gradle/file_parser.rb Outdated Show resolved Hide resolved
gradle/lib/dependabot/gradle/file_parser.rb Show resolved Hide resolved
gradle/lib/dependabot/gradle/file_parser.rb Outdated Show resolved Hide resolved
gradle/lib/dependabot/gradle/file_parser.rb Show resolved Hide resolved
gradle/spec/dependabot/gradle/file_fetcher_spec.rb Outdated Show resolved Hide resolved
@dsvensson
Copy link

Would it be more representable to look at the gradle lockfile? The libs.versions.toml file doesn't know about transitive dependencies (but maybe dependabot crawls that graph?), nor if these are updated via gradle version overrides.

@trevjonez
Copy link

Would it be more representable to look at the gradle lockfile? The libs.versions.toml file doesn't know about transitive dependencies (but maybe dependabot crawls that graph?), nor if these are updated via gradle version overrides.

FWIW: In the 8 years I have been using Gradle, I have never worked on a project that used Gradle lock files. I suspect this is more related to how mobile builds and deployments differ from a backend system CI/CD pipeline. Typically we run tools that scan and monitor the fully resolved tree before releasing. IE snyk.io

@dsvensson
Copy link

dsvensson commented Feb 13, 2023

FWIW: In the 8 years I have been using Gradle, I have never worked on a project that used Gradle lock files. I suspect this is more related to how mobile builds and deployments differ from a backend system CI/CD pipeline. Typically we run tools that scan and monitor the fully resolved tree before releasing. IE snyk.io

This is because lockfiles was made available with 7.0 at 9 April 2021 so for 6 years and 2 months of those 8 years this feature wasn't available. Added to that, IDEA didn't really support it in the beginning, so maybe bump that to the first 7 of your 8 years. I know it's underused, but that doesn't change anything. It is the only way to avoid false positives, just wanted note that, could be a future improvement. Hopefully some future supply chain poisoning with characteristics that would make it preventable with lockfiles and dependency verification happens, so broad adoption happens.

@trevjonez
Copy link

FWIW: In the 8 years I have been using Gradle, I have never worked on a project that used Gradle lock files. I suspect this is more related to how mobile builds and deployments differ from a backend system CI/CD pipeline. Typically we run tools that scan and monitor the fully resolved tree before releasing. IE snyk.io

This is because lockfiles was made available with 7.0 at 9 April 2021 so for 6 years and 2 months of those 8 years this feature wasn't available. Added to that, IDEA didn't really support it in the beginning, so maybe bump that to the first 7 of your 8 years. I know it's underused, but that doesn't change anything. It is the only way to avoid false positives, just wanted note that, could be a future improvement. Hopefully some future supply chain poisoning with characteristics that would make it preventable with lockfiles and dependency verification happens, so broad adoption happens.

Was it really only so recently? Surprised but does explain it.
Makes me wonder the format difference between nebula and gradle's built in lockfile is.
https://github.com/nebula-plugins/gradle-dependency-lock-plugin

That one has been available for a long time, though likely hard to say how widely used it is outside of netflix.

@deivid-rodriguez
Copy link
Contributor

I'll be working on this PR next week. Will push up my initial review suggestions, test out the code here a bit more, and in general try move this PR forward.

@alcere
Copy link
Contributor Author

alcere commented Feb 19, 2023

I'll be working on this PR next week. Will push up my initial review suggestions, test out the code here a bit more, and in general try move this PR forward.

@deivid-rodriguez sorry for taking so long to get you back 😓! I've been quite busy all this time. Do you mind if I address your comments? I have some spare time this week so I'd like to keep on working on this pull request.

@deivid-rodriguez
Copy link
Contributor

@deivid-rodriguez sorry for taking so long to get you back 😓! I've been quite busy all this time. Do you mind if I address your comments? I have some spare time this week so I'd like to keep on working on this pull request.

No worries and great having you back in this PR :), thanks for addressing my comments. I see that the PR appears as out of date and a lot of old commits appear in its timeline, I think it needs a rebase?

Can you answer this question from my first review? I lost context now and I haven't yet got to reviewing this again but at the time I was not clear about it:

My main concern is that when sharing version declarations between multiple dependencies, I think Dependabot may create a different PR for each dependency that uses the reference, all of them with the same contents? Is this correct?

@alcere
Copy link
Contributor Author

alcere commented Feb 20, 2023

My main concern is that when sharing version declarations between multiple dependencies, I think Dependabot may create a different PR for each dependency that uses the reference, all of them with the same contents? Is this correct?

That's a great question @deivid-rodriguez , do you mean something like this?

[versions]
composeUi = "1.2.0"

[libraries]
compose-ui = { module = "androidx.compose.ui:ui", version.ref = "composeUi" }
compose-ui-tooling = { module = "androidx.compose.ui:ui-tooling-preview", version.ref = "composeUi" }
test-compose-ui = { module = "androidx.compose.ui:ui-test-junit4", version.ref = "composeUi" }
test-compose-ui-tooling = { module = "androidx.compose.ui:ui-tooling", version.ref = "composeUi" }
test-compose-manifest = { module = "androidx.compose.ui:ui-test-manifest", version.ref = "composeUi" }

@deivid-rodriguez
Copy link
Contributor

Yes, I think that's what I meant.

@alcere
Copy link
Contributor Author

alcere commented Feb 20, 2023

Yes, I think that's what I meant.

You are very right, it does exactly that, I will look into the issue, thanks for the heads up!

@deivid-rodriguez
Copy link
Contributor

@alcere I had another look at this today and I think this is actually fine. We already have logic in place in the Gradle ecosystem to be able to group PRs together when they are updating a "property" so I think this is ok as is! 🎉

I also reviewed the PR, cleaned up git history and fixed a few bugs while I was at it. Were you planning on pushing any more changes? If not, I can push my cleanup and I think this would be mostly ready! 💪

@alcere
Copy link
Contributor Author

alcere commented Feb 21, 2023

@alcere I had another look at this today and I think this is actually fine. We already have logic in place in the Gradle ecosystem to be able to group PRs together when they are updating a "property" so I think this is ok as is! 🎉

I also reviewed the PR, cleaned up git history and fixed a few bugs while I was at it. Were you planning on pushing any more changes? If not, I can push my cleanup and I think this would be mostly ready! 💪

That's great news! Please go ahead!

@deivid-rodriguez deivid-rodriguez force-pushed the alcere/hack/gradle_toml branch 2 times, most recently from 4a659b7 to 0e81cb6 Compare February 21, 2023 19:55
@deivid-rodriguez
Copy link
Contributor

I pushed my changes. These include:

  • Cleanup git history into a handful of meaningful commits (removed style amendment commits and things like that).
  • Made some code simplifications and some style changes to make the implementation more "standard Ruby".
  • I added support for dependency libraries specified as group:artifact:version or dependency plugins specified as id:version.
  • I fixed an issue where Dependabot would crash when dependencies included the string "version" or the string "ref".

@yeikel
Copy link
Contributor

yeikel commented Feb 28, 2023

If possible, let's please add a test case to cover this #6742

@deivid-rodriguez
Copy link
Contributor

@yeikel The error you reported at #6742 was indeed still happening. I added a spec to make sure dependencies specified like that are ignored for now, and don't cause a crash in the update process like the one reported.

@deivid-rodriguez deivid-rodriguez force-pushed the alcere/hack/gradle_toml branch 2 times, most recently from 7979c07 to 015d9a8 Compare March 10, 2023 19:46
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.

Add support for Gradle Version Catalogs (Gradle 7.0)
9 participants