From 7746460f32ada4d1ff7916f8e05b8cf726f81f0f Mon Sep 17 00:00:00 2001 From: Alex Plate Date: Thu, 20 Jun 2024 14:21:58 +0300 Subject: [PATCH] [Workspace Model] [IJPL-867] Refactor deletion of iml files for removed imported modules `iml` files of imported modules are not automatically removed due to different circumstances. Because of this issue, the external build system did this manually for many years. Unfortunately, this also affects the serialization of files and caused IJPL-867. To fix the IJPL-867 issue, the following steps are taken: - Get rid of manual `iml` removal by the external build system (`AbstractModuleDataService`). - Update jps serializer to mark the imported iml for deletion. - The `.iml` should not be deleted by the component store, but it's not due to IJPL-926. Ideally, we fix IJPL-926, but for now do the workaround: remove the ` iml ` file on the level of JPS serialization. These steps fix the IJPL-867 issue, but a further fix for IJPL-926 is still needed. # Notes - Test `ExternalSystemProjectSaveTest` is created as a separate class that uses JUnit 4, however, this test can be placed in `ExternalSystemProjectTest`. The problem is that `ExternalSystemProjectTest` uses JUnit 3 and it freezez on `project.stateStore.save()` because of an unclear reason. - `JpsProjectSerializersImpl.saveEntities` has a complicated logic when the `iml` file should be removed. As there is no goal to refactor/refresh this logic, I tried to minimize the number of changes around this place. - `JpsProjectSerializersImpl.shouldDeleteImportedFile` has a corner case for broken configuration of the iml. This case is covered in `ExternalSystemStorageTest.multiple modules with the same name` test. - `ModuleImlFileEntitiesSerializer.manuallyRemoveImlFile` has a check for the correct casing. This test case is covered in `ExternalSystemStorageTest.multiple modules with the same name but different case`. - After the `iml` deletion, the VFS should technically be refreshed, but we don't do it because the module with JPS serialization does not have a dependency to the module with VFS. Because of that, the `iml` does not disappear immediately from the project tree if it's there. I consider that this problem is minor. GitOrigin-RevId: e6228e06017f6aef26fb78321a27a3ad05e1c22c --- .../manage/AbstractModuleDataService.java | 2 - .../project/ExternalSystemProjectSaveTest.kt | 87 +++++++++++++++++++ .../impl/JpsProjectSerializersImpl.kt | 33 +++++-- .../impl/ModuleImlFileEntitiesSerializer.kt | 16 ++++ 4 files changed, 127 insertions(+), 11 deletions(-) create mode 100644 platform/external-system-impl/testSrc/com/intellij/openapi/externalSystem/service/project/ExternalSystemProjectSaveTest.kt diff --git a/platform/external-system-impl/src/com/intellij/openapi/externalSystem/service/project/manage/AbstractModuleDataService.java b/platform/external-system-impl/src/com/intellij/openapi/externalSystem/service/project/manage/AbstractModuleDataService.java index 988f163cff95..503c8ff7ea4c 100644 --- a/platform/external-system-impl/src/com/intellij/openapi/externalSystem/service/project/manage/AbstractModuleDataService.java +++ b/platform/external-system-impl/src/com/intellij/openapi/externalSystem/service/project/manage/AbstractModuleDataService.java @@ -4,7 +4,6 @@ package com.intellij.openapi.externalSystem.service.project.manage; import com.intellij.configurationStore.StoreUtil; import com.intellij.history.LocalHistory; import com.intellij.history.LocalHistoryAction; -import com.intellij.ide.util.projectWizard.ModuleBuilder; import com.intellij.notification.Notification; import com.intellij.notification.NotificationGroupManager; import com.intellij.notification.NotificationType; @@ -287,7 +286,6 @@ public abstract class AbstractModuleDataService extends Ab } } modelsProvider.getModifiableModuleModel().disposeModule(module); - ModuleBuilder.deleteModuleFile(path); } } finally { diff --git a/platform/external-system-impl/testSrc/com/intellij/openapi/externalSystem/service/project/ExternalSystemProjectSaveTest.kt b/platform/external-system-impl/testSrc/com/intellij/openapi/externalSystem/service/project/ExternalSystemProjectSaveTest.kt new file mode 100644 index 000000000000..8b687c5aeeb3 --- /dev/null +++ b/platform/external-system-impl/testSrc/com/intellij/openapi/externalSystem/service/project/ExternalSystemProjectSaveTest.kt @@ -0,0 +1,87 @@ +// Copyright 2000-2024 JetBrains s.r.o. and contributors. Use of this source code is governed by the Apache 2.0 license. +package com.intellij.openapi.externalSystem.service.project + +import com.intellij.openapi.externalSystem.ExternalSystemManager +import com.intellij.openapi.externalSystem.model.DataNode +import com.intellij.openapi.externalSystem.model.ProjectKeys +import com.intellij.openapi.externalSystem.model.internal.InternalExternalProjectInfo +import com.intellij.openapi.externalSystem.model.project.ProjectData +import com.intellij.openapi.externalSystem.service.project.manage.ExternalProjectsManagerImpl +import com.intellij.openapi.externalSystem.util.ExternalSystemApiUtil +import com.intellij.openapi.module.ModuleManager +import com.intellij.platform.externalSystem.testFramework.TestExternalProjectSettings +import com.intellij.platform.externalSystem.testFramework.TestExternalSystemManager +import com.intellij.platform.externalSystem.testFramework.toDataNode +import com.intellij.project.stateStore +import com.intellij.testFramework.ApplicationRule +import com.intellij.testFramework.DisposableRule +import com.intellij.testFramework.ExtensionTestUtil +import com.intellij.testFramework.ProjectRule +import junit.framework.TestCase.assertFalse +import junit.framework.TestCase.assertTrue +import kotlinx.coroutines.runBlocking +import org.junit.ClassRule +import org.junit.Rule +import org.junit.Test +import kotlin.io.path.Path +import kotlin.io.path.exists + +class ExternalSystemProjectSaveTest { + companion object { + @JvmField + @ClassRule + val appRule = ApplicationRule() + + } + + @JvmField + @Rule + val disposableRule = DisposableRule() + + @JvmField + @Rule + val projectRule = ProjectRule() + + @Test + fun `iml file of removed module is also removed`() { + ExtensionTestUtil.addExtensions(ExternalSystemManager.EP_NAME, listOf(TestExternalSystemManager(projectRule.project)), disposableRule.disposable) + + val model = com.intellij.platform.externalSystem.testFramework.project("MyProject", projectRule.project.basePath!!) { + module("Module1") + module("Module2") + } + val nodes = model.toDataNode() + applyProjectModel(listOf(nodes)) + runBlocking { projectRule.project.stateStore.save() } + + val filePath = ModuleManager.getInstance(projectRule.project).findModuleByName("Module1")!!.moduleFilePath + assertTrue(Path(filePath).exists()) + + val model2 = com.intellij.platform.externalSystem.testFramework.project("MyProject", projectRule.project.basePath!!) { + module("Module2") + } + applyProjectModel(listOf(model2.toDataNode())) + runBlocking { projectRule.project.stateStore.save() } + + assertFalse(Path(filePath).exists()) + } + + private fun applyProjectModel(nodes: List>) { + val projectManager = ExternalProjectsManagerImpl.getInstance(projectRule.project) + for (node in nodes) { + val projectSystemId = node.data.owner + val settings = ExternalSystemApiUtil.getSettings(projectRule.project, projectSystemId) + val projectPath = node.data.linkedExternalProjectPath + val isLinked = settings.linkedProjectsSettings.map { it.externalProjectPath }.contains(projectPath) + if (!isLinked) { + settings.linkProject(TestExternalProjectSettings().also { it.externalProjectPath = projectPath }) + } + val externalModulePaths = ExternalSystemApiUtil.findAll(node, ProjectKeys.MODULE).map { it.data.linkedExternalProjectPath }.toSet() + settings.getLinkedProjectSettings(projectPath)!!.setModules(externalModulePaths) + + ProjectDataManager.getInstance().importData(node, projectRule.project) + val projectInfo = InternalExternalProjectInfo(projectSystemId, projectPath, node) + projectManager.updateExternalProjectData(projectInfo) + } + } +} \ No newline at end of file diff --git a/platform/workspace/jps/src/com/intellij/platform/workspace/jps/serialization/impl/JpsProjectSerializersImpl.kt b/platform/workspace/jps/src/com/intellij/platform/workspace/jps/serialization/impl/JpsProjectSerializersImpl.kt index 0ae76a0e6212..f90404a28472 100644 --- a/platform/workspace/jps/src/com/intellij/platform/workspace/jps/serialization/impl/JpsProjectSerializersImpl.kt +++ b/platform/workspace/jps/src/com/intellij/platform/workspace/jps/serialization/impl/JpsProjectSerializersImpl.kt @@ -671,18 +671,22 @@ class JpsProjectSerializersImpl(directorySerializersFactories: List sourcesStoredInternally[source.internalFile] - source is JpsImportedEntitySource && !source.storedExternally -> sourcesStoredExternally[source.internalFile] source is JpsFileEntitySource -> sourcesStoredExternally[source] else -> null } - // When user removes module from project we don't delete corresponding *.iml file located under project directory by default - // (because it may be included in other projects). However we do remove the module file if module actually wasn't removed, just - // its storage has been changed, e.g. if module was marked as imported from external system, or the place where module imported - // from external system was changed, or part of a module configuration is imported from external system and data stored in *.iml - // file was removed. - val deleteObsoleteFile = source in internalSourceConvertedToImported || (affectedImportedSourceStoredExternally != null && - affectedImportedSourceStoredExternally !in obsoleteSources) + // When user removes module from the project, we don't delete corresponding *.iml file located under project directory by default + // because it may be included in other projects. + // + // However, we do remove the module file if: + // - Module is imported from the external build system (like maven). + // - Module actually wasn't removed, just its storage has been changed, e.g: if module was marked as imported from the external system + // (aka mavenize module). + // - Imported module had user-configured information (like custom content roots). This additional information is stored in the local + // `.iml` file, and the `.iml` should be removed in case all custom elements are removed. + // - TO DO: Fill new cases if found! + val deleteObsoleteFile = shouldDeleteImportedFile(source, fileUrl) || + source in internalSourceConvertedToImported || + (affectedImportedSourceStoredExternally != null && affectedImportedSourceStoredExternally !in obsoleteSources) processObsoleteSource(fileUrl, deleteObsoleteFile, writer, affectedEntityTypeSerializers, affectedModuleListSerializers, storage) val actualSource = if (source is JpsImportedEntitySource && !source.storedExternally) source.internalFile else source if (actualSource is JpsProjectFileEntitySource.FileInDirectory) { @@ -804,6 +808,17 @@ class JpsProjectSerializersImpl(directorySerializersFactories: List