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

Build script tweaks #3485

Merged
merged 5 commits into from Apr 25, 2024
Merged

Build script tweaks #3485

merged 5 commits into from Apr 25, 2024

Conversation

3flex
Copy link
Contributor

@3flex 3flex commented Feb 3, 2024

Pretty low value stuff, but it tidies things up a little here and there.

Easiest to review commit by commit.

@3flex 3flex force-pushed the build-tweaks branch 3 times, most recently from 23e6910 to 135dc9f Compare February 3, 2024 10:59
@adam-enko
Copy link
Contributor

Hi @3flex, thanks for the contribution.

I've had a quick scan through and I've applied some of these changes in my own PRs. It makes sense to split them into a separate PR, so thanks for creating this.

Updating the scripts to use the new Kotlin Assignment feature is a more notable change however. Dokka needs to support a wide range of Gradle versions, and using Kotlin Assignment requires Gradle 8.2+. We can use Kotlin Assignment in our own build scripts, but not in the tested projects, and probably not in the docs (but I'll check further).

languageVersion.set(JavaLanguageVersion.of(11))
languageVersion = JavaLanguageVersion.of(11)
Copy link
Contributor

Choose a reason for hiding this comment

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

This version of Dokkatoo isn't connected to anything at the moment, and probably won't be worked on for a while. While editing it won't hurt, it also isn't necessary, so can you remove all of the changes in the dokka-runners/dokkatoo dir please?

val integrationTestPreparation by tasks.registering {
tasks.register("integrationTestPreparation") {
Copy link
Contributor

Choose a reason for hiding this comment

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

Can you explain more about this change? Personally I prefer the delegated accessor version over the string-based version.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It's just shorter and simpler. If you prefer the other way I'll revert.

@whyoleg
Copy link
Contributor

whyoleg commented Apr 8, 2024

Hey @3flex !
Do you still willing to work on this PR?
If so, could you please:

  • rebase on latest master (some changes could be applied already there by @adam-enko)
  • revert changes of .set to = in integration tests projects - tests for those projects are executed with different Gradle versions where property assignment via = doesn't exist
  • revert changes in dokkatoo - as stated above - this project is not connected for now

GPP redirects to JCenter when it can't find a requested artifact. This is
usually not what should happen. Setting Maven first means the order of
resolution is Maven Central -> GPP -> JCenter intsead of GPP -> JCenter ->
Maven Central.
Copy link
Contributor

@whyoleg whyoleg left a comment

Choose a reason for hiding this comment

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

Thank you!
Sorry for the long review, somehow missed the notification :)

@whyoleg whyoleg merged commit 262e220 into Kotlin:master Apr 25, 2024
21 checks passed
@3flex 3flex deleted the build-tweaks branch April 25, 2024 22:51
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.

None yet

3 participants