gradle icon indicating copy to clipboard operation
gradle copied to clipboard

Allow overriding the default Gradle library repository through a new Gradle property

Open akiraly opened this issue 2 years ago • 12 comments

Allow overriding the default Gradle library repository through a new Gradle property called org.gradle.internal.gradle.libs.repo.override (previously overriding was only possible through the GRADLE_LIBS_REPO_OVERRIDE environment variable).

The Gradle property can be more useful because it can be defined in the build itself (either through gradle.properties or programmatically)

Context

This can be useful for every project which needs to override the default Gradle library repository (typical in corporate settings, behind a firm firewall). While the environment variable works it is not convenient: each developer has to define the environment variable manually in his/her dev environment. The Gradle property is more practical because it can be defined inside the build and kept together with the rest of the build configuration in the source repository.

Contributor Checklist

  • [X] Review Contribution Guidelines
  • [X] Make sure that all commits are signed off to indicate that you agree to the terms of Developer Certificate of Origin.
  • [X] Make sure all contributed code can be distributed under the terms of the Apache License 2.0, e.g. the code was written by yourself or the original code is licensed under a license compatible to Apache License 2.0.
  • [X] Check "Allow edit from maintainers" option in pull request so that additional changes can be pushed by Gradle team
  • [x] Provide integration tests (under <subproject>/src/integTest) to verify changes from a user perspective
  • [x] Provide unit tests (under <subproject>/src/test) to verify logic
  • [X] Update User Guide, DSL Reference, and Javadoc for public-facing changes
  • [ ] Ensure that tests pass sanity check: ./gradlew sanityCheck
  • [x] Ensure that tests pass locally: ./gradlew <changed-subproject>:quickTest

Reviewing cheatsheet

Before merging the PR, comments starting with

  • ❌ ❓must be fixed
  • 🤔 💅 should be fixed
  • 💭 may be fixed
  • 🎉 celebrate happy things

akiraly avatar Oct 15 '23 21:10 akiraly

Thank you for your interest in Gradle.

The implementation does not match the proposed solution on the related issue.

  • https://github.com/gradle/gradle/issues/25840

Please rework your implementation to match the design.


The solution should use a Gradle property instead of a system property and provide tests to verify the functionality.

cobexer avatar Nov 07 '23 14:11 cobexer

@cobexer thanks for your comment.

I have updated the PR as requested. It is now looking for a Gradle property and I have copied 3 of the existing tests (that were using the env var for override) and modified them to use the new property.

Please check.

akiraly avatar Nov 14 '23 23:11 akiraly

Thank you for your contribution!

This PR has a valid DCO and tests. The relevant team for this area will confirm the design of the implementation choices.

cobexer avatar Nov 21 '23 13:11 cobexer

Change Summary

This PR is 89.82% new code.
Platform Added Lines % of Total Line Changes Deleted Lines % of Total Line Changes Files Changed % of Total Files Changed
bt_ge_build_cache 0 0% 0 0% 0 0%
build_infrastructure 0 0% 0 0% 0 0%
core_configuration 0 0% 0 0% 0 0%
core_execution 0 0% 0 0% 0 0%
core_runtime 0 0% 0 0% 0 0%
documentation 2 0.88% 1 0.44% 1 16.67%
extensibility 0 0% 0 0% 0 0%
gradle_enterprise 0 0% 0 0% 0 0%
ide 201 88.94% 22 9.73% 5 83.33%
jvm 0 0% 0 0% 0 0%
kotlin_dsl 0 0% 0 0% 0 0%
release_coordination 0 0% 0 0% 0 0%
software 0 0% 0 0% 0 0%

gitstream-cm[bot] avatar Nov 22 '23 21:11 gitstream-cm[bot]

This PR in its current state would already be really helpful for people sitting behind corporate firewalls and proxies.

However I agree with @SidB3's comment on #25840: in some cases users need full control over how that maven repository (used by DefaultGradleApiSourcesResolver) gets configured (for example to setup the authentication correctly).

So the support for that use case could be added (either as part of this PR or in a followup PR) with some DSL additions that would allow users to provide a custom Action<? super MavenArtifactRepository> which then could be used during DefaultGradleApiSourcesResolver creation. That's why I did some refactoring on DefaultGradleApiSourcesResolver to allow that kind of customization to happen. But I didn't include any DSL changes.

My questions to the reviewer(s) from the Gradle Team are: do you agree with this train of thought? Would you be supportive to have the custom-maven-config-action-DSL?

akiraly avatar Nov 22 '23 22:11 akiraly

My only question would be in regards to portability. Will this only work if all users and CI use the same OS type? Does it support ~ or other placeholders for user.home? Would it make sense to make ok aware properties org.gradle.internal.gradle.libs.repo.override.windows/org.gradle.internal.gradle.libs.repo.override.linux/... ?

What takes precedence, env or the new property?

leonard84 avatar Jan 10 '24 15:01 leonard84

@leonard84 thanks for taking a look at this. Let me try to answer your questions out of order:

  1. Currently the new gradle property takes precedence over the environment variable. But I can reverse it if needed.

  2. Not sure how dependent this is on OS type. The aim of this PR is to make it possible to override the repo URL more easily (so it can be done as part of the project setup instead of asking every developer to manually set an environment variable). So I would assume the URL wouldn't be OS dependent. At least in our organization it would be the same regardless of the client OS. However, because this is normal gradle property, it can be redefined/overdefined in various places (for example in command line or in a gradle.properties file in the GRADLE_USER_HOME dir). So I personally don't think OS dependent properties would add much.

  3. Regarding the placeholders. No explicit support was added for anything like that. Note that the existing env variable doesn't support those either. Also my assumption was that these would still mostly be used with remote URL-s (http addresses) not with file url-s.

akiraly avatar Jan 12 '24 13:01 akiraly

Thanks @akiraly for some reason I was assuming that it was referring to a file system location, if it is mainly intended for web resources, then I agree that my points are mostly irrelevant.

leonard84 avatar Jan 12 '24 13:01 leonard84

@akiraly thanks for your efforts with this PR. We are currently not able to to merge this due to ongoing discussions on how to address the underlying issue.

reinsch82 avatar Jan 31 '24 11:01 reinsch82

We've picked up the ball and resumed the conversation on this pull request. We still have a few things to sort out, but my understanding is that we actually want to get rid of the property altogether. The Gradle libs URLs was a workaround because the localGroovy dependency is handled in Gradle in a special way. We aim to simplify Gradle and making localGroovy resolvable/configurable via dependency management.

donat avatar Mar 18 '24 07:03 donat

@donat thanks, that would be the cleanest. I assume that way the global repository configuration (url, authentication, etc...) would apply to these resolutions as well?

akiraly avatar Mar 18 '24 08:03 akiraly

I assume that way the global repository configuration (url, authentication, etc...) would apply to these resolutions as well?

Exactly.

donat avatar Mar 18 '24 10:03 donat

To be explicit, we're not planning to accept this PR. Instead, we'll aim for the improving the dependency resolution for localGroovy(). Let's continue the conversation at https://github.com/gradle/gradle/issues/29049.

donat avatar May 07 '24 13:05 donat