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

chore: re-enable deps validation #442

Open
wants to merge 3 commits into
base: main
Choose a base branch
from
Open

Conversation

jeqo
Copy link
Contributor

@jeqo jeqo commented Nov 9, 2023

Re-enable custom task to validate storage dependencies.

As a reminder this task collects dep versions used by each storage module, and compares it against versions used by core. It it contains the same dep but versions do not match, then fail build.

e.g. running the first commit on storage:gcs and azure reports conflicting libraries that are fixed in the following commits:

> Task :storage:gcs:validateDeps FAILED
Conflict found for checker-qual: 3.42.0 vs 3.37.0
Conflict found for error_prone_annotations: 2.24.1 vs 2.21.1

> Task :storage:azure:validateDeps FAILED
Conflict found for jackson-annotations: 2.13.5 vs 2.16.1
Conflict found for jackson-core: 2.13.5 vs 2.16.1
Conflict found for jackson-databind: 2.13.5 vs 2.16.1

@jeqo jeqo force-pushed the jeqo/re-enable-deps-validation branch from 9f0914e to 3a67ac0 Compare November 9, 2023 12:07
@jeqo jeqo marked this pull request as ready for review November 9, 2023 21:05
@jeqo jeqo requested a review from a team as a code owner November 9, 2023 21:05
build.gradle Outdated
Comment on lines 245 to 253
def storages = ["s3", "gcs", "azure"]
def ignoreDeps = ["e2e"]

storages.forEach { s ->
println "Validating modules for storage: $s"
def dependencyVersions = [:]
subprojects { subproject ->
if (ignoreDeps.contains(subproject.name)) return
if (s == subproject.name || !storages.contains(subproject.name)) {
Copy link
Contributor

Choose a reason for hiding this comment

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

How do you thinks: would it be possible to declare this task on the storage project-level so we don't need to filter and search (of course from the single definition)?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good idea, there's only common and core modules to add as deps to storage backends. See last commit, thanks!

@jeqo jeqo requested a review from ivanyu November 13, 2023 11:35
@ivanyu ivanyu self-assigned this Jan 2, 2024
@jeqo jeqo force-pushed the jeqo/re-enable-deps-validation branch 2 times, most recently from 3c8df33 to 3cbfc6b Compare March 13, 2024 11:16
@AnatolyPopov
Copy link
Contributor

What about just using

configurations.configureEach {
    resolutionStrategy {
        failOnVersionConflict()
    }
}

?

@jeqo jeqo force-pushed the jeqo/re-enable-deps-validation branch from 3cbfc6b to bbc15c6 Compare March 14, 2024 14:10
@jeqo
Copy link
Contributor Author

jeqo commented Mar 14, 2024

@AnatolyPopov if you reproduce the first commit in this PR, the following conflicts are found:

> Task :storage:azure:validateDeps FAILED
Conflict found for jackson-annotations: 2.13.5 vs 2.16.1
Conflict found for jackson-core: 2.13.5 vs 2.16.1
Conflict found for jackson-databind: 2.13.5 vs 2.16.1

FAILURE: Build failed with an exception.

* Where:
Build file '/mnt/aiven-home-dir/aiven/aiven-open/tiered-storage-for-apache-kafka/storage/build.gradle' line: 62

* What went wrong:
Execution failed for task ':storage:azure:validateDeps'.
> Dependency conflicts found!. Expression: conflictsFound. Values: conflictsFound = true

This is unfortunately not found by your suggested configuration. IIUC, as the storage modules are not dependent on core then the collision of libraries between these are not found by default tooling--hence the custom script.

@AnatolyPopov
Copy link
Contributor

@jeqo What's the scope of the dependencies you listed? And because of reversed dependecies those conflicts should be visible when you run :core:dependencies then

@jeqo jeqo force-pushed the jeqo/re-enable-deps-validation branch from bbc15c6 to d4fc6d8 Compare March 15, 2024 00:16
@jeqo
Copy link
Contributor Author

jeqo commented Mar 15, 2024

What's the scope of the dependencies you listed?

It's comparing dependency versions from runtimeClasspath.

And because of reversed dependecies those conflicts should be visible when you run :core:dependencies then

I'm running core:dependencies with the configuration provided before and still I can't observe the same validation. Could you provide a complete example with an out-of-the-box command that reports the same issues on this project? As an reference I added the output of running: ./gradlew storage:gcs:validateDeps storage:azure:validateDeps --continue on the first commit in the PR description.

@AnatolyPopov
Copy link
Contributor

Well, after looking into this more deeply I can say that default tooling does not find the conflict because there is no dependencies between core module and submodules of storage. However I believe we should have such a dependency for this use case specifically because the storage can not be used with core. The ideal scope for such a dependency would be maven's provided I guess but there is no such thing in Gradle since we are not using war plugin. So IMO the solution should be:

  1. Use the default tooling as I proposed before
  2. Create a providedRuntime scope similar to Grade war plugin e.g.
configurations {
    providedRuntime
    implementation.extendsFrom(providedRuntime)
}
  1. Add a dependency in :storage:core like this
    providedRuntime(project(":core"))

This kind of scope definition will not change the resulting distribution, e.g. :core dependencies will not be included into artifacts of submodules of :storage but it will make the tooling work properly. And I guess for connectors or any other similar plugins people are doing similar things. And same for web apps, since the container provides some libraries there.

@AnatolyPopov
Copy link
Contributor

@jeqo Please take a look #518

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.

None yet

3 participants