Update build scripts to conform KUP requirements #4767
Conversation
WalkthroughThe changes refactor the build configuration by replacing old plugin identifiers with new ones under the Changes
Possibly related PRs
Suggested reviewers
Note ⚡️ AI Code Reviews for VS Code, Cursor, WindsurfCodeRabbit now has a plugin for VS Code, Cursor and Windsurf. This brings AI code reviews directly in the code editor. Each commit is reviewed immediately, finding bugs before the PR is raised. Seamless context handoff to your AI code agent ensures that you can easily incorporate review feedback. Note ⚡️ Faster reviews with cachingCodeRabbit now supports caching for code and dependencies, helping speed up reviews. This means quicker feedback, reduced wait times, and a smoother review experience overall. Cached data is encrypted and stored securely. This feature will be automatically enabled for all accounts on May 16th. To opt out, configure ✨ Finishing Touches
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
osipxd
changed the title
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (3)
build-logic/src/main/kotlin/ktorbuild.kmp.gradle.kts (1)
24-26: Static Kotlin compiler options configuration.The compiler options now use fixed version numbers (
KotlinVersion.KOTLIN_2_0forapiVersionandKotlinVersion.KOTLIN_2_1forlanguageVersion), which simplifies version management. Please verify that all project modules are compatible with these static versions.build-settings-logic/src/main/kotlin/ktorsettings.kotlin-user-project.settings.gradle.kts (2)
151-168: Test Task Filtering Implementation
ThefilterTests()function excludes a comprehensive list of test patterns. This targeted exclusion is useful for avoiding flaky or resource-intensive tests, although maintaining documentation on why these particular patterns were chosen could help future maintainers.
170-180: Manifest Printing for Kotlin Compiler Embeddable JAR
TheprintManifest()function iterates over thekotlinCompilerClasspathconfiguration to locate and print the manifest file. One potential edge case is that if no matching manifest is found, calling.first()may throw an exception. Consider handling an empty list more gracefully (e.g., by logging a message).
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (11)
build-logic/settings.gradle.kts(1 hunks)build-logic/src/main/kotlin/ktorbuild.kmp.gradle.kts(1 hunks)build-logic/src/main/kotlin/ktorbuild/KtorBuildExtension.kt(0 hunks)build-logic/src/main/kotlin/ktorbuild/internal/Train.kt(0 hunks)build-settings-logic/build.gradle.kts(3 hunks)build-settings-logic/src/main/kotlin/conventions-dependency-resolution-management.settings.gradle.kts(0 hunks)build-settings-logic/src/main/kotlin/ktorsettings.dependency-resolution-management.settings.gradle.kts(1 hunks)build-settings-logic/src/main/kotlin/ktorsettings.kotlin-user-project.settings.gradle.kts(1 hunks)build-settings-logic/src/main/kotlin/ktorsettings.settings.gradle.kts(1 hunks)ktor-test-server/settings.gradle.kts(1 hunks)settings.gradle.kts(1 hunks)
💤 Files with no reviewable changes (3)
- build-settings-logic/src/main/kotlin/conventions-dependency-resolution-management.settings.gradle.kts
- build-logic/src/main/kotlin/ktorbuild/KtorBuildExtension.kt
- build-logic/src/main/kotlin/ktorbuild/internal/Train.kt
🔇 Additional comments (27)
build-settings-logic/src/main/kotlin/ktorsettings.settings.gradle.kts (1)
1-10: New centralized build settings configuration file created successfully.This new Kotlin script correctly applies the KUP-specific plugins (
ktorsettings.dependency-resolution-managementandktorsettings.kotlin-user-project) to both the root project and its included builds. This centralization enhances organization and maintainability as intended by the PR objectives.build-logic/settings.gradle.kts (2)
5-9: Consistent inclusion of centralized build settings.Including the build-settings-logic using
includeBuild("../build-settings-logic")ensures that the centralized KUP configuration is available to this build. This change is in line with the overall objective to consolidate dependency resolution management.
10-11: Updated plugin identifier.The plugin declaration has been updated from the legacy identifier to
ktorsettings, reflecting the new unified approach. Please confirm that all modules referencing the old identifier have been updated accordingly.ktor-test-server/settings.gradle.kts (1)
5-10: Aligned plugin identifier update in ktor-test-server.The change to use
ktorsettingsin the plugins block ensures consistency with the other build modules. This alignment improves maintainability and reduces potential configuration discrepancies across the project.build-logic/src/main/kotlin/ktorbuild.kmp.gradle.kts (1)
40-41: Verify removal of train build setup.The call to
setupTrain()has been removed from the configuration. Confirm that this removal is intentional and that any functionality previously provided by the train build setup is either no longer needed or has been migrated to the new configuration.settings.gradle.kts (2)
11-16: Updated plugin identifiers for unified dependency management.The plugins block now uses the new KUP identifiers—
ktorsettings,ktorsettings.develocity, andktorsettings.configuration-cache—to replace legacy identifiers. This update helps ensure that all parts of the build adopt a consistent configuration approach.
7-10: Centralized plugin management inclusion.The inclusion of the build-settings-logic via
includeBuild("build-settings-logic")correctly centralizes the configuration. This change supports the overall objective of enhancing the build scripts’ organization to meet KUP requirements.build-settings-logic/src/main/kotlin/ktorsettings.dependency-resolution-management.settings.gradle.kts (5)
1-6: File Header and Suppression Annotation
The header and the@file:Suppress("UnstableApiUsage")annotation are correctly placed. This helps silence warnings about unstable API usage in a controlled manner.
7-12: Plugin Management Block Using Custom Repository Configuration
ThepluginManagementblock is clear and leverages the customconfigureRepositories()function. Ensuring the repositories are consistently configured across both plugin and dependency resolution is a good design choice.
14-28: Dependency Resolution Management with Version Catalog Configuration
The definition of thedependencyResolutionManagementblock is well implemented. Conditionals (like checking for the existence oflibs.versions.toml) and the call todowngradeTestDependencies()ensure that version overrides are applied appropriately.
30-46: CustomconfigureRepositories()Function
This function centralizes repository configuration by including Google’s repositories, Maven Central, Maven Local, and a specialized repository with exclusive content filtering. Its structure enhances reuse and maintainability.
48-52:downgradeTestDependencies()Function for Compatibility Adjustments
The function retrieves thetest.jdkproperty and conditionally downgrades the version for Logback if using a JDK below 11. This conditional check is clear and addresses compatibility requirements effectively.build-settings-logic/build.gradle.kts (7)
5-10: File Annotation and Import Improvements
The file-level suppression along with the added imports (such as forDocsTypeand supporting functions) are correctly added. They set up the context for the DSL configurations that follow.
21-25: Updated Dependencies Block
Adding the implementation dependency on the Kotlin Gradle Plugin viaimplementation(libs.kotlin.gradlePlugin)is appropriate. This ensures the build logic has access to the required API and resolves versions consistently.
31-38: Workaround Region for Deprecated Warning in Kotlin Plugin
The commented region explains the rationale behind suppressing the "Unsupported Kotlin plugin version" warning. Including a link to the related issue and a reference to external sources (e.g., the Kotest PR) enhances understanding for future maintainers.
39-48: Definition ofkotlinDslPluginSourcesConfiguration
This configuration is well defined to download the original Gradle Kotlin DSL plugin source code. The use of attributes such as non-consumability and visibility is appropriate. Ensure that the variableexpectedKotlinDslPluginsVersionis defined in the build environment.
50-60: Definition ofkotlinDslPluginSourcesResolverConfiguration
The resolver configuration extends from the previously defined source configuration and uses the appropriate attribute to filter for sources. This setup is clean and aligns with best practices for Gradle configurations.
62-103: Task to Suppress Gradle Plugin Version Warning
ThesuppressGradlePluginVersionWarningtask is robust. It locates theEmbeddedKotlinPlugin.ktfile from the downloaded sources, patches it by replacing a warning with an info log, and adjusts visibility modifiers. The fallback to simply log a warning when the file isn’t found is a pragmatic choice.
105-110: Including the Task Output in the Main Source Set
Adding the output directory of the suppress task to the main source set viakotlin.srcDir(suppressGradlePluginVersionWarning)ensures that the patched file is considered during compilation. This integration is clear and effective.build-settings-logic/src/main/kotlin/ktorsettings.kotlin-user-project.settings.gradle.kts (8)
37-63: Configuration of Kotlin Build Properties
The propertieskotlinRepoUrl,kotlinVersion,kotlinLanguageVersion,kotlinApiVersion, andkotlinAdditionalCliOptionsare initialized lazily. Logging on access provides useful runtime feedback, and the parsing (including splitting the CLI options) is correctly handled.
65-69: Plugin Management Repository Configuration viakotlinDev()
Adding the custom repository with thekotlinDev()function helps to centralize repository configuration for Kotlin development. This approach improves consistency across the build.
71-88: Dependency Resolution Management with Conditional Version Catalog Overrides
The logic withindependencyResolutionManagementappropriately injects the Kotlin version into the version catalog and triggersoverrideKotlinxVersions()when in snapshot train mode. The use ofcheckNotNull(kotlinVersion)enforces the necessary configuration whenbuildSnapshotTrainis true.
90-123: After-Project Configuration for Kotlin Compiler Options
Thegradle.afterProjectblock configures Kotlin compiler options effectively by setting language and API versions, disabling-Werror, and appending additional compiler arguments. This detailed configuration ensures that the compiler behavior is consistent with project requirements.
124-131: Conditional Manifest Printing and Test Filtering for Snapshot Train Builds
The conditional block that prints the Kotlin compiler manifest for the "ktor-client" project and applies test filtering in snapshot train mode is clear. This ensures that known problematic tests are excluded while providing additional diagnostic output when appropriate.
133-143: Override of Kotlinx Library Versions for Snapshot Train Builds
TheoverrideKotlinxVersions()function encapsulates version override logic neatly. Utilizing a local helper function to reduce repetition is a sound approach. Note that using.get()on a Gradle property will throw an error if the property is missing; this is acceptable if the properties are required by the build chain.
145-149: CustomkotlinDev()Repository Function
ThekotlinDev()function cleanly adds a Maven repository based on thekotlinRepoUrlproperty. This modular approach supports multiple repository configurations without redundant code.
182-184: Logging Utility Function
The simplelogfunction prepends a tag (<KUP>) to log messages, which is a neat way to separate these logs from others in the build output.
build-settings-logic/src/main/kotlin/ktorsettings.kotlin-user-project.settings.gradle.kts
Show resolved
Hide resolved
osipxd
force-pushed
the
osipxd/kup-properties
branch
from
cdd4c2c to
8edd163
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (5)
build-settings-logic/settings.gradle.kts (1)
13-17: Enhancement for Kotlin Version Override
Adding the mechanism to override the Kotlin version using a Gradle property is a solid improvement for configurability. The code correctly checks for a non-null property before applying the override. Consider adding a debug log message (or an inline comment) to clarify what happens when no override is provided.build-settings-logic/src/main/kotlin/ktorsettings.dependency-resolution-management.settings.gradle.kts (2)
19-28: Conditional Version Catalog Setup
The version catalog’s creation conditionally loads fromlibs.versions.tomland appliesdowngradeTestDependencies()for compatibility. Consider adding an inline note (or enhancing documentation) on the impact of the dependency downgrade for clarity.
48-52: Test Dependency Downgrade Logic
ThedowngradeTestDependencies()function conditionally enforces a specific Logback version for JDK versions below 11—a smart move for ensuring compatibility. Adding a log statement when the downgrade is applied could further aid in debugging.build-settings-logic/build.gradle.kts (1)
62-103: Task to Suppress Plugin Version Warning
ThesuppressGradlePluginVersionWarningtask cleverly patches theEmbeddedKotlinPlugin.ktfile to change warning logs to info and to mark internal functions appropriately. To make the solution even more robust, consider adding error handling (e.g., try/catch) around file operations and ensuring that the replacement patterns are resilient to minor formatting differences.build-settings-logic/src/main/kotlin/ktorsettings.kotlin-user-project.settings.gradle.kts (1)
178-188: Manifest Printing for Kotlin Compiler
TheprintManifest()function extracts and prints the manifest fromkotlin-compiler-embeddable.jar, aiding in diagnostics. For enhanced clarity, consider logging the source file’s name along with its manifest contents or handling multiple manifest files if present.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (12)
build-logic/settings.gradle.kts(1 hunks)build-logic/src/main/kotlin/ktorbuild.kmp.gradle.kts(1 hunks)build-logic/src/main/kotlin/ktorbuild/KtorBuildExtension.kt(0 hunks)build-logic/src/main/kotlin/ktorbuild/internal/Train.kt(0 hunks)build-settings-logic/build.gradle.kts(3 hunks)build-settings-logic/settings.gradle.kts(1 hunks)build-settings-logic/src/main/kotlin/conventions-dependency-resolution-management.settings.gradle.kts(0 hunks)build-settings-logic/src/main/kotlin/ktorsettings.dependency-resolution-management.settings.gradle.kts(1 hunks)build-settings-logic/src/main/kotlin/ktorsettings.kotlin-user-project.settings.gradle.kts(1 hunks)build-settings-logic/src/main/kotlin/ktorsettings.settings.gradle.kts(1 hunks)ktor-test-server/settings.gradle.kts(1 hunks)settings.gradle.kts(1 hunks)
💤 Files with no reviewable changes (3)
- build-settings-logic/src/main/kotlin/conventions-dependency-resolution-management.settings.gradle.kts
- build-logic/src/main/kotlin/ktorbuild/KtorBuildExtension.kt
- build-logic/src/main/kotlin/ktorbuild/internal/Train.kt
🚧 Files skipped from review as they are similar to previous changes (5)
- build-settings-logic/src/main/kotlin/ktorsettings.settings.gradle.kts
- build-logic/settings.gradle.kts
- ktor-test-server/settings.gradle.kts
- build-logic/src/main/kotlin/ktorbuild.kmp.gradle.kts
- settings.gradle.kts
🧰 Additional context used
🧠 Learnings (1)
build-settings-logic/src/main/kotlin/ktorsettings.kotlin-user-project.settings.gradle.kts (1)
Learnt from: osipxd
PR: ktorio/ktor#4745
File: build-settings-logic/src/main/kotlin/ktorbuild.develocity.settings.gradle.kts:37-40
Timestamp: 2025-03-31T12:57:59.438Z
Learning: In Kotlin, `String?.toBoolean()` is a null-safe extension function that returns false when called on a null value. The implementation delegates to Java's `Boolean.parseBoolean(this)`, making it safe to use after operations that might return null (like `.orNull`).
🔇 Additional comments (23)
build-settings-logic/src/main/kotlin/ktorsettings.dependency-resolution-management.settings.gradle.kts (3)
7-12: Centralized Plugin Repository Configuration
Using the customconfigureRepositories()function within thepluginManagementblock centralizes repository settings, ensuring consistency across plugin resolution.
14-18: Unified Dependency Resolution Management
ReusingconfigureRepositories()in thedependencyResolutionManagementblock ensures both plugin and dependency resolution share the same repository configuration. This approach improves maintainability.
30-46: Well-Modularized Repositories Configuration
TheconfigureRepositories()function encapsulates repository settings (including content filtering and exclusive content for Ktor EAP) in a modular way, which enhances clarity and reuse across configurations.build-settings-logic/build.gradle.kts (7)
5-10: Suppression and Imports
The suppression directive along with the added imports for Gradle attributes and DSL support are standard and help manage unstable API usage effectively.
20-24: Dependency Configuration
The dependencies block now explicitly includes the Kotlin Gradle Plugin and develocity libraries. Ensure that the corresponding entries (e.g.,libs.kotlin.gradlePlugin) are correctly defined in your version catalog for consistency.
26-29: JVM Toolchain Setup
By specifyingjvmToolchain(21), the build is configured to use Java 21. Confirm that this choice is compatible with all components of the project.
31-38: Gradle Warning Workaround
The workaround section for suppressing the "Unsupported Kotlin plugin version" warning is well documented and clearly demarcated with context. It’s advisable to verify periodically if this fix remains necessary in newer Gradle versions.
39-48: kotlinDslPluginSources Configuration
The configuration forkotlinDslPluginSourcesis clear; however, please verify that the variableexpectedKotlinDslPluginsVersionis defined (or provided via Gradle properties) to avoid resolution issues.
50-60: kotlinDslPluginSourcesResolver Setup
Setting upkotlinDslPluginSourcesResolverwith the appropriate attributes helps extract the plugin sources as intended. This configuration is straightforward and effective.
105-109: SourceSet Configuration
Including the patched file’s output directory in the main Kotlin source set is intentional and helps integrate the fix. Please verify that this addition does not lead to duplication or conflicts with existing sources.build-settings-logic/src/main/kotlin/ktorsettings.kotlin-user-project.settings.gradle.kts (13)
34-38: Build Snapshot Train Property Initialization
The lazy initialization ofbuildSnapshotTrainleverages Kotlin’s null-safetoBoolean()(which returns false on null), making it robust against missing properties. This approach aligns with best practices and previous discussions on this pattern.
39-42: Kotlin Development Repository URL
The propertykotlinRepoUrlis lazily initialized and logs its value. This offers flexibility by allowing an external override of the Kotlin repository URL when needed.
43-46: Kotlin Version Property
InitializingkotlinVersionvia a Gradle property with immediate logging ensures transparency of the build configuration. The approach is concise and effective.
47-52: Kotlin Language Version Configuration
Converting the property to aKotlinVersionand logging the parsed version enhances clarity and provides useful feedback during builds.
53-58: Kotlin API Version Configuration
The lazy initialization ofkotlinApiVersionwith logging mirrors the language version setup and is both clear and reliable.
59-65: Additional CLI Options Parsing
Splitting the CLI options string into a list using a regular expression and defaulting to an empty list if absent ensures that extra compiler arguments are handled gracefully.
67-71: Plugin Management Repository Configuration
Delegating repository lookup to thekotlinDev()function abstracts the repository configuration, thereby simplifying management of the plugin repositories.
73-90: Dependency Resolution and Version Catalog Setup
The dependency resolution block conditionally applies the Kotlin version to the version catalog and enforces its presence whenbuildSnapshotTrainis active. This check is a prudent safeguard against misconfiguration.
92-133: Kotlin Compiler Options Configuration
In thegradle.afterProjectblock, Kotlin compiler options are configured by setting language/API versions, disabling the-Werrorflag, and appending extra compiler arguments. This configuration offers a balanced approach between strictness and flexibility.
135-145: Overriding kotlinx Dependency Versions
TheoverrideKotlinxVersions()function enforces specific versions for dependencies likeatomicfu,coroutines, andserialization. This targeted override is effective for managing compatibility and should help maintain a stable build environment.
147-151: Custom Repository Addition for Kotlin Development
ThekotlinDev()function useskotlinRepoUrlto conditionally add a Maven repository, neatly encapsulating the setup for a custom Kotlin development repository.
153-176: Test Filtering for Build Stability
ThefilterTests()function systematically excludes tests known to be flaky or overly stressful. This proactive filtering is vital for maintaining stability in the build pipeline.
190-192: Logging Utility
The simplelogfunction prefixes messages with a "[KUP]" marker, which is useful for filtering build logs. Its minimalist design suits the current needs; however, future growth might warrant a more flexible logging framework.
osipxd
force-pushed
the
osipxd/kup-properties
branch
from
8edd163 to
ab4f67e
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
build-settings-logic/build.gradle.kts (1)
62-103: Task for Suppressing Plugin Version Warning
ThesuppressGradlePluginVersionWarningtask smartly patches theEmbeddedKotlinPlugin.ktfile by converting warning logs to info and marking certain elements internal. Consider enhancing error handling (e.g., for file I/O issues) so that failure to patch does not silently impact the build process in the future.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (12)
build-logic/settings.gradle.kts(1 hunks)build-logic/src/main/kotlin/ktorbuild.kmp.gradle.kts(1 hunks)build-logic/src/main/kotlin/ktorbuild/KtorBuildExtension.kt(0 hunks)build-logic/src/main/kotlin/ktorbuild/internal/Train.kt(0 hunks)build-settings-logic/build.gradle.kts(3 hunks)build-settings-logic/settings.gradle.kts(1 hunks)build-settings-logic/src/main/kotlin/conventions-dependency-resolution-management.settings.gradle.kts(0 hunks)build-settings-logic/src/main/kotlin/ktorsettings.dependency-resolution-management.settings.gradle.kts(1 hunks)build-settings-logic/src/main/kotlin/ktorsettings.kotlin-user-project.settings.gradle.kts(1 hunks)build-settings-logic/src/main/kotlin/ktorsettings.settings.gradle.kts(1 hunks)ktor-test-server/settings.gradle.kts(1 hunks)settings.gradle.kts(1 hunks)
💤 Files with no reviewable changes (3)
- build-settings-logic/src/main/kotlin/conventions-dependency-resolution-management.settings.gradle.kts
- build-logic/src/main/kotlin/ktorbuild/KtorBuildExtension.kt
- build-logic/src/main/kotlin/ktorbuild/internal/Train.kt
🚧 Files skipped from review as they are similar to previous changes (5)
- build-logic/settings.gradle.kts
- ktor-test-server/settings.gradle.kts
- build-settings-logic/src/main/kotlin/ktorsettings.settings.gradle.kts
- build-logic/src/main/kotlin/ktorbuild.kmp.gradle.kts
- settings.gradle.kts
🧰 Additional context used
🧠 Learnings (1)
build-settings-logic/src/main/kotlin/ktorsettings.kotlin-user-project.settings.gradle.kts (1)
Learnt from: osipxd
PR: ktorio/ktor#4745
File: build-settings-logic/src/main/kotlin/ktorbuild.develocity.settings.gradle.kts:37-40
Timestamp: 2025-04-03T10:10:40.262Z
Learning: In Kotlin, `String?.toBoolean()` is a null-safe extension function that returns false when called on a null value. The implementation delegates to Java's `Boolean.parseBoolean(this)`, making it safe to use after operations that might return null (like `.orNull`).
🔇 Additional comments (29)
build-settings-logic/settings.gradle.kts (3)
10-10: Proper Use of Suppression Annotation
The use of@Suppress("UnstableApiUsage")is appropriate here given the reliance on unstable APIs in the dependency resolution configuration.
15-19: Dynamic Kotlin Version Override
The addition of a dynamic override for the Kotlin version via the Gradle property (kotlin_version) is a flexible enhancement. Please ensure that all downstream projects are updated to expect this behavior.
22-25: Custom Maven Repository Configuration
Using the Gradle propertykotlin_repo_urlto conditionally add a Maven repository (named "KotlinDev") is a neat solution to support custom repository configurations. Be sure that the provided URL is validated to prevent potential resolution issues.build-settings-logic/src/main/kotlin/ktorsettings.dependency-resolution-management.settings.gradle.kts (5)
5-6: Appropriate Suppression in the New Script
Placing@file:Suppress("UnstableApiUsage")at the top is well justified given the experimental APIs used in this script.
7-12: Clean Plugin Management Configuration
ThepluginManagementblock correctly delegates repository setup toconfigureRepositories(), which centralizes and simplifies repository declarations for plugin resolution.
14-28: Robust Dependency Resolution Management
Defining the version catalog and conditionally loading the TOML file—and callingdowngradeTestDependencies()—ensures that dependency versions remain consistent. This structure aids maintainability and clarity.
30-46: Consolidated Repository Configuration
TheconfigureRepositories()function aggregates various repository sources (Google, Maven Central, local Maven, and a dedicated Ktor EAP repository using exclusive content filtering). This consolidation enhances clarity and reduces duplication.
48-52: Conditional Downgrade of Test Dependencies
ThedowngradeTestDependencies()function is a clever strategy to enforce compatibility for JDK versions below 11 by downgrading Logback. Verify periodically that the required versions remain valid as dependencies evolve.build-settings-logic/build.gradle.kts (5)
21-23: Updated Dependency Declarations
The newly added dependencies (including the Kotlin Gradle Plugin and develocity libraries) ensure that the build-logic module remains in sync with the project’s requirements. Confirm that these versions are compatible with the overall system.
31-38: Workaround Documentation for Gradle Warning
The detailed comment block explaining the workaround for the Gradle Kotlin DSL plugin warning (referencing the known Gradle issue) is very helpful. It provides clear guidance about why this patch is necessary and its origin.
39-48: Kotlin DSL Plugin Sources Configuration
DefiningkotlinDslPluginSourcesto download the original Gradle Kotlin DSL plugin source is an inventive approach. Make sure that the variableexpectedKotlinDslPluginsVersion(referenced here) is defined elsewhere in the build so that the correct version is fetched.
50-60: Kotlin DSL Plugin Sources Resolver Setup
The configuration ofkotlinDslPluginSourcesResolverusing the SOURCES attribute ensures that the source artifacts are correctly resolved. This setup is clear and concise.
105-110: Integration of Patched Sources into Source Sets
Adding the patched sources directory to the main Kotlin source set ensures that the modifications take effect during compilation. This is an effective way to incorporate the workaround seamlessly.build-settings-logic/src/main/kotlin/ktorsettings.kotlin-user-project.settings.gradle.kts (16)
5-32: Comprehensive Documentation and Context
The header comment provides extensive context and clear documentation for all the configurable properties (likekotlin_version,kotlin_repo_url, etc.) and build modes (Ktor K2 and Ktor Train). This level of detail is very beneficial for maintainability and user understanding.
34-38: Lazy Initialization ofbuildSnapshotTrain
The property is lazily initialized while logging its value, and it leverages Kotlin’s null-safetoBoolean()extension correctly. This implementation is both concise and safe.
39-42: Lazy Initialization ofkotlinRepoUrl
Retrieving and logging thekotlin_repo_urlproperty facilitates custom repository management. This implementation is straightforward and effective.
43-46: Dynamic Retrieval ofkotlinVersion
The approach used for obtaining thekotlin_versionproperty is clear and well-implemented. Logging the value aids in troubleshooting and verification of the build configuration.
47-52: Structured Handling ofkotlinLanguageVersion
Mapping the property to aKotlinVersioninstance viaKotlinVersion::fromVersionensures proper version handling. Just ensure that the input string strictly follows the expected format.
53-58: Consistent Handling ofkotlinApiVersion
LikekotlinLanguageVersion, this configuration cleanly handles the API version. The logging statement helps verify that the correct version is applied.
59-65: Parsing Additional CLI Options
Splitting and trimming the CLI options string to produce a list makes the handling of additional compiler arguments robust. Defaulting to an empty list when absent further enhances stability.
67-71: Centralized Plugin Repository viakotlinDev()
Delegating the repository setup tokotlinDev()in thepluginManagementblock ensures that configuration remains consistent across the build. This promotes reusability and clarity in repository management.
73-90: Dynamic Version Catalog Configuration
Within thedependencyResolutionManagementblock, dynamically setting the Kotlin version (if provided) and conditionally overriding kotlinx dependency versions whenbuildSnapshotTrainis enabled creates a robust and flexible versioning mechanism. ThecheckNotNullcall ensures that required properties are present when needed.
92-125: Kotlin Compiler Options Configuration
The configuration applied in thegradle.afterProjectblock correctly sets the language and API versions, adjusts compiler arguments, and unconditionally disables-Werror. The detailed logging here is useful for validating the applied settings.
126-133: Conditional Diagnostics and Test Filtering
The decision to print the manifest for thektor-clientproject and to filter out specific tests whenbuildSnapshotTrainis enabled is a practical approach to managing build diagnostics and test stability in snapshot train builds.
135-145: Overriding Kotlinx Versions
TheoverrideKotlinxVersions()function provides a neat encapsulation for updating dependency versions (such asatomicfu,coroutines, andserialization) based on Gradle properties. Consider whether additional error handling is needed if a property is unexpectedly absent, although usingget()here enforces the expectation.
147-151: Custom Repository HandlerkotlinDev()
Encapsulating the repository addition logic within thekotlinDev()function promotes reuse and keeps the repository configuration DRY. This approach ensures that repository settings are applied consistently in both plugin and dependency resolution contexts.
153-176: Test Filtering for Stability
ThefilterTests()function methodically excludes tests known to be stressful or flaky, thereby increasing the reliability of test runs in environments where snapshot builds are performed. This selective exclusion is well considered.
178-188: Manifest Printing Utility
TheprintManifest()function is a useful addition for debugging the contents of the Kotlin compiler’s classpath. It accurately retrieves and prints the manifest file, though you might consider adding graceful error handling should the manifest file not be present.
190-193: Consistent Logging withlog()
The customlog()function ensures that build messages are prefixed uniformly with<KUP>, which enhances the traceability of logs. This abstraction simplifies logging across the build configuration.
osipxd
force-pushed
the
osipxd/kup-properties
branch
from
ab4f67e to
98669d2
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (4)
build-settings-logic/src/main/kotlin/ktorsettings.dependency-resolution-management.settings.gradle.kts (1)
48-52: Conditional Downgrade of Test Dependencies
ThedowngradeTestDependencies()function adjusts the version of the "logback" dependency when the test JDK version is below 11, which is key for maintaining compatibility with environments that have not upgraded past Java 8.
Consider adding an inline comment explaining the rationale for choosing version "1.3.14" to aid future maintainers.build-settings-logic/build.gradle.kts (1)
57-98: Task for Suppressing Gradle Plugin Version Warning
ThesuppressGradlePluginVersionWarningtask downloads and patches theEmbeddedKotlinPlugin.ktfile, converting warning logs to info logs and altering access modifiers. While this clever workaround effectively mutes the unwanted warning, please note that its reliance on specific text patterns might become fragile if the upstream file changes.Consider adding unit or integration tests to verify that the patch applies correctly in future Gradle releases.
build-settings-logic/src/main/kotlin/ktorsettings.kotlin-user-project.settings.gradle.kts (2)
126-133: Conditional Manifest Printing & Test Filtering
WhenbuildSnapshotTrainis true, the script conditionally handles specific projects (e.g., printing the manifest forktor-client) and applies test filtering. Note that using a directprintlnfor output differs from the logging strategy used elsewhere; ensure that this is an intentional decision for user-visible output.
178-188: Manifest Printing Routine
TheprintManifestfunction retrieves the manifest from files within thekotlinCompilerClasspathconfiguration. Consider adding error handling or a check (e.g., usingfirstOrNull()) to gracefully manage cases where no manifest file is found, thereby avoiding potential runtime exceptions.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (12)
build-logic/settings.gradle.kts(1 hunks)build-logic/src/main/kotlin/ktorbuild.kmp.gradle.kts(1 hunks)build-logic/src/main/kotlin/ktorbuild/KtorBuildExtension.kt(0 hunks)build-logic/src/main/kotlin/ktorbuild/internal/Train.kt(0 hunks)build-settings-logic/build.gradle.kts(2 hunks)build-settings-logic/settings.gradle.kts(1 hunks)build-settings-logic/src/main/kotlin/conventions-dependency-resolution-management.settings.gradle.kts(0 hunks)build-settings-logic/src/main/kotlin/ktorsettings.dependency-resolution-management.settings.gradle.kts(1 hunks)build-settings-logic/src/main/kotlin/ktorsettings.kotlin-user-project.settings.gradle.kts(1 hunks)build-settings-logic/src/main/kotlin/ktorsettings.settings.gradle.kts(1 hunks)ktor-test-server/settings.gradle.kts(1 hunks)settings.gradle.kts(1 hunks)
💤 Files with no reviewable changes (3)
- build-settings-logic/src/main/kotlin/conventions-dependency-resolution-management.settings.gradle.kts
- build-logic/src/main/kotlin/ktorbuild/internal/Train.kt
- build-logic/src/main/kotlin/ktorbuild/KtorBuildExtension.kt
🚧 Files skipped from review as they are similar to previous changes (3)
- build-settings-logic/settings.gradle.kts
- build-settings-logic/src/main/kotlin/ktorsettings.settings.gradle.kts
- build-logic/src/main/kotlin/ktorbuild.kmp.gradle.kts
🧰 Additional context used
🧠 Learnings (1)
build-settings-logic/src/main/kotlin/ktorsettings.kotlin-user-project.settings.gradle.kts (1)
Learnt from: osipxd
PR: ktorio/ktor#4745
File: build-settings-logic/src/main/kotlin/ktorbuild.develocity.settings.gradle.kts:37-40
Timestamp: 2025-04-03T10:10:40.262Z
Learning: In Kotlin, `String?.toBoolean()` is a null-safe extension function that returns false when called on a null value. The implementation delegates to Java's `Boolean.parseBoolean(this)`, making it safe to use after operations that might return null (like `.orNull`).
🔇 Additional comments (29)
ktor-test-server/settings.gradle.kts (2)
6-14: Repository Configuration Update
The updatedrepositoriesblock now explicitly includesgradlePluginPortal()and conditionally adds a Maven repository ("KotlinDev") if thekotlin_repo_urlproperty is provided. This centralizes repository management and ensures consistency with KUP standards.
19-19: Plugin Identifier Update
The plugin identifier has been updated toktorsettings, which aligns with the new organizational structure. Please verify that all build scripts referencing this identifier have been updated accordingly.build-logic/settings.gradle.kts (2)
6-14: Repository Configuration Update
The plugin management block now calls out to a dedicated repositories block that includesgradlePluginPortal()and conditionally sets up the Maven repository based onkotlin_repo_url. This mirrors the strategy used in other settings files and improves maintainability.
19-19: Plugin Identifier Update
Updating the plugin identifier toktorsettingsreinforces consistency across the project. Ensure that any downstream references are updated as well.settings.gradle.kts (2)
8-16: Repository Configuration Block
The repositories block underpluginManagementnow correctly includes bothgradlePluginPortal()and a conditional Maven repository ("KotlinDev") when thekotlin_repo_urlproperty is set. This provides a centralized approach to repository configuration following KUP guidelines.
22-24: Plugin Identifiers Update
The updated plugin identifiers (ktorsettings,ktorsettings.develocity, andktorsettings.configuration-cache) replace the older naming scheme. This refactoring supports a more organized dependency resolution strategy.build-settings-logic/src/main/kotlin/ktorsettings.dependency-resolution-management.settings.gradle.kts (3)
7-12: PluginManagement Repository Configuration
ThepluginManagementblock now leverages a call toconfigureRepositories()after includinggradlePluginPortal(). This provides a centralized mechanism to ensure that repository configurations are consistent across the build setup.
14-28: Dependency Resolution and Version Catalog Setup
ThedependencyResolutionManagementblock now reusesconfigureRepositories()for its repository setup and creates a version catalog ("libs") that conditionally loads dependency versions from the TOML file. The invocation ofdowngradeTestDependencies()ensures that test dependencies remain compatible with older JDK versions.
30-46: Centralized Repository Configuration Function
TheconfigureRepositories()function consolidates repository declarations by including Google’s repository, Maven Central, Maven Local, and an exclusively filtered Ktor EAP repository. This refactor enhances maintainability and consistency across different modules.build-settings-logic/build.gradle.kts (7)
5-5: Suppressing Unstable API Usage
The file-level suppression (@file:Suppress("UnstableApiUsage")) is acceptable here given the need to work with experimental Gradle APIs.
7-10: New Import Statements
The added import statements forDocsTypeand the Kotlin DSL support utilities are required for the new configurations and resolver attributes that follow.
15-19: Dependencies Update
Introducing the dependency onlibs.kotlin.gradlePluginensures that the project remains aligned with the Kotlin Gradle Plugin versioning. Just confirm thatexpectedKotlinDslPluginsVersionis set as intended elsewhere in your build configuration.
26-33: Workaround Documentation
The detailed comments in this workaround region clearly describe the issue with the unsupported Kotlin plugin version warning and the rationale behind patchingEmbeddedKotlinPlugin.kt. This documentation aids in maintaining the workaround in the future.
34-43: kotlinDslPluginSources Configuration
This configuration block is designed to download the original Gradle Kotlin DSL plugin source code. By setting appropriate dependency parameters (e.g., not consumable/resolvable), it isolates the configuration solely for source retrieval purposes.
45-55: kotlinDslPluginSourcesResolver Setup
The resolver configuration extends the sources setup and includes an attribute specifying that only source-type documentation is needed. This facilitates proper resolution of source files without unnecessary artifacts.
100-105: Updating Kotlin SourceSet
By adding thesuppressGradlePluginVersionWarningtask output as an additional source directory, the build ensures that the patched file is compiled alongside the rest of the DSL sources. Double-check that this inclusion does not inadvertently expose internal implementation details.build-settings-logic/src/main/kotlin/ktorsettings.kotlin-user-project.settings.gradle.kts (13)
34-38: Null-safety ofbuildSnapshotTrainInitialization
The lazy initialization uses Kotlin’s null‐safetoBoolean()extension, which correctly returnsfalsefor a null input. This approach avoids a potential NullPointerException and is in line with the expected behavior.
39-42:kotlinRepoUrlInitialization
The property is lazily retrieved and logs its value if present. This design is clear and useful for diagnostic purposes.
43-46:kotlinVersionProperty Setup
The retrieval and logging of the Kotlin version using a lazy delegate are implemented correctly.
47-52:kotlinLanguageVersionConversion and Logging
Mapping the Gradle property to aKotlinVersioninstance and logging its parsed value is handled cleanly.
53-58:kotlinApiVersionConfiguration
The property’s lazy initialization and logging mirror the approach used for the language version, ensuring consistency.
59-65:kotlinAdditionalCliOptionsSetup
Splitting the additional CLI options into a list—with a fallback to an empty list when the property is not set—ensures robust handling of optional configurations.
67-71: Plugin Management Configuration
Invoking thekotlinDev()function within the pluginManagement block adds the Kotlin Dev repository properly. Although the function is defined later, Gradle’s lazy evaluation makes this ordering acceptable.
73-90: Dependency Resolution and Version Catalog Setup
The configuration effectively sets up repository management and the version catalog. The conditional block that checks forbuildSnapshotTrainensures thatkotlinVersionis specified before overriding kotlinx versions. This implementation aligns well with the KUP requirements.
92-124: Kotlin Compiler Options Configuration
Within thegradle.afterProjectblock, the script first verifies the presence of the Kotlin extension before configuring compiler options. Setting of the language and API versions, disabling-Werror, and appending additional compiler arguments (including extra CLI options) is done in a clear and maintainable manner.
135-145: Override Kotlinx Versions Function
The helper function encapsulates the logic for overriding versions of libraries (atomicfu, coroutines, serialization) effectively. While the use ofget()assumes that these properties are present when needed, consider adding graceful error handling if any unexpected absence might occur.
147-151: KotlinDev Repository Handler Extension
The extension function onRepositoryHandlercorrectly adds the Maven repository whenkotlinRepoUrlis available, neatly abstracting repository configuration details.
153-176: Test Filtering Implementation
ThefilterTestsfunction methodically excludes known stress, flaky, and specific test patterns, which helps in maintaining stable build behavior. The use ofexcludeTestsMatchingis appropriate and clearly documented via in-line comments.
190-192: Logging Utility
Thelogfunction acts as a simple wrapper overlogger.info, prefixing messages with<KUP>for easier identification during log analysis. This abstraction enhances maintainability.
osipxd
force-pushed
the
osipxd/kup-properties
branch
from
98669d2 to
9335046
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (4)
build-settings-logic/src/main/kotlin/ktorsettings.kotlin-user-project.settings.gradle.kts (3)
59-66: Consider simplifying CLI‐options parsing
The split‑and‑filter pipeline works, but you might extract this logic into a helper function for readability or leveragesplitToSequenceto reduce intermediate collections.
108-124: Extract freeCompilerArgs into a constant
The list of compiler flags is domain‑specific and grows over time. Consider hoisting this into a top‑levelval defaultCompilerArgs = listOf(…)for maintainability and easier future adjustments.
155-177: Keep test‑filter patterns in sync
The hard‑coded list of excluded tests is fine, but as tests evolve, consider externalizing this list or grouping by annotation/tag to reduce maintenance overhead.build-settings-logic/build.gradle.kts (1)
67-96: Harden source‑patching logic
The string‑replace approach works but may break if upstream formatting changes. Consider switching to a Regex replacement or AST‑aware tool to locate and patch thewarn(...)call and class modifiers more robustly.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (12)
build-logic/settings.gradle.kts(1 hunks)build-logic/src/main/kotlin/ktorbuild.kmp.gradle.kts(1 hunks)build-logic/src/main/kotlin/ktorbuild/KtorBuildExtension.kt(0 hunks)build-logic/src/main/kotlin/ktorbuild/internal/Train.kt(0 hunks)build-settings-logic/build.gradle.kts(2 hunks)build-settings-logic/settings.gradle.kts(1 hunks)build-settings-logic/src/main/kotlin/conventions-dependency-resolution-management.settings.gradle.kts(0 hunks)build-settings-logic/src/main/kotlin/ktorsettings.dependency-resolution-management.settings.gradle.kts(1 hunks)build-settings-logic/src/main/kotlin/ktorsettings.kotlin-user-project.settings.gradle.kts(1 hunks)build-settings-logic/src/main/kotlin/ktorsettings.settings.gradle.kts(1 hunks)ktor-test-server/settings.gradle.kts(1 hunks)settings.gradle.kts(1 hunks)
💤 Files with no reviewable changes (3)
- build-settings-logic/src/main/kotlin/conventions-dependency-resolution-management.settings.gradle.kts
- build-logic/src/main/kotlin/ktorbuild/internal/Train.kt
- build-logic/src/main/kotlin/ktorbuild/KtorBuildExtension.kt
✅ Files skipped from review due to trivial changes (1)
- build-settings-logic/src/main/kotlin/ktorsettings.dependency-resolution-management.settings.gradle.kts
🚧 Files skipped from review as they are similar to previous changes (6)
- build-logic/settings.gradle.kts
- build-settings-logic/src/main/kotlin/ktorsettings.settings.gradle.kts
- build-settings-logic/settings.gradle.kts
- ktor-test-server/settings.gradle.kts
- build-logic/src/main/kotlin/ktorbuild.kmp.gradle.kts
- settings.gradle.kts
🧰 Additional context used
🧠 Learnings (1)
build-settings-logic/src/main/kotlin/ktorsettings.kotlin-user-project.settings.gradle.kts (1)
Learnt from: osipxd
PR: ktorio/ktor#4745
File: build-settings-logic/src/main/kotlin/ktorbuild.develocity.settings.gradle.kts:37-40
Timestamp: 2025-03-21T10:35:55.647Z
Learning: In Kotlin, `String?.toBoolean()` is a null-safe extension function that returns false when called on a null value. The implementation delegates to Java's `Boolean.parseBoolean(this)`, making it safe to use after operations that might return null (like `.orNull`).
🔇 Additional comments (16)
build-settings-logic/src/main/kotlin/ktorsettings.kotlin-user-project.settings.gradle.kts (12)
34-38: Correct handling ofbuildSnapshotTrainproperty
UsingString?.toBoolean()here is already null‑safe (it returnsfalseon null), so this concise pattern is appropriate and avoids unnecessary verbosity.
39-42: Approve lazy retrieval ofkotlinRepoUrl
The property is optional and only applied when present—this aligns with the requirement to conditionally add the Kotlin dev repository.
43-46: Approve lazy retrieval ofkotlinVersion
Deferring the read ofkotlin_versionuntil needed is good for startup performance and maintains consistency with other Gradle property reads.
47-52: ApprovekotlinLanguageVersionparsing
Mapping the string toKotlinVersionand logging it when set is correct. The use of.map(...).orNullensures safe handling if the property is absent.
53-58: ApprovekotlinApiVersionparsing
Same rationale as language version: safe mapping, good logging, and optional application.
68-72: ApprovepluginManagementrepository configuration
CentralizingkotlinDev()here ensures every included build can opt in to the snapshot repository whenkotlin_repo_urlis provided.
74-91: Approve version catalog management
Conditionally overriding the Kotlin version and kotlinx library versions underbuildSnapshotTrainis well‑structured. ThecheckNotNullguard forkotlinVersionprovides an early failure with a clear message.
93-107: Approve Kotlin compiler options configuration
Post‐evaluation hooking into projects with the Kotlin plugin and configuringlanguageVersion,apiVersion, and disabling-Werroris aligned with KUP requirements.
127-133: Approve test filtering underbuildSnapshotTrain
Conditionally suppressing flaky/stress tests and printing the compiler manifest forktor-clientmeets the Train requirements.
148-152: ApprovekotlinDev()repository handler
This clean extension method properly adds the Maven URL only whenkotlinRepoUrlis set.
179-189: Approve manifest printing logic
Extracting and printing theMANIFEST.MFfrom the embeddable compiler JAR is straightforward and informative for debugging snapshot builds.
191-193: Approve structured logging helper
A simplelogwrapper ensures consistent<KUP>‑prefixed messages.build-settings-logic/build.gradle.kts (4)
34-43: Approve configuration ofkotlinDslPluginSources
The custom configuration to fetch and isolate the Kotlin DSL plugin’s source code is well‑formed and correctly scoped.
45-55: Approve resolver configuration
Defining a separate, resolvable configuration with theSOURCESattribute is appropriate for extracting.ktfiles for patching.
57-66: ApprovesuppressGradlePluginVersionWarningtask inputs/outputs
UsingClasspathNormalizerandtemporaryDirfor outputs ensures task up‑to‑date checks function correctly.
100-103: Approve inclusion of patched source inmain
Registering the task’s output as an additional Kotlin source directory ensures the patched plugin is used at compile time without further manual wiring.
build-settings-logic/src/main/kotlin/ktorsettings.kotlin-user-project.settings.gradle.kts
Show resolved
Hide resolved
osipxd
force-pushed
the
osipxd/kup-properties
branch
from
9335046 to
287f1d3
Compare
osipxd
force-pushed
the
osipxd/kup-properties
branch
from
287f1d3 to
2f0e91c
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (4)
build-settings-logic/src/main/kotlin/ktorsettings.kotlin-user-project.settings.gradle.kts (3)
68-72: Consider adding standard plugin repositories
Right nowpluginManagementonly points tokotlinDev(). If other Gradle plugins are needed here, you may want to prependgradlePluginPortal()(and/ormavenCentral()) to avoid resolution failures.pluginManagement { repositories { - kotlinDev() + gradlePluginPortal() + kotlinDev() } }
93-134: Centralized compiler and test-filter logic is well organized
- Hooks into
afterProjectonly for projects with a Kotlin extension.- Applies language/API version overrides and compiler args.
- Filters flaky/stress tests and prints manifest for
ktor-client.
Consider using a plugin-applied check rather than hardcodingproject.name == "ktor-client"for manifest printing if this pattern grows.
179-189: Manifest printing could guard against missing entries
files.first()will throw if no matching manifest is found. Consider a safe lookup to avoid unexpected configuration-time exceptions:- .files.first() + .files.firstOrNull()?.also { manifest -> + manifest.useLines { println(it) } + } ?: logger.warn("<KUP> No MANIFEST.MF found in $file")build-settings-logic/settings.gradle.kts (1)
27-28: Conditional KotlinDev repository addition
Adding theKotlinDevMaven repository behind a property guard matches thektorsettingsapproach. Consider documentingkotlin_repo_urlin the root README for easier discoverability.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (12)
build-logic/settings.gradle.kts(1 hunks)build-logic/src/main/kotlin/ktorbuild.kmp.gradle.kts(1 hunks)build-logic/src/main/kotlin/ktorbuild/KtorBuildExtension.kt(0 hunks)build-logic/src/main/kotlin/ktorbuild/internal/Train.kt(0 hunks)build-settings-logic/build.gradle.kts(2 hunks)build-settings-logic/settings.gradle.kts(1 hunks)build-settings-logic/src/main/kotlin/conventions-dependency-resolution-management.settings.gradle.kts(0 hunks)build-settings-logic/src/main/kotlin/ktorsettings.dependency-resolution-management.settings.gradle.kts(1 hunks)build-settings-logic/src/main/kotlin/ktorsettings.kotlin-user-project.settings.gradle.kts(1 hunks)build-settings-logic/src/main/kotlin/ktorsettings.settings.gradle.kts(1 hunks)ktor-test-server/settings.gradle.kts(1 hunks)settings.gradle.kts(1 hunks)
💤 Files with no reviewable changes (3)
- build-logic/src/main/kotlin/ktorbuild/KtorBuildExtension.kt
- build-logic/src/main/kotlin/ktorbuild/internal/Train.kt
- build-settings-logic/src/main/kotlin/conventions-dependency-resolution-management.settings.gradle.kts
✅ Files skipped from review due to trivial changes (2)
- build-logic/src/main/kotlin/ktorbuild.kmp.gradle.kts
- build-settings-logic/src/main/kotlin/ktorsettings.dependency-resolution-management.settings.gradle.kts
🚧 Files skipped from review as they are similar to previous changes (5)
- ktor-test-server/settings.gradle.kts
- settings.gradle.kts
- build-settings-logic/src/main/kotlin/ktorsettings.settings.gradle.kts
- build-logic/settings.gradle.kts
- build-settings-logic/build.gradle.kts
🧰 Additional context used
🧠 Learnings (1)
build-settings-logic/src/main/kotlin/ktorsettings.kotlin-user-project.settings.gradle.kts (2)
Learnt from: osipxd
PR: ktorio/ktor#4745
File: build-settings-logic/src/main/kotlin/ktorbuild.develocity.settings.gradle.kts:37-40
Timestamp: 2025-03-21T10:35:55.647Z
Learning: In Kotlin, `String?.toBoolean()` is a null-safe extension function that returns false when called on a null value. The implementation delegates to Java's `Boolean.parseBoolean(this)`, making it safe to use after operations that might return null (like `.orNull`).
Learnt from: osipxd
PR: ktorio/ktor#4767
File: build-settings-logic/src/main/kotlin/ktorsettings.kotlin-user-project.settings.gradle.kts:136-146
Timestamp: 2025-04-23T11:48:32.634Z
Learning: For Kotlin Train builds in the Ktor project, the properties `atomicfu_version`, `coroutines_version`, and `serialization_version` are expected to be defined in the TeamCity CI environment, so using `.get()` without custom error handling is sufficient.
🔇 Additional comments (14)
build-settings-logic/src/main/kotlin/ktorsettings.kotlin-user-project.settings.gradle.kts (11)
34-38: buildSnapshotTrain initialization is safe and clear
The use ofString?.toBoolean()covers the null case (returnsfalse) and logs only when the property is explicitly set. No NPE risk here.
39-42: kotlinRepoUrl provider lookup is correct
.orNullcombined with.also { log(...) }cleanly handles presence/absence of thekotlin_repo_urlproperty.
43-46: kotlinVersion property retrieval looks good
Fetchingkotlin_versionviaproviders.gradlePropertyand logging its presence is straightforward and consistent.
47-52: Safe parsing of kotlin_language_version
Mapping the string toKotlinVersionvia.map(KotlinVersion::fromVersion)and unwrapping with.orNullis clear. Invalid version syntax will fail fast at configuration time.
53-58: Safe parsing of kotlin_api_version
Same pattern as language version; this ensures API version overrides are applied only when specified.
59-66: Robust splitting of additional CLI options
Trimming quotes/whitespace and splitting on whitespace yields a cleanList<String>, with.orEmpty()defaulting nicely.
74-91: Dependency resolution and version-catalog overrides are correct
- Sets the Kotlin version when provided.
- Enforces
kotlinVersionpresence underbuildSnapshotTrain.- Invokes
overrideKotlinxVersions()to overrideatomicfu,coroutines, andserializationversions as intended.
Everything aligns with the KUP requirements.
136-146: overrideKotlinxVersions fails fast as expected
Using.get()on each${name}_versiontriggers a clear failure when values are missing in CI, per the KUP conventions.
148-153: kotlinDev extension is concise and reusable
Encapsulating the conditionalmaven(...)call inkotlinDev()keeps the repository logic DRY.
155-177: Test-filtering patterns capture known flaky/stress cases
Hard-coded excludes align with the previousTrain.ktlogic. You might eventually externalize these patterns into a shared list if they change often.
191-193: Simple log wrapper for KUP messages
Thelog(...)helper cleanly namespaces log messages.build-settings-logic/settings.gradle.kts (3)
5-8: Verify develocity plugin version alignment
Ensurecom.gradle.develocityversion3.18.2remains in sync with other build-logic scripts (e.g.,ktorbuild.develocity.settings.gradle.kts).
10-10: Suppress UnstableApiUsage
The@Suppress("UnstableApiUsage")annotation is appropriate for using newer Gradle APIs in the settings script.
18-21: Conditional Kotlin version override
Overriding thelibscatalog’s Kotlin version whenkotlin_versionis set aligns with the KUP centralization goal.
|
❌ Junie can be launched only by a repository collaborator |
osipxd
force-pushed
the
osipxd/kup-properties
branch
from
2f0e91c to
9185fe2
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
build-settings-logic/src/main/kotlin/ktorsettings.kotlin-user-project.settings.gradle.kts (1)
59-66: TokenizingkotlinAdditionalCliOptions
Splitting on whitespace and trimming quotes produces a reliable list of compiler flags. Consider extending the trim logic to handle single quotes or escaped characters if needed in the future.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (12)
build-logic/settings.gradle.kts(1 hunks)build-logic/src/main/kotlin/ktorbuild.kmp.gradle.kts(1 hunks)build-logic/src/main/kotlin/ktorbuild/KtorBuildExtension.kt(0 hunks)build-logic/src/main/kotlin/ktorbuild/internal/Train.kt(0 hunks)build-settings-logic/build.gradle.kts(2 hunks)build-settings-logic/settings.gradle.kts(1 hunks)build-settings-logic/src/main/kotlin/conventions-dependency-resolution-management.settings.gradle.kts(0 hunks)build-settings-logic/src/main/kotlin/ktorsettings.dependency-resolution-management.settings.gradle.kts(1 hunks)build-settings-logic/src/main/kotlin/ktorsettings.kotlin-user-project.settings.gradle.kts(1 hunks)build-settings-logic/src/main/kotlin/ktorsettings.settings.gradle.kts(1 hunks)ktor-test-server/settings.gradle.kts(1 hunks)settings.gradle.kts(1 hunks)
💤 Files with no reviewable changes (3)
- build-logic/src/main/kotlin/ktorbuild/KtorBuildExtension.kt
- build-logic/src/main/kotlin/ktorbuild/internal/Train.kt
- build-settings-logic/src/main/kotlin/conventions-dependency-resolution-management.settings.gradle.kts
🚧 Files skipped from review as they are similar to previous changes (8)
- build-settings-logic/src/main/kotlin/ktorsettings.settings.gradle.kts
- build-logic/src/main/kotlin/ktorbuild.kmp.gradle.kts
- ktor-test-server/settings.gradle.kts
- build-settings-logic/settings.gradle.kts
- settings.gradle.kts
- build-logic/settings.gradle.kts
- build-settings-logic/src/main/kotlin/ktorsettings.dependency-resolution-management.settings.gradle.kts
- build-settings-logic/build.gradle.kts
🧰 Additional context used
🧠 Learnings (1)
build-settings-logic/src/main/kotlin/ktorsettings.kotlin-user-project.settings.gradle.kts (2)
Learnt from: osipxd
PR: ktorio/ktor#4745
File: build-settings-logic/src/main/kotlin/ktorbuild.develocity.settings.gradle.kts:37-40
Timestamp: 2025-03-21T10:35:55.647Z
Learning: In Kotlin, `String?.toBoolean()` is a null-safe extension function that returns false when called on a null value. The implementation delegates to Java's `Boolean.parseBoolean(this)`, making it safe to use after operations that might return null (like `.orNull`).
Learnt from: osipxd
PR: ktorio/ktor#4767
File: build-settings-logic/src/main/kotlin/ktorsettings.kotlin-user-project.settings.gradle.kts:136-146
Timestamp: 2025-04-23T11:48:32.634Z
Learning: For Kotlin Train builds in the Ktor project, the properties `atomicfu_version`, `coroutines_version`, and `serialization_version` are expected to be defined in the TeamCity CI environment, so using `.get()` without custom error handling is sufficient.
🔇 Additional comments (13)
build-settings-logic/src/main/kotlin/ktorsettings.kotlin-user-project.settings.gradle.kts (13)
34-38: Null-safe evaluation ofbuildSnapshotTrain
The use ofString?.toBoolean()is safe even when the Gradle property is unset, thanks to the Kotlin stdlib extension on nullableString. This will correctly default tofalsewithout risking an NPE.
39-42: Lazy retrieval ofkotlinRepoUrl
The property is lazily loaded and logged only when present, avoiding unnecessary repository configuration when unset.
43-46: Lazy retrieval ofkotlinVersion
Defining the Kotlin version viaproviders.gradlePropertyand logging it is clear and aligns with the KUP requirements.
47-52: ParsingkotlinLanguageVersiontoKotlinVersion
Mapping the property to aKotlinVersioninstance and logging itsversionfield ensures correct interpretation of language-level overrides.
53-58: ParsingkotlinApiVersiontoKotlinVersion
Same pattern as language version—clean and effective for API-level overrides.
68-72: Centralized plugin repository configuration
ThepluginManagementblock cleanly injects the Kotlin Dev repository for all plugin resolution.
74-91: Conditional version catalog adjustments
UsingversionCatalogsto override the Kotlin version and, underbuildSnapshotTrain, enforce presence ofkotlinVersionand calloverrideKotlinxVersions()centralizes dependency management effectively.
93-134: Dynamic Kotlin compiler configuration per project
TheafterProjecthook checks for the Kotlin plugin, applies language/API versions, disables-Werror, and augmentsfreeCompilerArgswith both static and user-specified options—this fulfills KUP requirements for consistent compiler behavior.
136-146: Use of.get()for required CI properties
Given thatatomicfu_version,coroutines_version, andserialization_versionare guaranteed in the CI environment, using.get()here is appropriate to fail fast on missing values.
148-152: Conditional addition of KotlinDev Maven repository
ThekotlinDev()extension neatly encapsulates repository logic, adding the dev repo only whenkotlinRepoUrlis defined.
154-177: Filtering flaky and stress tests for snapshot train builds
Disabling known unstable test groups, classes, and methods underbuildSnapshotTrainmaintains build stability for Ktor Train configurations.
179-189: Printingkotlin-compiler-embeddablemanifest
TheprintManifest()helper correctly unpacks and logs the JAR manifest for thektor-clientproject, mirroring the previous Train logic.
191-193: Custom logging helper with<KUP>prefix
Thelog()function provides consistent, identifiable log messages for all KUP-related configuration steps.
Subsystem
Infrastructure
Motivation
KTOR-8365 Update build scripts to conform with other Kotlin User Projects
Solution
Moved everything related to KUP and Train configurations to a build settings plugin, so now all these configurations are isolated in one file.
Now KUP settings should be also properly applied to included builds (
build-logicandktor-test-server).It is better to review commit-by-commit.