#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. 😃
There was a problem hiding this comment.
I think this problem is caused by configuring the workspace before the extra SDKs are installed. Instead, workspace synchronization should run after installation, so IntelliJ only tries to import SDKs that already exist on disk.
We should call it after the extra tools have been installed, not during workspace setup, so the SDK paths already exist when IntelliJ config is merged.
So, I made these changes in commit: a108c07
| 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
…elliJ sees available SDKs
| if ((tool != null) && (extraInstallation != null) && tool.equalsIgnoreCase(extraInstallation.name())) { | ||
| throw new IllegalArgumentException( | ||
| "Invalid extra installation name '" + extraInstallation.name() + "' for tool '" + tool | ||
| + "': the extra installation name must not be the same as the tool name."); | ||
| } | ||
|
|
There was a problem hiding this comment.
It might be worth considering handling the IllegalArgumentException here. Otherwise, the update command could fail due to a user misconfiguration.
One option could be to wrap the following call in a try-catch block in the caller (ExtraToolsJsonDeserializer#setProperty(...)):
this.result.addExtraInstallations(tool, installation);and log the validation error from the exception instead of aborting the entire update process. An error-level log message could be appropriate in this case.
There was a problem hiding this comment.
Hi @JoelAdbu, thanks for the suggestion.
I’d prefer not to catch this in ExtraToolsJsonDeserializer#setProperty(...). This is configuration validation, so an IllegalArgumentException here indicates the user's ide-extra-tools.json is inconsistent with the expected model. If we swallow it and continue, we end up with a partially applied update: some extra installations are processed, while others are silently skipped, and the resulting workspace state no longer matches the declared configuration.
Failing fast is safer here because it keeps the system in a predictable state and makes the problem immediately obvious. The current load path already handles JSON-loading errors at the file boundary, so we still get a single top-level failure for the config file.
If we want better UX, I’d rather improve that outer exception message or logging so the user can clearly see which file and entry are invalid, instead of hiding the error at the property level.
|
I went through the testing instructions and was able to reproduce the expected behavior successfully. The extra SDKs were imported correctly, and ide update reinstalled and synchronized the SDKs as expected. |
5 participants
This PR fixes #1676
Implemented changes:
1. Register the IntelliJ template
2. Read extra tool installations
$IDE_HOME/settings/ide-extra-tools.json.For example:
3. Keep SDKs synchronized
ide createoride updateand before the IDE starts.4. Validate configuration and handle errors
java.5. Added GraalVM reflection metadata to reflect-config.json
IntellijviaIdeToolCommandlet-based lookup.Testing instructions
1. Build the native executable
IDEasy/cli/target.2. Create a test project with custom settings
IDEasy/cli/target, create a new test project using the modified settings from my test branch foride-settings.3. Verify automatic SDK import in IntelliJ
All additional SDKs should be automatically imported into IntelliJ.
This step validates that workspace configuration and IDE settings are correctly applied (as IDEasy manages IDE configuration via workspace templates).
4. Validate
ide updatebehavior4.1 Prepare test scenario
4.2 Run update
4.3 Verify result
All previously removed SDKs are re-downloaded and reinstalled.
IntelliJ configuration is updated accordingly, as shown at the end of step 3.
The file
$IDE_HOME/workspaces/main/.intellij/config/options/jdk.table.xmlhas been updated with the extra SDK entries.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