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

SOLR-17602: Per-Module Dependency Locking #2925

Conversation

malliaridis
Copy link
Contributor

@malliaridis malliaridis commented Dec 23, 2024

https://issues.apache.org/jira/browse/SOLR-17602

Description

To improve the developer experience of the dependency management, we are looking for solutions to make the dependency lock files more developer-friendly / readable.

Solution

This solution is based on #2915 which introduces a platform module for aligning the dependency version across our project via constraints.

This PR removes the carrotsearch dependencycheck plugin and falls back to the default gradle tasks for generating lockfiles. It defines a new task that can be used via gradlew resoslveAndLockAll --write-locks. This task generates or updates existing gradle.lockfile in each module.

Additional gradle configuration has been added to lock all module configurations by default. The task resolveAndLockAll will then filter and lock only the resolveable dependencies.

Additional Notes

When generating lockfiles under Windows, the lockfiles may use CRLF instead of LF and display difference in IDE. If you have configured your Git correctly, you may ignore the changes.

Additionally, the caching in CI/CD uses versions.lock for index, which with this PR would not exist anymore. Since this PR uses per-module dependency lockfiles, this commit updates the workflows to use all lockfiles as keys.

I have also noticed that when running ./gradlew assemble the lockfiles are ignored for some reason and different libraries are included. Adding the platform module explicitly via implementation platform(project(":platform") solves the issue. So it may be necessary to include the platfform module in all modules and submodules explicitly to ensure the consistency in the future.

Tests

Effects tested with lucene version update in main while this PR was open.

Checklist

Please review the following and check all that apply:

  • I have reviewed the guidelines for How to Contribute and my code conforms to the standards described there to the best of my ability.
  • I have created a Jira issue and added the issue ID to my pull request title.
  • I have given Solr maintainers access to contribute to my PR branch. (optional but recommended, not available for branches on forks living under an organisation)
  • I have developed this patch against the main branch.
  • I have run ./gradlew check.
  • I have added tests for my changes.
  • I have added documentation for the Reference Guide

The platform module includes all dependencies from the
version catalog as constraints and is added to the root
modules as API for transitive inheritance of the constraints.
Removes carrot-search dependencychecks plugin
@malliaridis
Copy link
Contributor Author

@dsmiley does this go to the right direction in regards to your ticket?

In the gradle documentation and in general, lockfiles
are generated via ./gradlew dependencies --write-locks.

This commit fixes the issue where the dependencies
task is used instead of the resolveAndLockAll for
lockfile generation and the lockfiles are not
properly updated.
@dsmiley dsmiley requested a review from dweiss January 4, 2025 19:01
Copy link
Contributor

@dsmiley dsmiley left a comment

Choose a reason for hiding this comment

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

does this go to the right direction in regards to your ticket?

Yes but definitely doesn't solve them. I like that this PR makes it easy to know if a dependency is on a certain configuration/classpath without having to google the gradle command to accomplish the same. And if we change the dependencies, we might notice unanticipated changes.

The error-prone aspect is unfortunate. I wonder if we're disabling error-prone by default in the wrong way.

+1 to this PR.

gradle/documentation/render-javadoc.gradle Outdated Show resolved Hide resolved
@janhoy
Copy link
Contributor

janhoy commented Jan 6, 2025

+1 More gradle-default behavior is better, if we can make it jive with our project requirements.. Especially when the plugin in use (carrot) is not that actively maintained.

@malliaridis malliaridis marked this pull request as ready for review January 11, 2025 13:34
@malliaridis malliaridis requested a review from dsmiley January 11, 2025 13:35
Comment on lines +56 to +60
In case it is a transitive dependency that is not directly used, you can simply add it to `libs.versions.toml` as you
would with any other dependency. The dependency resolution approach defined in `:platform` will handle the rest.
Don't forget to add a `# @keep` note with a reference to the vulnerable version and CVE that is fixed with the explicit
definition of the library and new version. This way it is easier to keep track of unreferenced dependencies in our
libraries toml file, and we can clean them up once the libraries using the modules are updated.
Copy link
Contributor

Choose a reason for hiding this comment

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

Ooooh, now I see what this is for.

@@ -20,6 +20,7 @@ apply plugin: 'java-library'
description = 'SQL Module'

dependencies {
implementation platform(project(':platform'))
Copy link
Contributor

Choose a reason for hiding this comment

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

The explicit-ness of this is good, I think, even if it means an additional line everywhere. Maybe another different approach would be for the build to insert these automatically. Either is fine.

This task dependency seems to not have the expected effect.
…king

# Conflicts:
#	dev-tools/scripts/smokeTestRelease.py
#	versions.lock
@malliaridis malliaridis merged commit 7605593 into apache:main Jan 22, 2025
6 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants