Improved build performance by pre-creating Ivy XML files in the extracted IDE location. #1785

AlexanderBartash · 2024-10-04T22:29:39Z

Pull Request Details

Combines #1782 #1783 and adds another performance improvement.
Recommended to review by commits.

Description

ExtractorTransformer now pre-creates Ivy XML files in the extracted IDE location, which makes it unnecessary to load all IDE plugins, resolve their location, create Ivy XML files on every build thereafter.

Potentially this is a breaking change because bundledPlugin & bundledModule dependencies now live in a separate repository created by IntelliJPlatformDependenciesHelper#registerIntellijPlatformIvyRepo That repository is not customizable by the user.

Related Issue

No issue.

Motivation and Context

Life is short.

How Has This Been Tested

Tests, manually. On my project also on https://github.com/InSyncWithFoo/ryecharm

Types of changes

Docs change / refactoring / dependency upgrade
Bug fix (non-breaking change which fixes an issue)
New feature (non-breaking change which adds functionality)
[x ] Breaking change (fix or feature that would cause existing functionality to change)

Checklist

[x ] I have read the CONTRIBUTING document.
[ x] My code follows the code style of this project.
My change requires a change to the documentation.
I have updated the documentation accordingly.
[x ] I have included my change in the CHANGELOG.
I have added tests to cover my changes.
[ x] All new and existing tests passed.

...in/kotlin/org/jetbrains/intellij/platform/gradle/artifacts/transform/ExtractorTransformer.kt

AlexanderBartash · 2024-10-04T22:38:59Z

...tlin/org/jetbrains/intellij/platform/gradle/extensions/IntelliJPlatformDependenciesHelper.kt

+            includeGroup(Dependencies.BUNDLED_PLUGIN_GROUP)
+        }
+    }
+


I could create it in createDefaultRepositores() as well, so that users have a dedicated method for this, but I thought it would be bad, because then, there we would be updating ivy & artifact path of that repo on the fly. Which is error prone, if this is exposed to customizations.

I actually think now, that this approach is better than what was before, because previously (I have not actually tried it) if the developer did not call createDefaultRepositores() these dependencies won't work, Which is counter intuitive, why do we allow defining dependencies which will not work. With this approach if a dependency on a bundled plugin or module is added it will work.

AlexanderBartash · 2024-10-04T22:39:57Z

...tlin/org/jetbrains/intellij/platform/gradle/extensions/IntelliJPlatformDependenciesHelper.kt

