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

Dependency versions fix #2380

Merged
merged 6 commits into from
Jan 8, 2024
Merged

Conversation

vitorpamplona
Copy link
Collaborator

Moves the version selection from resolutionStrategy to dependencyConstraints to make sure it is exported to maven correctly on publishing.

Type
Bug fix

Checklist

  • I have read and acknowledged the Code of conduct.
  • I have read the Contributing page.
  • I have signed the Google Individual CLA, or I am covered by my company's Corporate CLA.
  • I have discussed my proposed solution with code owners in the linked issue(s) and we have agreed upon the general approach.
  • I have run ./gradlew spotlessApply and ./gradlew spotlessCheck to check my code follows the style guide of this project.
  • I have run ./gradlew check and ./gradlew connectedCheck to test my changes locally.
  • I have built and run the demo app(s) to verify my change fixes the issue and/or does not break the demo app(s).

…traints to make sure it is exported to maven correctly.
# Conflicts:
#	common/build.gradle.kts
#	engine/build.gradle.kts
@vitorpamplona
Copy link
Collaborator Author

@jingtang10 👆

Copy link
Collaborator

@MJ1998 MJ1998 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 interesting.

One question: Can we add this constraint to project level build.gradle since we are adding it to all the libraries ?

@vorburger
Copy link
Member

@vitorpamplona how about moving more dependencies to the #2306 approach instead?

@vitorpamplona
Copy link
Collaborator Author

@vorburger I looked at Version Catalogs but I didn't find a way to explicitly force strict versions for our dependencies.

@MJ1998 The constraint block to specify versions seems to be required every time one of the modules declares the dependency on HAPI individually. It then places the correct <dependencyManagement> tag on the maven side. I think your idea would work if gradle exported hierarchical pom.xmls.

@vorburger
Copy link
Member

@vorburger I looked at Version Catalogs but I didn't find a way to explicitly force strict versions for our dependencies.

How do you mean? In

glide = "4.16.0"
there is a fixed version, right?

But I'm possibly missing the full problem of what this proposes to fix.

@vitorpamplona
Copy link
Collaborator Author

vitorpamplona commented Dec 21, 2023

No, that is just the preferred version. The strict flag tells dependency resolvers to NOT go for newer versions EVEN IF other dependencies use a newer version. Info: here

@vitorpamplona
Copy link
Collaborator Author

vitorpamplona commented Dec 21, 2023

I just found out that gradle does offer a !! notation for strict.

    implementation('org.slf4j:slf4j-api:[1.7, 1.8[!!1.7.25')

is equivalent to

    implementation('org.slf4j:slf4j-api') {
        version {
           strictly '[1.7, 1.8['
           prefer '1.7.25'
        }
    }

It's kinda hidden in the specs (maybe outdated?), but we can test this later on with the TOML syntax. I just don't know how to declare a proper constraints block in TOML so that it becomes a <dependencyManagement> block in maven.

@jingtang10
Copy link
Collaborator

@vorburger the problem we're trying to solve is to stop applications using our libraries from having to declare resolution strategies like in this file: https://github.com/google/android-fhir/pull/2380/files#diff-a645a2c44f3874493dc92c37e4dbbcd33cb5042edd2de9d9ce6259ffdbf33010

This is a pretty strong functional requirement. I would place this as higher priority than migrating to toml files.

So what I would suggest is to complete this fix first, and continue to migrate to toml wherever we can (it appears a partial migration is possible). And hopefully complete the migration soon with a way to declare constraints as @vitorpamplona mentioned in the above comment.

Copy link
Collaborator

@jingtang10 jingtang10 left a comment

Choose a reason for hiding this comment

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

thanks @vitorpamplona for fixing this - huge improvement.

a couple of suggestions. would be great to fix in this pr but can be in an immediate follow-up too.

buildSrc/src/main/kotlin/Dependencies.kt Outdated Show resolved Hide resolved
datacapture/build.gradle.kts Show resolved Hide resolved
@vorburger
Copy link
Member

It's kinda hidden in the specs (maybe outdated?), but we can test this later on with the TOML syntax. I just don't know how to declare a proper constraints block in TOML so that it becomes a block in maven.

I don't suppose those strictly and prefer (and reject and rejectAll) keywords documented here are what you are looking for?

@jingtang10 jingtang10 enabled auto-merge (squash) January 8, 2024 16:27
@jingtang10 jingtang10 merged commit 4e11525 into google:master Jan 8, 2024
3 checks passed
hugomilosz pushed a commit to hugomilosz/android-fhir that referenced this pull request Jan 17, 2024
* Moves the version selection from resolutionStrategy to dependencyConstraints to make sure it is exported to maven correctly.

* Removes unused dependency declarations.
sharon2719 pushed a commit to opensrp/android-fhir that referenced this pull request Feb 6, 2024
* Moves the version selection from resolutionStrategy to dependencyConstraints to make sure it is exported to maven correctly.

* Removes unused dependency declarations.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: Complete
Development

Successfully merging this pull request may close these issues.

4 participants