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

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

Closed
wants to merge 1 commit into from

Conversation

akiraly
Copy link
Contributor

@akiraly akiraly commented Oct 15, 2023

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

  • Review Contribution Guidelines
  • Make sure that all commits are signed off to indicate that you agree to the terms of Developer Certificate of Origin.
  • 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.
  • Check "Allow edit from maintainers" option in pull request so that additional changes can be pushed by Gradle team
  • Provide integration tests (under <subproject>/src/integTest) to verify changes from a user perspective
  • Provide unit tests (under <subproject>/src/test) to verify logic
  • Update User Guide, DSL Reference, and Javadoc for public-facing changes
  • Ensure that tests pass sanity check: ./gradlew sanityCheck
  • 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 akiraly requested review from a team as code owners October 15, 2023 21:19
@bot-gradle bot-gradle added the from:contributor PR by an external contributor label Oct 15, 2023
@cobexer
Copy link
Member

cobexer commented Nov 7, 2023

Thank you for your interest in Gradle.

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

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 cobexer added the pending:feedback Indicates that changes or additional info are required, and the issue will be closed without them label Nov 7, 2023
@akiraly akiraly force-pushed the akiraly-master branch 2 times, most recently from 8fe68af to 748c38f Compare November 14, 2023 23:24
@akiraly
Copy link
Contributor Author

akiraly commented Nov 14, 2023

@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 akiraly force-pushed the akiraly-master branch 2 times, most recently from 684da6a to c8d68a4 Compare November 15, 2023 21:12
@akiraly akiraly changed the title Allow overriding the default Gradle library repository through a new system property Allow overriding the default Gradle library repository through a new Gradle property Nov 15, 2023
@cobexer cobexer added in:repository-declarations declaring repositories and filtering in:build-environment Gradle, project, system and environment properties, CLI flags and removed to-triage pending:feedback Indicates that changes or additional info are required, and the issue will be closed without them labels Nov 21, 2023
@cobexer
Copy link
Member

cobexer commented Nov 21, 2023

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.

@ov7a ov7a linked an issue Nov 21, 2023 that may be closed by this pull request
@cobexer cobexer added the 👋 team-triage Issues that need to be triaged by a specific team label Nov 22, 2023
Copy link

gitstream-cm bot commented Nov 22, 2023

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%

@akiraly
Copy link
Contributor Author

akiraly commented Nov 22, 2023

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 akiraly force-pushed the akiraly-master branch 2 times, most recently from 12ce195 to 8baf6c1 Compare November 29, 2023 20:21
@leonard84
Copy link
Member

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?

@reinsch82 reinsch82 marked this pull request as draft January 11, 2024 13:33
@big-guy big-guy added in:plugin-development and removed in:repository-declarations declaring repositories and filtering labels Jan 11, 2024
@big-guy big-guy removed the in:build-environment Gradle, project, system and environment properties, CLI flags label Jan 11, 2024
…property called `org.gradle.internal.gradle.libs.repo.override` (previously overriding was only possible through the `GRADLE_LIBS_REPO_OVERRIDE` environment variable).

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

Signed-off-by: akiraly <kiralyattila.hu@gmail.com>
@akiraly
Copy link
Contributor Author

akiraly commented Jan 12, 2024

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

@leonard84
Copy link
Member

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.

@reinsch82 reinsch82 removed the 👋 team-triage Issues that need to be triaged by a specific team label Jan 24, 2024
@reinsch82 reinsch82 removed their assignment Jan 31, 2024
@reinsch82
Copy link
Contributor

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

@ljacomet ljacomet added 👋 team-triage Issues that need to be triaged by a specific team and removed to-triage labels Mar 12, 2024
@donat donat self-assigned this Mar 13, 2024
@donat
Copy link
Member

donat commented Mar 18, 2024

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.

@akiraly
Copy link
Contributor Author

akiraly commented Mar 18, 2024

@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?

@donat
Copy link
Member

donat commented Mar 18, 2024

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

Exactly.

Copy link
Member

@lkasso lkasso left a comment

Choose a reason for hiding this comment

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

Docs LGTM

@donat donat removed the 👋 team-triage Issues that need to be triaged by a specific team label Mar 20, 2024
@reinsch82 reinsch82 marked this pull request as ready for review March 20, 2024 14:50
@reinsch82 reinsch82 marked this pull request as draft March 20, 2024 14:51
@ov7a ov7a removed the to-triage label Apr 4, 2024
@donat donat removed their assignment Apr 17, 2024
@ov7a ov7a removed the to-triage label May 7, 2024
@donat
Copy link
Member

donat commented May 7, 2024

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 #29049.

@donat donat closed this May 7, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
from:contributor PR by an external contributor in:plugin-development
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Central configuration option for GRADLE_LIBS_REPO_OVERRIDE
10 participants