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

Use modern API to get property values #2007

Closed
wants to merge 3 commits into from

Conversation

@Goooler
Copy link
Member Author

Goooler commented Jan 17, 2024

Some of these APIs are introduced in Gradle 6.2, we probably should merge this after bumping the min requirement.

@Goooler Goooler marked this pull request as draft January 18, 2024 01:30
@jbduncan
Copy link
Member

Nice! LGTM. It makes sense to me to wait until we depend on Gradle 6.2.

@nedtwigg
Copy link
Member

Awesome, thanks @Goooler! Want to bump the minimum Gradle in this PR? That will get it to pass tests, and then we can merge. 6.2 is very old, I'm fine to bump our requirements to that (or newer if needed)

@Goooler
Copy link
Member Author

Goooler commented Jan 22, 2024

That would be a breaking change, I'm not sure if we should do it in minor releases. Might be in the next major update.

@nedtwigg
Copy link
Member

We have bumped the required Gradle version in the past without bumping the major version. As long as we call it out in the release notes I think it is fine.

@nedtwigg
Copy link
Member

Thanks for being so cautious!

@Goooler Goooler force-pushed the replace-property-getter-setter branch from 29b240e to a7d6459 Compare January 22, 2024 08:18
@Goooler Goooler marked this pull request as ready for review January 22, 2024 08:18
@Goooler Goooler marked this pull request as draft January 22, 2024 08:57
@nedtwigg
Copy link
Member

Gradle on Java 11 is failing with

com.diffplug.gradle.spotless.GitRatchetGradleTest > singleProjectExhaustive(int) [1] 0 FAILED (16.1s)

  org.gradle.testkit.runner.UnexpectedBuildFailure: Unexpected build execution failure in /tmp/junit4180957295393577931 with arguments [spotlessCheck]
  
  Output:
  
  FAILURE: Build failed with an exception.
  
  * Where:
  Build file '/tmp/junit4180957295393577931/build.gradle' line: 2
  
  * What went wrong:
  An exception occurred applying plugin request [id: 'com.diffplug.spotless']
  > Failed to apply plugin 'com.diffplug.spotless'.
     > Cannot obtain value from provider of Gradle property 'spotlessModern' at configuration time.
       Use a provider returned by 'forUseAtConfigurationTime()' instead.

@nedtwigg
Copy link
Member

@nedtwigg
Copy link
Member

If bumping to 6.3 or 6.4 or whatever fixes the issue, we should just do that.

@Goooler
Copy link
Member Author

Goooler commented Jan 23, 2024

Some tests failed due to without forUseAtConfigurationTime using, we need something like

private fun <T> Provider<T>.forUseAtConfigurationTimeCompat(): Provider<T> =
  if (GradleVersion.current() < GradleVersion.version("6.5")) {
      // Gradle < 6.5 doesn't have this function.
      this
  } else if (GradleVersion.current() < GradleVersion.version("7.4")) {
      // Gradle 6.5 - 7.3 requires this function to be called.
      @Suppress("DEPRECATION")
      this.forUseAtConfigurationTime()
  } else {
      // Gradle >= 7.4 deprecated this function in favor of not calling it (became no-op, and will eventually nag).
      // https://docs.gradle.org/current/userguide/upgrading_version_7.html#for_use_at_configuration_time_deprecation
      this
  }

to compact various Gradle versions, it's a bit annoying. I would hold this before bumping min Gradle requirement to 7.4

@Goooler Goooler closed this Jan 23, 2024
@Goooler Goooler deleted the replace-property-getter-setter branch January 23, 2024 07:07
@nedtwigg
Copy link
Member

nedtwigg commented Jan 23, 2024

Do we get anything positive by switching to these APIs? is it related to project isolation (#1979)?

If we do get anything useful, it might be worth dealing with the annoyance. Seems like there's only 4 .gets and and 6 .isPresent by my count.

@Goooler
Copy link
Member Author

Goooler commented Jan 23, 2024

It would be a suggestion for migration from Gradle side, but every property getting works well with CC, I believe we have no much emergency to do it for now.

@nedtwigg
Copy link
Member

Roger, thanks for looking into this!

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.

3 participants