#1965: improve dependent installation - #2246
Conversation
|
Please, when checking the code give extra attention to ToolInstallRequest.detectInstallLoopRecursively, since that's where I suspect the new changes might have unsuspected failures. The tests do pass, but maybe edge cases are not covered |
Coverage Report for CI Build 30882542201Coverage increased (+0.03%) to 72.616%Details
Uncovered ChangesNo uncovered changes found. Coverage Regressions25 previously-covered lines in 4 files lost coverage.
Coverage Stats
💛 - Coveralls |
There was a problem hiding this comment.
What works well
The root cause is correctly identified. runPackageManager built the nested install request without a parent, so isInstallLoop() could never see the cycle. A repeated ide install nest now skips the extra npm installation cycle that ran before, which is what the issue asks for.
NpmBasedCommandlet.java:68 and PipBasedCommandlet.java:113 also bring both callers in line with the contract already documented on computeInstalledVersion.
Findings
1. PackageManagerBasedLocalToolCommandlet.java:94 no longer uses pc.createChild(). The nested install now shares the caller's ProcessContext, including errorHandling and exitCodeAcceptor, which the child deliberately resets (ProcessContextImpl.java:84). Restoring it has to happen inside the constructor, since setProcessContext rejects a second write.
2. ToolInstallRequest.java:69 hard-codes silent to false, the replaced code passed true. With the current ng fixture this moves the "already installed" lines for node and npm from DEBUG to INFO. Was that intended, or should the flag come from the parent request?
3. The fixed duplication has no test. computeInstalledVersion is only reached on a repeated installation, so a second install() in NgTest pins it. The mock npm in the ng fixture needs a list branch for this, it currently answers --version only:
commandlet.install();
context.getTestStartContext().getEntries().clear();
commandlet.install();
assertThat(context).log().hasNoMessageContaining("No CVEs found for version 9.9.2 of tool npm");4. Step 1 of the testing instructions has no effect. ide uninstall npm refuses the uninstall because Npm.canBeUninstalled() is false, and still prints "Successfully uninstalled npm". Running ide install nest twice reproduces the duplication reliably and would be the clearer instruction.
Optional: one parent field instead of two
Both inline bugs come from two mutually exclusive parent fields with two constructors that must be kept in sync. Resolving the parent in runPackageManager via request.getToolInstallRequest() keeps a single field, and the nested request then inherits silent and ignoreProject automatically.
Nits
ToolInstallRequest.java:90: the private constructor has no javadoc, and itssilentanddirectparameters are alwaysfalseat the single call site.PackageManagerBasedLocalToolCommandlet.java:93lost the// See Node.postInstallOnNewInstallationpointer that explained the npm/node cycle.
| } else if (this.parentPackageManagerRequest != null) { | ||
| if (this.parentPackageManagerRequest.getType().equals("uninstall")) { | ||
| return false; | ||
| } | ||
| loopFound = this.parentPackageManagerRequest.getToolInstallRequest().detectInstallLoopRecursively(toolEditionAndVersion, sb); |
There was a problem hiding this comment.
getToolInstallRequest() is null for every request that carries no tool installation, so only the hard-coded "uninstall" comparison keeps this from dereferencing null. The public one-arg runPackageManager makes other types reachable. Guarding on the parent itself covers the uninstall case too, since those requests never carry one.
| } else if (this.parentPackageManagerRequest != null) { | |
| if (this.parentPackageManagerRequest.getType().equals("uninstall")) { | |
| return false; | |
| } | |
| loopFound = this.parentPackageManagerRequest.getToolInstallRequest().detectInstallLoopRecursively(toolEditionAndVersion, sb); | |
| } else if (this.parentPackageManagerRequest != null) { | |
| // a package manager request without tool installation (e.g. uninstall) cannot be part of an installation loop | |
| ToolInstallRequest parentRequest = this.parentPackageManagerRequest.getToolInstallRequest(); | |
| if (parentRequest == null) { | |
| return false; | |
| } | |
| loopFound = parentRequest.detectInstallLoopRecursively(toolEditionAndVersion, sb); |
| if (parent != null) { | ||
| this.processContext = parent.getProcessContext(); | ||
| } |
There was a problem hiding this comment.
This constructor does not propagate ignoreProject, while the constructor above it does (line 86). An install request that ignores the project therefore loses that flag as soon as it goes through a package manager, silently and without any error.
| if (parent != null) { | |
| this.processContext = parent.getProcessContext(); | |
| } | |
| if (parent != null) { | |
| this.processContext = parent.getProcessContext(); | |
| ToolInstallRequest parentRequest = parent.getToolInstallRequest(); | |
| if (parentRequest != null) { | |
| this.ignoreProject = parentRequest.ignoreProject; | |
| } | |
| } |
| * https://github.com/devonfw/IDEasy/issues/1965[#1965]: Improve dependent installation, fixed npm and pip-based installation duplication | ||
| * https://github.com/devonfw/IDEasy/issues/2100[#2100]: Fix Python not available for Mac x64 |
There was a problem hiding this comment.
The #2100 entry already exists further down in the same release block, added by PR #2152.
| * https://github.com/devonfw/IDEasy/issues/1965[#1965]: Improve dependent installation, fixed npm and pip-based installation duplication | |
| * https://github.com/devonfw/IDEasy/issues/2100[#2100]: Fix Python not available for Mac x64 | |
| * https://github.com/devonfw/IDEasy/issues/1965[#1965]: Improve dependent installation, fixed npm and pip-based installation duplication |
| /** | ||
| * @param toolInstallRequest new value of {@link #getToolInstallRequest()} ToolInstallRequest()}. | ||
| * @return this {@link PackageManagerRequest} for fluent API calls. | ||
| */ | ||
| public PackageManagerRequest setToolInstallRequest( | ||
| ToolInstallRequest toolInstallRequest) { |
There was a problem hiding this comment.
The javadoc contains a copy/paste artifact, and the signature fits on one line within the 160 character limit.
| /** | |
| * @param toolInstallRequest new value of {@link #getToolInstallRequest()} ToolInstallRequest()}. | |
| * @return this {@link PackageManagerRequest} for fluent API calls. | |
| */ | |
| public PackageManagerRequest setToolInstallRequest( | |
| ToolInstallRequest toolInstallRequest) { | |
| /** | |
| * @param toolInstallRequest new value of {@link #getToolInstallRequest()}. | |
| * @return this {@link PackageManagerRequest} for fluent API calls. | |
| */ | |
| public PackageManagerRequest setToolInstallRequest(ToolInstallRequest toolInstallRequest) { |
3 participants
This PR fixes #1965
Implemented changes:
Testing instructions
-cp ide-cliwithide uninstall npmand run, wait to endide install nestChecklist 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