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

Fix improper system prop instead of project prop use in docs #29231

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

consp1racy
Copy link

@consp1racy consp1racy commented May 20, 2024

Fixes #18087.

Context

I was interested in providers.gradleProperty and got sidetracked. Apparently this one letter caused much confusion so here's a quick fix.

EDIT: I shan't wear this PR as a contributed to open source badge.

EDIT 2: This should be followed up by a PR that moves the examples from Gradle properties section to Project properties section, with adjustments, because that's what they are about.

Contributor Checklist

Check.

Signed-off-by: Eugen Pechanec <[email protected]>
@consp1racy consp1racy requested a review from a team as a code owner May 20, 2024 23:34
@bot-gradle bot-gradle added from:contributor PR by an external contributor to-triage labels May 20, 2024
@gitstream-cm gitstream-cm bot requested a review from lkasso May 20, 2024 23:37
@gitstream-cm gitstream-cm bot added 1 min review a:documentation Documentation content labels May 20, 2024
Copy link

gitstream-cm bot commented May 20, 2024

📊 Changes by Platform: this PR is 50% new code

1 platform was affected

See details
Platform Added Lines % of Total Line Changes Deleted Lines % of Total Line Changes Files Changed % of Total Files Changed
documentation 1 50% 1 50% 1 100%

@@ -425,7 +425,7 @@ include::sample[dir="snippets/tutorial/gradleProperties/groovy",files="build.gra
*Example 4:* Setting Gradle properties from the command line:
====
----
$ gradle -DgradlePropertiesProp=commandLineValue
$ gradle -PgradlePropertiesProp=commandLineValue
Copy link
Contributor

Choose a reason for hiding this comment

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

That's not correct though. With -D you set system properties and Gradle properties. With -P you set project properties. Unfortunately, there is a big mix-up of terminology even within Gradle. For example with providers.gradleProperty you can not retrieve Gradle properties, but only project properties.

Copy link
Contributor

Choose a reason for hiding this comment

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

See for example #24491 and the linked forum topic for more information.

Copy link
Author

Choose a reason for hiding this comment

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

You can access system properties with System.getProperty or providers.systemProperty.

Let's not pollute providers.gradleProperty API with system properties. Or Gradle properties which are intended for Gradle itself. That's the nice thing about it, as opposed to project.property which returns extras, conventions, tasks and all kinds of stuff on top of actual project properties.


What's confusing is that the expected behavior you originally described actually behaves as expected. Instead, it would appear that this entire section of the docs is wrong because it calls project properties Gradle properties. :(

I propose merging this so all the examples in section Gradle properties work as intended (as project properties).

A follow up issue and PR could tackle separating a new moving examples to the Project properties section from the current Gradle properties section, where they belong.

Copy link
Contributor

Choose a reason for hiding this comment

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

Again, the example you modify here shows how to set a Gradle property.
You cannot set a Gradle property using -P, only using -D.

https://docs.gradle.org/current/userguide/build_environment.html#sec:project_properties shows how to set Project properties, which are set via -P or from system properties with a org.gradle.project. prefix (including from -D) or from environment variables with a ORG_GRADLE_PROJECT_ prefix.

https://docs.gradle.org/current/userguide/build_environment.html#sec:gradle_configuration_properties shows how to set Gradle properties, which are set via -D.

You cannot set a Gradle property like org.gradle.caching using -P, that just does not work. If you set it through -P you will be able to get it through providers.gradleProperty as it is a Project property and that method only delivers project properties. If you set the Gradle property through a gradle.properties file, it is set as Gradle property and project property which is why you can request it through providers.gradleProperty. But as described in the linked issue, if you for example set the Gradle property through the gradle.properties file and then change its value through the proper -D commandline switch so that it acutally has an effect, providers.gradleProperty still delivers the project property value set through the gradle.properties file, not the Gradle property that actually has an effect.

So again, the documentation is correct in showing -D for setting a Gradle property and using -P just does not work for setting it effectively.

Copy link
Author

Choose a reason for hiding this comment

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

So again, the documentation is correct in showing -D [...]

Note that all of the surrounding examples actually work with project properties, despite being listed under the Gradle properties section. My first goal is to make the example make sense in its surrounding context. We can deal with the rest later.

I've read what you had to say and I agree. But I have to remind you that the original issue was about using environment variables to define project and Gradle/system properties, and that works correctly. Inconsistencies of providers.gradleProperty are tracked in other issues, some of which you've linked.

Copy link
Contributor

Choose a reason for hiding this comment

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

Note that all of the surrounding examples actually work with project properties, despite being listed under the Gradle properties section

Exactly, and that is the error.
Getting a value of a Project Property should not be documented under the Gradle Properties heading.
Changing the -D to -P here just makes the docs even more wrong.
As I said in the linked ticket, even in the Gradle docs and unfortunately also the implementation the terms are mixed up, as providers.gradleProperty does not give you the value of a Gradle Property, just the value of a Project Property and even there only from the root project or up, but not from the actual subproject file.

A better fix would probably be to move the providers.gradleProperty examples up to the Project Properties chapter instead of making the Gradle Properties chapter even more wrong.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
a:documentation Documentation content from:contributor PR by an external contributor
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Some gradle properties are not settable by environment variable pattern or system property pattern
4 participants