-
Notifications
You must be signed in to change notification settings - Fork 29
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
Proof of concept: Gradle 8.3 #258
Conversation
@@ -59,7 +59,7 @@ class StagingRepositoryTransitioner(val nexusClient: NexusClient, val retrier: A | |||
private fun assertRepositoryInDesiredState(repository: StagingRepository, vararg desiredStates: StagingRepository.State) { | |||
if (repository.state !in desiredStates) { | |||
throw RepositoryTransitionException( | |||
"Staging repository is not in desired state ${desiredStates.contentToString()}: $repository. It is unexpected. Please check " + | |||
"Staging repository is not in desired state ${desiredStates.contentDeepToString()}: $repository. It is unexpected. Please check " + |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this a backward compatible change? When was it introduced (before 6.2?)?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
contentToString
seems actually Kotlin 1.4, and we're targeting Kotlin 1.3. Not sure how it was compiling before. contentDeepToString
is Kotlin 1.1
It's very likely some kind of inference / overload resolution change.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Btw, as I see in our regression tests the plugin can already be built with 8.3-rc-1 and the change it to "just" make the project itself "buildable" with it?
If you think, the changes are safe (and backward compatible at runtime), it's ok for me to apply them to be able to keep up with the changes in Gradle even more :).
Btw. what will be the minimal Gradle version able to build the project? 8.0?
Sorry this was meant to be a draft only, for future reference. For now this is RC, let's at least wait for stable. Things can change significantly in RCs (if they are broken in RC1, they might be fixed in RC2). This PR might not even be necessary if we bump the minimum Gradle to 7.x, but that's a different convo. Let's get this 2.0 out first :)
The regression tests are running on Gradle 8.3-rc-1, still using the wrapper to build.
Not sure I get this question. Wrapper locks in a specific version. Using the plugin (applying it) in a (user) build depends on what we set explicitly as the minimum in the plugin code. |
Yes, I meant, that the currently built plugin (with 8.2) is at runtime compatible with (in) the 3rd-party projects built with 8.3-rc-1.
Yes, the wrapper keeps the things consistent. I was just wondering, if - due to that changes - the "builability" with the lower Gradle version is impossible (however, it was just a theoretical question, as 8.2+ should be enough for us). |
And yes, waiting for 8.3-final seems to be a good idea :-). |
@TWiStErRob Would it be easier with Gradle 8.5? |
I think 8.5 will be even harder, hopefully I can have a look this weekend. I'll try to carve a few hours for reviving these PRs. But please review closely after, because it's really hard to remember the context and rebase these with confidence. |
This will allow us to use Gradle 8.3+, but we'll have to live with the warning: