#1676 import extra sdks automatically into ide #2073
Conversation
- Load extra SDKs from ide-extra-tools.json and register supported ones in IntelliJ. - The IntelliJ merge template is expected from the settings repository.
…utomatically-into-IDE-' into feature/1676-Import-extra-SDKs-automatically-into-IDE-
…atically-into-IDE-
vivu001
added
enhancement
Coverage Report for CI Build 29021898039Coverage increased (+0.09%) to 72.311%Details
Uncovered ChangesNo uncovered changes found. Coverage Regressions14 previously-covered lines in 3 files lost coverage.
Coverage Stats
💛 - Coveralls |
…med attribute placeholders in jdk-extra-java.xml
hohwille
left a comment
hohwille
left a comment
There was a problem hiding this comment.
@vivu001 thanks for your PR and sorry to interrupt before team review.
I had a look into your diff and found some misunderstandings and before moving any further, I wanted to guide the way to the proper solution.
You did a lot perfectly correct, but there are some misunderstandings between #1166 and #1676 that seemed not clear.
Also preparing this implementation for VSCode, Eclipse and other potential IDEs or even future SDKs I suggest some more generic approach and according refactoring.
Please have a look and see if you can follow my idea and suggestions.
If there are any questions do not hesitate to reach out.
| @@ -373,6 +376,28 @@ private void installExtraToolInstallations(String tool, List<ExtraToolInstallati | |||
| } | |||
| } | |||
|
|
|||
| private void synchronizeIdeExtraToolInstallations(ExtraTools extraTools) { | |||
|
|
|||
| if (extraTools == null) { | |||
| return; | |||
| } | |||
|
|
|||
| Path workspacePath = this.context.getWorkspacePath(); | |||
| if (!Files.isDirectory(workspacePath)) { | |||
| LOG.debug("Skipping IDE extra SDK synchronization because workspace path does not exist: {}", workspacePath); | |||
| return; | |||
| } | |||
|
|
|||
| Intellij intellij = this.context.getCommandletManager().getCommandlet(Intellij.class); | |||
| if (intellij != null) { | |||
| this.context.newStep("Synchronize IntelliJ extra SDKs").run(() -> intellij.synchronizeExtraToolInstallations(workspacePath)); | |||
| } | |||
|
|
|||
| // later: | |||
| // Eclipse eclipse = ... | |||
| // VsCode vscode = ... | |||
| } | |||
|
|
|||
There was a problem hiding this comment.
IMHO this is the wrong place to trigger this:
The workspace will just be the current workspace what is typically main for ide update and will always be for ide create.
However, when I start ide intellij the workspace configuration is updated and IntelliJ is started in the currnet workspace that may differ from the one used by ide update.
Therefore you need to apply the changes when running the IDE.
This also gives us better SoC (separation of concerns):
With the current approach, the AbstractUpdateCommandlet needs to know about all IDE commandlets that can handle extra tools. Instead we should only trigger this inside the according IDE commandlet like Intellij, when the workspace is synchronized. So simply extend this method:
I will add the idea as extra review comment to IdeToolCommandlet.
There was a problem hiding this comment.
I've fixed it in the latest commits.
| if ((config.reservedName() != null) && config.reservedName().equalsIgnoreCase(name)) { | ||
| LOG.warn("Skipping IntelliJ import for extra {} installation '{}': name conflicts with main {} installation.", tool, name, tool); | ||
| return; | ||
| } |
There was a problem hiding this comment.
Valid point: You want to prevent that an extra installation of <tool> (e.g. java) also has the name <tool>:
{
"java": {
"java": { "version":"11.0.27_6", "edition":"azul" }
}
}
I fully agree but IMHO we should add this as a validation to ExtraTools (or ExtraToolsMapper) so it already fails fast.
| if (!Files.exists(templateFile)) { | ||
| throw new CliException( | ||
| "Cannot import extra " + tool + " installation into IntelliJ: template file not found at " + templateFile + "\n" | ||
| + "Please do an upstream merge of your settings git repository."); | ||
| } |
There was a problem hiding this comment.
Dont do this:
This is likely to happen for existing projects that have already forked their own ide-settings.
Since you do not run this code inside an extra step, any exception, will terminate the entire process.
That means users will be blocked and cannot start IntelliJ any more with ide intellij.
There was a problem hiding this comment.
This has been removed in the latest commits.
| errors = mergeWorkspace(this.context.getUserHomeIde(), workspaceFolder, errors); | ||
| errors = mergeWorkspace(this.context.getSettingsPath(), workspaceFolder, errors); | ||
| errors = mergeWorkspace(this.context.getConfPath(), workspaceFolder, errors); | ||
| if (errors == 0) { |
There was a problem hiding this comment.
here add the following call above:
| synchronizeExtraToolInstallations(); | |
| if (errors == 0) { |
There was a problem hiding this comment.
I also thought this method is called at the correct place. But actually, IntelliJ’s workspace sync runs before the software/extra/java/... folders exist, so it cannot import the new sdks.
"java": {
"client": {
"version": "11.0.31_11",
"edition": "azul"
},
"process-engine": {
"version": "21.*"
}
}
...
The warning is therefore shown some thing like that:
Skipping extra tool installation import to intellij because it is missing at [$IDE_HOME]\software\extra\java\client.
Please run the following command to fix:
ide update
Skipping extra tool installation import to intellij because it is missing at [$IDE_HOME]\software\extra\java\process-engine.
Please run the following command to fix:
ide update
But I’m struggling to find out how to fix it. 😃
| public abstract class IdeToolCommandlet extends PluginBasedCommandlet { | ||
|
|
||
| private static final Logger LOG = LoggerFactory.getLogger(IdeToolCommandlet.class); | ||
|
|
There was a problem hiding this comment.
| private final Map<String, Set<Path>> extraSdkMap; |
| /** | ||
| * Synchronizes extra IDEasy tool installations into the IDE workspace configuration if supported. | ||
| * | ||
| * <p> | ||
| * The default implementation does nothing. IDEs such as IntelliJ may override this hook to import extra SDK/tool installations configured via | ||
| * {@code settings/ide-extra-tools.json}. | ||
| * </p> | ||
| * | ||
| * @param workspacePath the workspace root whose IDE configuration should be synchronized. | ||
| */ | ||
| public void synchronizeExtraToolInstallations(Path workspacePath) { | ||
| // default no-op | ||
| } |
There was a problem hiding this comment.
| /** | |
| * Synchronizes extra IDEasy tool installations into the IDE workspace configuration if supported. | |
| * | |
| * <p> | |
| * The default implementation does nothing. IDEs such as IntelliJ may override this hook to import extra SDK/tool installations configured via | |
| * {@code settings/ide-extra-tools.json}. | |
| * </p> | |
| * | |
| * @param workspacePath the workspace root whose IDE configuration should be synchronized. | |
| */ | |
| public void synchronizeExtraToolInstallations(Path workspacePath) { | |
| // default no-op | |
| } | |
| /** | |
| * TODO: add JavaDoc please | |
| */ | |
| protected void registerExtraSdkTemplate(String sdk, Path relativeTemplatePath) { | |
| Set<Path> templatePaths = this.extraSdkMap.computeIfAbsent(sdk, HashSet::new); | |
| templatePaths.add(relativeTemplatePath); | |
| } | |
| /** | |
| * Synchronizes extra IDEasy tool installations into the current IDE workspace configuration if supported. | |
| * | |
| * <p> | |
| * By default nothing will happen. Your IDE commandlet has to register one or more according templates in its constructor. | |
| * </p> | |
| */ | |
| protected void synchronizeExtraToolInstallations() { | |
| ExtraTools extraTools = ExtraToolsMapper.get().loadJsonFromFolder(this.context.getSettingsPath()); | |
| if (extraTools == null) { | |
| return; | |
| } | |
| for (String sdk : extraTools.getSortedToolNames()) { | |
| Set<Path> templatePaths = this.extraSdkMap.get(sdk); | |
| if ((templatePaths == null) || templatePaths.isEmpty()) { | |
| LOG.debug("Skipping import of extra tool {} into {} because not configured or supported.", sdk, this.tool); | |
| continue; | |
| } | |
| List<ExtraToolInstallation> extraInstallations = extraTools.getExtraInstallations(sdk); | |
| synchronizeExtraToolInstallation(sdk, templatePaths, extraInstallations); | |
| } | |
| } | |
| private void synchronizeExtraToolInstallation(String sdk, Set<Path> templatePaths, List<ExtraToolInstallation> extraInstallations) { | |
| for (Path templatePath : templatePaths) { | |
| Path workspaceFile = this.context.getWorkspacePath().resolve(templatePath); | |
| Path templateFile = this.context.getSettingsPath().resolve(this.tool).resolve(IdeContext.FOLDER_WORKSPACE) | |
| .resolve("repository") // TODO: create and use constant IdeContext.FOLDER_REPOSITORY - this string literal exists at least 16 times in our code base | |
| .resolve(templatePath); | |
| if (Files.exists(templateFile)) { | |
| for (ExtraToolInstallation extraInstallation : extraInstallations) { | |
| synchronizeExtraToolInstallation(sdk, templateFile, workspaceFile, extraInstallation); | |
| } | |
| } else { | |
| LOG.warn("You are missing a template file at {}."); | |
| IdeLogLevel.INTERACTION.log(LOG, "Please ask the IDEasy admin in your project to merge your settings with upstream."); | |
| } | |
| } | |
| } | |
| private void synchronizeExtraToolInstallation(String sdk, Path templateFile, Path workspaceFile, ExtraToolInstallation installation) { | |
| String name = installation.name(); | |
| Path extraToolHome = this.context.getSoftwareExtraPath().resolve(tool).resolve(name); | |
| if (!Files.isDirectory(extraToolHome)) { | |
| LOG.warn("Skipping extra tool installation import to {} because it is missing at {}", this.tool, extraToolHome); | |
| IdeLogLevel.INTERACTION.log(LOG, "Please run the following command to fix:\nide update"); | |
| return; | |
| } | |
| ExtensibleEnvironmentVariables environmentVariables = new ExtensibleEnvironmentVariables( | |
| (AbstractEnvironmentVariables) this.context.getVariables().getParent(), this.context); | |
| String variablePrefix = "EXTRA_" + tool.toUpperCase(Locale.ROOT); | |
| environmentVariables.setValue(variablePrefix + "_NAME", name); | |
| environmentVariables.setValue(variablePrefix + "_HOME", extraToolHome.toString().replace('\\', '/')); | |
| environmentVariables.setValue(variablePrefix + "_VERSION", installation.version().toString()); | |
| if (extraInstallation.edition() != null) { | |
| environmentVariables.setValue(variablePrefix + "_EDITION", installation.edition()); | |
| } | |
| XmlMerger xmlMerger = new XmlMerger(this.context); | |
| XmlMergeDocument workspaceDocument = xmlMerger.load(workspaceFile); | |
| XmlMergeDocument templateDocument = xmlMerger.loadAndResolve(templateFile, environmentVariables); | |
| Document mergedDocument = xmlMerger.merge(templateDocument, workspaceDocument, false); | |
| xmlMerger.save(mergedDocument, workspaceFile); | |
| } |
| /** | ||
| * IntelliJ-specific configuration describing how an extra IDEasy tool installation can be imported as an SDK/tool definition into IntelliJ configuration. | ||
| * | ||
| * @param templateFile the IntelliJ XML template file used as merge source. | ||
| * @param targetFile the IntelliJ configuration file relative to the workspace root that should be updated. | ||
| * @param reservedName an optional reserved logical name that must not be reused by an extra installation because it would collide with the main SDK/tool | ||
| * definition. | ||
| */ | ||
| private record IntellijExtraSdkConfig(String templateFile, String targetFile, String reservedName) { | ||
|
|
||
| } | ||
|
|
||
| /** | ||
| * Mapping of IDEasy tool names to IntelliJ-specific SDK import configuration. | ||
| * | ||
| * <p> | ||
| * Only tools contained in this map are imported automatically into IntelliJ. This keeps the import logic generic for IntelliJ while allowing support to be | ||
| * added incrementally tool by tool. | ||
| * </p> | ||
| */ | ||
| private static final Map<String, IntellijExtraSdkConfig> EXTRA_TOOL_CONFIGS = Map.of( | ||
| "java", new IntellijExtraSdkConfig("jdk-extra-java.xml", ".intellij/config/options/jdk.table.xml", "java") | ||
| ); | ||
|
|
There was a problem hiding this comment.
Nice that you solved this in a generic way.
IMHO this generic pattern is not specific for IntelliJ.
I suggested how to solve this fully generic in IdeToolCommandlet.
| /** | |
| * IntelliJ-specific configuration describing how an extra IDEasy tool installation can be imported as an SDK/tool definition into IntelliJ configuration. | |
| * | |
| * @param templateFile the IntelliJ XML template file used as merge source. | |
| * @param targetFile the IntelliJ configuration file relative to the workspace root that should be updated. | |
| * @param reservedName an optional reserved logical name that must not be reused by an extra installation because it would collide with the main SDK/tool | |
| * definition. | |
| */ | |
| private record IntellijExtraSdkConfig(String templateFile, String targetFile, String reservedName) { | |
| } | |
| /** | |
| * Mapping of IDEasy tool names to IntelliJ-specific SDK import configuration. | |
| * | |
| * <p> | |
| * Only tools contained in this map are imported automatically into IntelliJ. This keeps the import logic generic for IntelliJ while allowing support to be | |
| * added incrementally tool by tool. | |
| * </p> | |
| */ | |
| private static final Map<String, IntellijExtraSdkConfig> EXTRA_TOOL_CONFIGS = Map.of( | |
| "java", new IntellijExtraSdkConfig("jdk-extra-java.xml", ".intellij/config/options/jdk.table.xml", "java") | |
| ); |
| */ | ||
| public Intellij(IdeContext context) { | ||
|
|
||
| super(context, "intellij", Set.of(Tag.INTELLIJ)); |
There was a problem hiding this comment.
Now all that we need in IntelliJ is the following:
| super(context, "intellij", Set.of(Tag.INTELLIJ)); | |
| registerExtraSdkTemplate("java", ".intellij/config/options/jdk.table.xml"); |
ATTENTION:
But you need to move the template again in ide-settings by another PR:
Instead of intellij/workspace/repository/.idea/jdk-extra-java.xml it should then be intellij/workspace/repository/.intellij/config/options/jdk.table.xml.
IMHO all other changes to IntelliJ below can be reverted.
There was a problem hiding this comment.
I created a new PR for ide-settings at devonfw/ide-settings#91.
| LOG.warn("No supported build descriptor was found for project import in {}", repositoryPath); | ||
| } | ||
|
|
||
| importExtraToolInstallations(repositoryPath); |
There was a problem hiding this comment.
This is a misunderstanding.
In #1166 we want to import some repository into Intellij.
Therefore we need to figure out the workspace of that repository in order to change the workspace where the repository is located (getWorkspacePath(repositoryPath)).
In #1676 we want to ensure that all extra SDKs are configured in the IJ config of the current workspace before we actually launch IntelliJ (this.context.getWorkspacePath()).
There was a problem hiding this comment.
I've addressed it in the latest commits. Thanks for the suggestion 😊
…-into-IDE- # Conflicts: # CHANGELOG.adoc
5 participants
This PR fixes #1676
Implemented changes:
$IDE_HOME/settings/ide-extra-tools.json.jdk.table.xmlto$IDE_HOME/settings/intellij/workspace/repository/.intellij/config/optionsas the template for importing extra Java SDKs into IntelliJ. (Related PR #1676: import extra sdks automatically into ide (new) ide-settings#91).extrainstallation name as the SDK name.ide updateso one update is enough.Testing instructions
Preconditions
Make sure the following files are available for testing:
$IDE_HOME/settings/ide-extra-tools.json$IDE_HOME/settings/intellij/workspace/repository/.intellij/config/options/jdk.table.xmlManual test
ide update.$IDE_HOME/workspaces/main/.intellij/config/options/jdk.table.xmlAutomated test
Run the IntelliJ-related tests, especially the update flow test coverage:
Checklist for this PR
Make sure everything is checked before merging this PR. For further info please also see
our DoD.
mvn clean testlocally all tests pass and build is successful#«issue-id»: «brief summary»(e.g.#921: fixed setup.bat). If no issue ID exists, title only.In Progressand assigned to you or there is no issue (might happen for very small PRs)with
internal