Support Gradle multi-project builds #583

LouisBoudreau · 2022-12-02T19:35:04Z

Description

The goal of this PR is to add support for Gradle multi project builds. To do so I implemented the suggestion provided in this issue that recommended the use of an init script.

The approach used is different from the other opened PR to fix this issue.

I also upgraded to the latest version of the gradle plugin.

Tests

I have split the gradle fixtures, one single project and one multi project. I also added some tests for the multi-project implementation.

jonabc

Thanks for opening this! I'm excited to see the gradle source getting updated.

😓 apologies if any of my questions are simplistic, I don't use gradle at all in my day-to-day development work and am pretty unfamiliar with how it works.

Along with the comments left on the change, a few higher level comments:

can you move .gitignore back to test/fixtures/gradle? It looks like both single and multi project fixtures use the same gitignore, and it would be preferable to keep the file closer to root
based on ☝️ , can you remove test/fixtures/gradle/.gradle from this change? I don't believe that folder is meant to be checked in based on the .gitignore file, but it wasn't ignored because .gitignore was moved to the different fixture folders
can you update the gradle documentation with anything that users need to know for how to use the new multi-project support, including the new gradle.project config value?

lib/licensed/sources/gradle.rb

jonabc · 2022-12-04T20:13:35Z

lib/licensed/sources/gradle.rb

          Dependency.new(
            name: name,
            version: package["version"],
            path: config.pwd,
            executable: gradle_executable,
            configurations: configurations,
+            project: project,


is it possible for different projects in a multi-project build to have different versions of the same dependency?

If yes, this value should be part of the name: argument rather than being added as a new property value.

lib/licensed/sources/gradle.rb

.github/workflows/test.yml

lib/licensed/sources/gradle.rb

jonabc

Just to call out - I appreciate that moving code around so that the logic to run gradle is encapsulated in the new runner class, but all the code moving around makes this change a little hard for me to review and figure out what changes are functional vs what is refactoring.

An easy review in this repo is pretty important to me as I tend to be the only maintainer for the project. The contribution model in this repo has historically looked like

someone adds or updates some piece of logic, or fixing a bug
once everything is working, they don't drop back in until there's something else broken or could be otherwise improved

I'm ok with that model and I REALLY appreciate the community support, but it means that I really need to know the code that ships with licensed so I can support it if I get a bug report that noone else is available to fix.

I think I've gotten to the bottom of this change and better understand what it's doing, but in the future I'd appreciate if you could try to limit the scope of changes and prefer multiple smaller, targeted changes vs a single big change that adds or changes functionality as well as refactoring code for readability.

I've left a few comments, I think the only comments that I'd like to see resolved before merging this are the ones related to finding the license URLs and passing the url into the dependencies in enumerate_dependencies

lib/licensed/sources/gradle.rb

jonabc · 2022-12-21T04:05:54Z

lib/licensed/sources/gradle.rb

-      def self.add_gradle_license_report_plugins_block(gradle_build_file)
+      # Prefixes the gradle command with the project name for multi-build projects.
+      def format_command(command)
+        if config.source_path != config.pwd


I don't think this will ever be true now that you're relying on specifying multiple apps via configuration rather than using a special configuration key. Does this function still make sense to use or can it be removed?

This function is still relevant, here is an example of when:

Let's say my project has the following structure:

project │ settings.gradle │ build.gradle │ licensed.yml │ └──/app │ build.gradle

And the licensed.yml file content is:

sources: gradle: true apps: - source_path: ./app

Then since the config file is at the root of the project and I specified the apps source path, the conditionconfig.source_path != config.pwd is true.

The gradle command will be run in the subproject context like so app:<command>

Does this makes sense to you ?

Does this makes sense to you ?

Let me resay what I think you're explaining and let me know if I got it right?

When a command for the app subproject is run from the project directory, the gradle command needs to be prefixed with the project name. Is that correct? So to run a command on the app subproject while in the project directory, you'd need to run app:<command> instead of just command?

If so, I think I get the purpose but the usage might not be correct based on how licensed works elsewhere in the app code. Licensed changes the current directory to an app's source_path when evaluating that app. Also, config.pwd is a shim around Pathname.pwd and doesn't return the directory containing the licensed configuration file. Unless you're manually changing the current directory, config.source_path == config.pwd for the duration of enumerating and evaluating dependencies for an app.

It looks like Gradle::Runner#run changes the directory to it's @root_path value, which is set at the creation of gradle_runner to config.pwd. The value of config.pwd at that point should be equivalent to config.source_path though, so it's probably not actually changing the directory.

I could definitely have missed something here or misunderstood the intentions of prefixing the command with the project name though 🤷 .

You are right, I applied the necessary changes

Thanks for taking the time to explain!

lib/licensed/sources/gradle.rb

script/source-setup/gradle

LouisBoudreau · 2022-12-22T02:31:14Z

You are absolutely right!

While applying the changes for the init script I saw an opportunity to refactor a section of code and did so without thinking about the impact it would have on the review. I apologize, in the future I will make sure to scope the changes so they are smaller and more enjoyable to review!

On another note, I wanted to point out my admiration for the work you do for Licensed. This is a really a beautiful project with excellent quality. Licensed is a project all the more relevant now that open source is more and more widespread in our codebases.

Let me know if I can help 😄

jonabc

a few more comments, this is getting closer 🚀

lib/licensed/sources/gradle.rb

jonabc

This looks like it's just stuck up now by how to find the right project name. Also I won't be able to run CI checks until you resolve the merge conflicts on the test file.

Sorry to cause the conflicts, I went through the test.yml workflow file to replace manual usage of actions/cache with setup actions' built-in caching wherever possible.

…der gradle versions

jonabc

🚀 thanks so much for working through all of the feedback on this 🙇 !

@LouisBoudreau

### Added - Licensed supports Cocoapods as a dependency source (:tada: @LouisBoudreau #584) - Licensed supports Gradle multi-project builds (:tada: @LouisBoudreau #583) ### Fixed - Licensed no longer crashes when run with Bundler >= 2.4.0 (:tada: @JoshReedSchramm #597) ### Changed - BREAKING: Licensed no longer ships executables with releases (#586) - BREAKING: Licensed no longer includes support for Go <= 1.11

jonabc reviewed Dec 4, 2022

View reviewed changes

LouisBoudreau force-pushed the master branch from 4be2c8e to 73c388f Compare December 6, 2022 19:15

LouisBoudreau commented Dec 6, 2022

View reviewed changes

lib/licensed/sources/gradle.rb Outdated Show resolved Hide resolved

LouisBoudreau commented Dec 6, 2022

View reviewed changes

.github/workflows/test.yml Outdated Show resolved Hide resolved

LouisBoudreau commented Dec 14, 2022

View reviewed changes

lib/licensed/sources/gradle.rb Outdated Show resolved Hide resolved

jonabc reviewed Dec 21, 2022

View reviewed changes

jonabc reviewed Dec 22, 2022

View reviewed changes

lib/licensed/sources/gradle.rb Outdated Show resolved Hide resolved

lib/licensed/sources/gradle.rb Show resolved Hide resolved

jonabc reviewed Jan 3, 2023

View reviewed changes

lib/licensed/sources/gradle.rb Outdated Show resolved Hide resolved

jonabc reviewed Jan 3, 2023

View reviewed changes

LouisBoudreau added 7 commits January 3, 2023 13:08

Support gradle multiproject builds

f4fdcfe

Add example in documentation

930d632

Use gradle/gradle-build-action@v2 to test different gradle versions

0cf6211

Only add --no-configuration cache gradle versions supporting it

722b418

Apply requested changes

5fa17fe

Apply requested changes

4f3ba1d

Use gradle CLI to format command

f71db05

LouisBoudreau force-pushed the master branch from 5de76dd to f71db05 Compare January 3, 2023 19:42

LouisBoudreau added 4 commits January 4, 2023 11:46

Fix tests

11cb45b

Fix merge error

015c852

Handle older versions of gradle

f498ce8

Dynamically set version of gradle-license-report plugin to support ol…

21a5509

…der gradle versions

jonabc approved these changes Jan 5, 2023

View reviewed changes

jonabc merged commit 45eabf1 into github:master Jan 5, 2023

This was referenced Jan 5, 2023

Support gradle multi-project #470

Closed

Add support for Gradle multi-module projects. #301

Closed

jonabc mentioned this pull request Jan 5, 2023

Release 4.0.0 #604

Merged

LouisBoudreau mentioned this pull request Jan 11, 2023

Fix gradlew location when trying to location a gradle executable #610

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Support Gradle multi-project builds #583

Support Gradle multi-project builds #583

LouisBoudreau commented Dec 2, 2022 •

edited

Loading

jonabc left a comment

jonabc Dec 4, 2022

jonabc left a comment

jonabc Dec 21, 2022

LouisBoudreau Dec 22, 2022

jonabc Dec 22, 2022

LouisBoudreau Jan 4, 2023

LouisBoudreau commented Dec 22, 2022

jonabc left a comment

jonabc left a comment

jonabc left a comment

Support Gradle multi-project builds #583

Support Gradle multi-project builds #583

Conversation

LouisBoudreau commented Dec 2, 2022 • edited Loading

Description

Tests

jonabc left a comment

Choose a reason for hiding this comment

jonabc Dec 4, 2022

Choose a reason for hiding this comment

jonabc left a comment

Choose a reason for hiding this comment

jonabc Dec 21, 2022

Choose a reason for hiding this comment

LouisBoudreau Dec 22, 2022

Choose a reason for hiding this comment

jonabc Dec 22, 2022

Choose a reason for hiding this comment

LouisBoudreau Jan 4, 2023

Choose a reason for hiding this comment

LouisBoudreau commented Dec 22, 2022

jonabc left a comment

Choose a reason for hiding this comment

jonabc left a comment

Choose a reason for hiding this comment

jonabc left a comment

Choose a reason for hiding this comment

LouisBoudreau commented Dec 2, 2022 •

edited

Loading