+                .listDirectoryEntries()
+                .filter { it.isDirectory() }
+                .mapNotNull { path ->
+                    // TODO: try not to parse all plugins at once


Now it parses them only if local("/opt/ideaIU") is used. Otherwise it is done in the transformer only once.

AlexanderBartash · 2024-10-04T22:40:56Z

...in/kotlin/org/jetbrains/intellij/platform/gradle/artifacts/transform/ExtractorTransformer.kt

+        val pluginManager = IdePluginManager.createManager(createTempDirectory())
+        val plugins = platformPath.getBundledPlugins(pluginManager)
+
+        for (plugin in plugins.values) {


I think some optimizations are possible here for caching & skipping what was already discovered, but I do not want to over complicate it. Also since it is done in the transformer now, it does not matter that much.

AlexanderBartash · 2024-10-04T22:41:59Z

...tlin/org/jetbrains/intellij/platform/gradle/extensions/IntelliJPlatformDependenciesHelper.kt

+            pathToWriteInto: Path,
+
+            isCreateModules: Boolean
+        ): List<IvyModule.Dependency> {


So the above list of args is not something I am proud of. It is like that because I've kept things as they were as much as possible to avoid making this PR too big. It can be refactored later if needed.

...tlin/org/jetbrains/intellij/platform/gradle/extensions/IntelliJPlatformDependenciesHelper.kt

AlexanderBartash · 2024-10-04T22:44:07Z

...tlin/org/jetbrains/intellij/platform/gradle/extensions/IntelliJPlatformRepositoriesHelper.kt

+
+            // For performance reasons make it exclusive.
+            // https://docs.gradle.org/current/userguide/declaring_repositories_adv.html#declaring_content_exclusively_found_in_one_repository
+            repositories.exclusiveContent {


So this is also an improvement over #1783 I noticed that there is a dedicated API for telling Gradle "look for this group only here" which is probably the safest option possible, especially if combined with prefixed groups with "com.jetbrains".

Btw, if we pre-create that new dynamic Ivy repository here as well, we can also make it exclusive, it may save a couple of seconds on the build. But again, I am not sure that creating a repo with broken paths is something we should be doing.

...in/kotlin/org/jetbrains/intellij/platform/gradle/artifacts/transform/ExtractorTransformer.kt

AlexanderBartash · 2024-10-06T03:57:49Z

...tlin/org/jetbrains/intellij/platform/gradle/extensions/IntelliJPlatformRepositoriesHelper.kt

+            val absNormArtifactLocationPath = artifactLocationPath?.absolute()?.normalize()
+
+            // Location of Ivy files generated for the current project.
+            val ivyPath = providers.localPlatformArtifactsPath(absNormIvyLocationPath)


localPlatformArtifactsPath does not make much sense with the dynamic repository. Should I apply it only for repositories which are static?

I did that. Now the dynamic repo stores Ivy.xml files into "localPlatformArtifacts" inside platformPath (not customisable). If a local("/path/to/ide") is used, the customizable location is used from providers.localPlatformArtifactsPath(absNormIvyLocationPath)

AlexanderBartash · 2024-10-06T03:58:53Z

src/main/kotlin/org/jetbrains/intellij/platform/gradle/models/IvyModule.kt

+private fun Path.containingDirPathStringRelativeTo(basePath: Path? = null): String {
+    // The contract is that we are working with absolute normalized paths here.
+    val absNormalizedPath = this.absolute().normalize()
+    val absNormalizedBasePath = basePath?.absolute()?.normalize()


So, I do this very often. The reason is that I try to design functions here as independent black boxes, which should just work by themselves. It should be more fool-proof this way.

AlexanderBartash · 2024-10-06T06:48:47Z

I think I have finally made it all work, but it seems that tests on windows fail randomly. I've seen this error first on Gradle 8.2 Win, then on Gradle 8.10.2 Win, now it is back on 8.2 Win.

I do no see how my changes could have caused this.

There seems to be an open bug https://youtrack.jetbrains.com/issue/KTIJ-893

org.gradle.testkit.runner.UnexpectedBuildFailure: Unexpected build execution failure in C:\Users\RUNNER~1\AppData\Local\Temp\tmp12965048409070354875 with arguments [-Dorg.gradle.kotlin.dsl.scriptCompilationAvoidance=false, verifyPluginProjectConfiguration, --stacktrace, --configuration-cache]

Output:
Calculating task graph as no configuration cache is available for tasks: verifyPluginProjectConfiguration

> Configure project :
Kotlin DSL property assignment is an incubating feature.
e: file:///C:/Users/runneradmin/AppData/Local/Temp/tmp12965048409070354875/build.gradle.kts:1:1: Cannot access implicit script receiver class 'org.gradle.api.Project'. Check your module classpath for missing or conflicting dependencies
e: file:///C:/Users/runneradmin/AppData/Local/Temp/tmp12965048409070354875/build.gradle.kts:8:1: Function invocation 'version(...)' expected
e: file:///C:/Users/runneradmin/AppData/Local/Temp/tmp12965048409070354875/build.gradle.kts:8:1: Unresolved reference. None of the following candidates is applicable because of receiver type mismatch: 
public infix fun PluginDependencySpec.version(version: String?): PluginDependencySpec defined in org.gradle.kotlin.dsl
public infix fun PluginDependencySpec.version(version: Provider<String>): PluginDependencySpec defined in org.gradle.kotlin.dsl
e: file:///C:/Users/runneradmin/AppData/Local/Temp/tmp12965048409070354875/build.gradle.kts:16:1: Unresolved reference. None of the following candidates is applicable because of receiver type mismatch: 
public fun Project.kotlin(configure: Action<KotlinJvmProjectExtension>): Unit defined in org.gradle.kotlin.dsl
public fun DependencyHandler.kotlin(module: String, version: String? = ...): Any defined in org.gradle.kotlin.dsl
public fun SourceSet.kotlin(configure: Action<SourceDirectorySet>): Unit defined in org.gradle.kotlin.dsl
public fun PluginDependenciesSpec.kotlin(module: String): PluginDependencySpec defined in org.gradle.kotlin.dsl

AlexanderBartash · 2024-10-06T16:02:55Z

I am done with this PR, I am not planning to change anything, unless there is a feedback. I tested this very extensively on my project on both linux and windows. Also I used this to build https://github.com/InSyncWithFoo/ryecharm on linux, it works.

AlexanderBartash · 2024-10-06T16:41:58Z

src/main/kotlin/org/jetbrains/intellij/platform/gradle/models/IvyModule.kt

@@ -71,7 +75,16 @@ data class IvyModule(
 * <ivy-module version="2.0">
 *     ...
 *     <publications>
- *         <artifact conf="default" ext="jar" name="/path/to/artifact.jar" type="jar" />


Previously in all paths we had the leading slash, but the artifact pattern was already /[artifact] which would lead to double slash eventually. It seems to work like that, but I fixed this anyway.

AlexanderBartash · 2024-10-08T17:03:11Z

Rebased on HEAD. Also reverted one change:

Revert prefixing local groups with "com.jetbrains.localhost-only" because group names are hardcoded in the IDE https://github.com/JetBrains/intellij-community/blob/c8ba8401f73488f966b5ed5b5c5afaa38a9bcbdf/plugins/devkit/intellij.devkit.gradle/src/IntelliJPlatformAttachSourcesProvider.kt#L64-L67 AlexanderBartash 5 minutes ago f0bb8cc

…n. Previously it was failing with "Failed to create MD5 hash for file".

…n the list and prefixing all "fake" artifact groups with "com.jetbrains.localhost-only" and excluding that group from already declared remote repositories.

…cted IDE location.

…ause group names are hardcoded in the IDE https://github.com/JetBrains/intellij-community/blob/c8ba8401f73488f966b5ed5b5c5afaa38a9bcbdf/plugins/devkit/intellij.devkit.gradle/src/IntelliJPlatformAttachSourcesProvider.kt#L64-L67

AlexanderBartash · 2024-10-09T17:24:11Z

Rebased.

…ies because since we are creating the bundled artifacts repository lazily it is last in the list and it tried to resolve artifacts in other repos, which might lead to accidental download of a bundled plugin from the marketplace instead of taking it from the local file system.

AlexanderBartash force-pushed the issue-1779+performance-improvements branch from fa28d35 to 55c3d65 Compare October 4, 2024 22:32

AlexanderBartash commented Oct 4, 2024

View reviewed changes

...in/kotlin/org/jetbrains/intellij/platform/gradle/artifacts/transform/ExtractorTransformer.kt Outdated Show resolved Hide resolved

AlexanderBartash commented Oct 4, 2024

View reviewed changes

...tlin/org/jetbrains/intellij/platform/gradle/extensions/IntelliJPlatformDependenciesHelper.kt Outdated Show resolved Hide resolved

AlexanderBartash commented Oct 4, 2024

View reviewed changes

AlexanderBartash force-pushed the issue-1779+performance-improvements branch 4 times, most recently from c8d6d46 to 01a2617 Compare October 5, 2024 00:32

AlexanderBartash commented Oct 5, 2024

View reviewed changes

...in/kotlin/org/jetbrains/intellij/platform/gradle/artifacts/transform/ExtractorTransformer.kt Show resolved Hide resolved

AlexanderBartash closed this Oct 5, 2024

AlexanderBartash reopened this Oct 5, 2024

AlexanderBartash force-pushed the issue-1779+performance-improvements branch 6 times, most recently from 5117a87 to 659b1aa Compare October 5, 2024 16:19

AlexanderBartash mentioned this pull request Oct 5, 2024

Support dependency verification #1779

Closed

AlexanderBartash force-pushed the issue-1779+performance-improvements branch 2 times, most recently from cd0934e to a1c661f Compare October 6, 2024 03:52

AlexanderBartash commented Oct 6, 2024

View reviewed changes

AlexanderBartash force-pushed the issue-1779+performance-improvements branch 3 times, most recently from 65773d4 to 5ecf8db Compare October 6, 2024 06:09

AlexanderBartash force-pushed the issue-1779+performance-improvements branch from 5ecf8db to f65a70d Compare October 6, 2024 15:28

AlexanderBartash commented Oct 6, 2024

View reviewed changes

AlexanderBartash force-pushed the issue-1779+performance-improvements branch 2 times, most recently from f0bb8cc to cfd2cdb Compare October 8, 2024 17:02

AlexanderBartash added 4 commits October 9, 2024 20:22

JetBrains#1779 Fixes compatibility with Gradle dependency verificatio…

2c44200

…n. Previously it was failing with "Failed to create MD5 hash for file".

Improved build performance by making the local Ivy repository first i…

e422035

…n the list and prefixing all "fake" artifact groups with "com.jetbrains.localhost-only" and excluding that group from already declared remote repositories.

Improved build performance by pre-creating Ivy XML files in the extra…

2dc9a6a

…cted IDE location.

AlexanderBartash force-pushed the issue-1779+performance-improvements branch from cfd2cdb to f46afa5 Compare October 9, 2024 17:23

hsz self-requested a review October 9, 2024 20:10

hsz and others added 2 commits October 10, 2024 18:51

code review

dc1e034

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Improved build performance by pre-creating Ivy XML files in the extracted IDE location. #1785

Improved build performance by pre-creating Ivy XML files in the extracted IDE location. #1785

AlexanderBartash commented Oct 4, 2024 •

edited

Loading

AlexanderBartash Oct 4, 2024

AlexanderBartash Oct 8, 2024 •

edited

Loading

AlexanderBartash Oct 4, 2024

AlexanderBartash Oct 4, 2024

AlexanderBartash Oct 4, 2024

AlexanderBartash Oct 4, 2024

AlexanderBartash Oct 4, 2024

AlexanderBartash Oct 6, 2024

AlexanderBartash Oct 6, 2024

AlexanderBartash Oct 6, 2024

AlexanderBartash commented Oct 6, 2024 •

edited

Loading

AlexanderBartash commented Oct 6, 2024 •

edited

Loading

AlexanderBartash Oct 6, 2024

AlexanderBartash commented Oct 8, 2024

AlexanderBartash commented Oct 9, 2024

Improved build performance by pre-creating Ivy XML files in the extracted IDE location. #1785

Are you sure you want to change the base?

Improved build performance by pre-creating Ivy XML files in the extracted IDE location. #1785

Conversation

AlexanderBartash commented Oct 4, 2024 • edited Loading

Pull Request Details

Description

Related Issue

Motivation and Context

How Has This Been Tested

Types of changes

Checklist

Choose a reason for hiding this comment

AlexanderBartash Oct 8, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

AlexanderBartash commented Oct 6, 2024 • edited Loading

AlexanderBartash commented Oct 6, 2024 • edited Loading

Choose a reason for hiding this comment

AlexanderBartash commented Oct 8, 2024

AlexanderBartash commented Oct 9, 2024

AlexanderBartash commented Oct 4, 2024 •

edited

Loading

AlexanderBartash Oct 8, 2024 •

edited

Loading

AlexanderBartash commented Oct 6, 2024 •

edited

Loading

AlexanderBartash commented Oct 6, 2024 •

edited

Loading