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

Update gradle build files and tested gradle versions in CI #630

Merged
merged 7 commits into from
Feb 25, 2023

Conversation

jonabc
Copy link
Contributor

@jonabc jonabc commented Feb 21, 2023

current gradle in CI was recently changed to 8.x, and the update of 7.9 -> 8.0 looks to have been causing issues due to a new deprecation with the versions of the kotlin jvm and gradle plugin that were used in the test fixtures' build.gradle files. I made a few changes that I'm hoping will fix this.

  • add previous current, 7.6, to run in CI
  • update 6.4 -> 6.9.3
  • bump plugins in build.gradle files causing CI failures

current gradle in CI is 8.x
- add previous current, 7.6, to run in CI
- update 6.4 -> 6.9.3
- bump plugins in build.gradle files causing CI failures
it only looks like it was needed due to how tests
were set up with the fixtures folder set to the
multi_project folder rather than the expected source path
1. runtime -> runtimeOnly
2. create custom configurations that extend from
  the real configurations being used to work around
  Gradle core configurations not being resolvable
@jonabc
Copy link
Contributor Author

jonabc commented Feb 22, 2023

I needed to make deeper changes to the code and tests to get things fully working for Gradle 8. I was having trouble getting tests to work as I was expecting them to, and the following commits were made to try and get things in a more predictable place.

  1. 807d6fb updated the test file to simplify app configurations and to properly set fixtures folders for multi_project scenarios
  2. 4797fc5 then built on the fixtures fix by no longer passing directory paths into the gradle runner class. This matches conventions and expectations used by other sources and simplifies the solution a bit.
    • at this point I also noticed that determining the project path wasn't necessary when configuration source paths were pointing at each individual project in a multi-project Gradle setup rather than the root. Calling the Gradle executable is expensive and removing the call to determine the current project path should speed up the source a bit

At this point I was able to fix the problems that surfaced after updating plugin versions

  1. c5c5c2d replaces the runtime configuration with runtimeOnly and then updates the init script to create custom configurations that extend from the users requested configurations. Using the runtimeOnly configuration in the init script caused problems because the configuration has canBeResolved = false which broke everything. The error message suggested creating new configurations that extend the existing ones to work around the issue, which was faster than trying to dig into the Gradle code itself and it's MANY layers of interfaces, abstract classes, delegate classes and all sorts of enterprise-y complication goodness.

Along the way I also put in a bug fix and a small code cleanup

  1. 84731f3 fixed a correctness issue where string comparisons were used for version checks. this is a bug waiting to happen when gradle reaches major version 10, and is fixed by performing version checks with Gem::Version
    irb(main):001:0> "10" > "7"
    => false
    irb(main):002:0> Gem::Version.new("10") > Gem::Version.new("7")
    => true
    
  2. 81be51a reuses a constant to reduce the likelihood of getting path mismatches in future changes to this source, and to more clearly tie the code that creates license CSV output with the code reading the CSV.

@jonabc
Copy link
Contributor Author

jonabc commented Feb 22, 2023

@LouisBoudreau 👋 would you be able to take a look at this change, specifically c5c5c2d? It looks like that change is working, but I'm not sure if it's the best solution.

c5c5c2d replaces the runtime configuration with runtimeOnly and then updates the init script to create custom configurations that extend from the users requested configurations. Using the runtimeOnly configuration in the init script caused problems because the configuration has canBeResolved = false which broke everything. The error message suggested creating new configurations that extend the existing ones to work around the issue, which was faster than trying to dig into the Gradle code itself and it's MANY layers of interfaces, abstract classes, delegate classes and all sorts of enterprise-y complication goodness.

@LouisBoudreau
Copy link
Contributor

Yes, I will check this out friday!

Copy link
Contributor

@LouisBoudreau LouisBoudreau left a comment

Choose a reason for hiding this comment

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

I made a really simple proposition here that could be merged into this PR before merging this one.

Thanks for cleaning up the code and updating the gradle source 🙇

lib/licensed/sources/gradle.rb Show resolved Hide resolved
@jonabc
Copy link
Contributor Author

jonabc commented Feb 25, 2023

I made a really simple proposition here that could be merged into this PR before merging this one.

To close the collaboration loop, I closed that PR and will not be bringing any of the changes proposed into this PR before merging. It's a good proposal and I'm going to open an issue to re-evaluate the default Gradle configurations used for dependency enumeration, but that change likely needs to coincide with a major version bump since there's a chance that it could be a breaking change for users. If conversation on the issue shows that it wouldn't be a breaking change then it's also really easy to remove runtimeOnly as a default configuration in the future 👍

@jonabc jonabc merged commit 39bec7d into master Feb 25, 2023
@jonabc jonabc deleted the update-gradle-build branch February 25, 2023 19:55
jonabc added a commit that referenced this pull request Feb 25, 2023
### Added
- Reviewed and ignored configuration lists support matching on versions and version ranges (#629)

### Fixed
- Licensed should more reliably source dependencies from Gradle >= 8.0 (#630)
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.

2 participants