#2313: Fixed python installation failing - #2330
Conversation
Coverage Report for CI Build 32126869284Coverage decreased (-0.007%) to 72.901%Details
Uncovered ChangesNo uncovered changes found. Coverage Regressions34 previously-covered lines in 3 files lost coverage.
Coverage Stats💛 - Coveralls |
maybeec
left a comment
There was a problem hiding this comment.
Thanks for digging into #2313, and especially for tracking down the stale .venv while reproducing it 👍 The root-cause write-up in the PR description made this easy to follow, and the stale nearestVulnerabilities you found is a real defect.
I checked out the branch and compared the CVE suggestion behaviour against main by running CveCheckInstallTest on the existing intellij fixture with different configured versions. Unfortunately the change to the nearest condition regresses behaviour that is explicitly documented in documentation/security.adoc, so I have to request changes here.
Blocker
ToolCommandlet.java:751—!version.isGreater(resolvedVersion)removes the "nearest safe upgrade" suggestion.documentation/security.adocdocumentside install mvn 3.6.2offeringOption 2: nearest (3.8.1 - safe), and3.8.1is greater than the requested3.6.2. Reproduced below in the inline comment.
Should-fix
ToolCommandlet.java:745— resettingnearestVulnerabilitiestolatestVulnerabilitiesraises the bar fornearestfrom "less CVEs than current" (as documented) to "as safe as latest", so partially-safer versions are no longer suggested once a fully safelatestexists. Inline comment contains an alternative that fixes the reported problem without either side effect, verified against the existing tests.- No regression test. Per
AGENTS.md/ DoD a bug fix starts with a failing test. Both fixes are testable with what is already there:- CVE logic:
CveCheckInstallTest+ theintellijfixture already contains safe and unsafe versions above and below the configured one; a second test project with a differentINTELLIJ_VERSIONis enough to pin thenearestsemantics (that is exactly how I reproduced the regression). .venvcleanup:PythonTest.testInstallOnIntelMacResolvesVersionFromUvNotIdeUrlsshows the uv interaction is mockable, so a test that pre-createssoftware/.venvand asserts the install succeeds (and the folder is gone) is feasible.
- CVE logic:
CHANGELOG.adocentry missing (the checkbox is unticked in the PR description). Both defects shipped in released versions — CVE suggestions since #1145, so this is user-facing and needs an entry under== 2026.08.001, unless the issue gets theinternallabel.Python.java:57— redundantFiles.existsguard, see inline comment.Python.java:56—".venv"is now a literal in two places in this class;coding-conventions.adoc§ Constants asks for astatic finalconstant.- Scope vs. the issue. #2313 reproduces with
ide python install 3.14.0(which runs the python binary with the argumentsinstall 3.14.0— that is what thecan't open file '...\software\install'in the issue log comes from), while the testing instructions here useide install python 3.14.0. Please confirm what the reporter's exact command does after this PR, or state in the issue that the command was mistyped and which part of the reported behaviour is actually fixed. - Manual QA. The uv-driven python installation is not exercised end-to-end in CI, so please verify the stale-
.venvscenario manually (interrupt an install, then re-install) on Windows and at least one Unix OS before merging.
Housekeeping
- The branch is
BEHINDmain — please update it before merging. - Neither the PR nor issue #2313 has a milestone set.
- Non-blocking, pre-existing and outside the diff:
Python.java:31declaresprivate final VersionIdentifier PYTHON_MIN_VERSION— a constant name on a non-static field initialized at its declaration (coding-conventions.adoc§ Constants and § Field Initialization). Nice to clean up while you are in this file.
3c2ea29 to
77e3cb0
Compare
|
@maybeec thanks for the Review! You were totally right with the problem of the nearest version. I've implemented your suggestion and added the changelog entry. |
maybeec
left a comment
There was a problem hiding this comment.
Thanks for turning this around so quickly, and for taking the alternative from the last round seriously. I re-checked out the branch and re-ran the tests: the CVE behaviour is correct again - INTELLIJ_VERSION=2022.3 offers nearest (2022.3.2 - safe) as documented in documentation/security.adoc, and the new post-loop guard does suppress the useless suggestion from #2313. CHANGELOG.adoc, the milestone and the merge with main are all sorted now.
One correction on my side: I wrote == 2026.08.001 for the changelog entry last time. The milestone is release:2026.08.002, so where you put it is the right place.
Status of the previous review
Addressed:
- Blocker
ToolCommandlet.java:751- the!inversion is effectively reverted, the "nearest safe upgrade" suggestion is back. - Should-fix
ToolCommandlet.java:745-nearestVulnerabilitiesreset reverted, post-loop guard adopted. - Should-fix
Python.java- redundantFiles.existsguard removed,.venvliteral extracted intoVENV_FOLDER. - Should-fix
CHANGELOG.adocentry added (please also tick the checkbox in the PR description). - Housekeeping - branch merged with
main(MERGEABLE/CLEAN), milestone set on both PR and issue.
Still open - details below.
Blocker
- Out-commented code in
ToolCommandlet.java(lines 745 and 751). Both reverts were done by commenting out rather than deleting.documentation/contributing/coding-conventions.adoc(Clean Code section) states explicitly: "Do not push out-commented code". The/*!*/on line 751 is the worse of the two - a disabled operator sitting inside a live boolean expression is one careless edit away from silently reintroducing exactly the regression we just removed, and it hides from the next reader that this line is the load-bearing part of the documentednearestsemantics. Please delete both; git history is what keeps the old variant.
Should-fix
-
Still no regression test. This was the main open item last round and it is unchanged: the diff touches no test at all. Per
AGENTS.mdanddocumentation/contributing/DoD.adoca bug fix starts with a failing test - otherwise nothing stops the next refactoring ofcveCheck()from reintroducing either defect, which is exactly how we got here. I did the work to show it is cheap; the proposal below is verified, not hypothetical. -
Scope vs. issue #2313. Unchanged: the issue reproduces with
ide python install 3.14.0(which runs the python binary with the argumentsinstall 3.14.0- that is wherecan't open file '...\software\install'in the reported log comes from), while the testing instructions here useide install python 3.14.0. There is still no comment on the issue clarifying this. Please either confirm what the reporter's exact command does after this PR, or note on the issue that the command was mistyped and which part of the reported behaviour is actually fixed - otherwise we close #2313 without knowing whether the reporter's symptom is gone. -
Manual QA. Unchanged: the uv-driven python installation is not exercised end-to-end in CI. Please confirm in the PR that you interrupted an install and re-installed successfully on Windows and at least one Unix OS.
Change proposal (verified locally)
I wrote the three missing tests and validated each one both ways - every test below fails without the corresponding production change and passes with it. Verified on this branch on Windows via mvn -Dtest=CveCheckInstallTest test and mvn -Dtest=PythonTest test.
Two notes that cost me some time and will save you the same:
newContext(...)already resolves the variables, so writingsettings/ide.propertiesafterwards has no effect unless you callcontext.reload().- The shared
intellijCVE fixture cannot reproduce the #2313 end state: every version below the configured one is safer, so a legitimatenearestalways overwrites the bogus one. Copyingsrc/test/resources/urlsinto the project'stargetfolder and rewritingsecurity.jsonthere keeps the git-tracked fixture untouched (junit-testing.adoc: never write intosrc/...) and makes the defect reproducible.
cli/src/test/java/com/devonfw/tools/ide/tool/CveCheckInstallTest.java:
import com.devonfw.tools.ide.context.AbstractIdeContextTest;
import com.devonfw.tools.ide.context.IdeTestContext;
+import com.devonfw.tools.ide.io.FileAccess;
+import com.devonfw.tools.ide.io.FileCopyMode;
import com.devonfw.tools.ide.tool.intellij.Intellij;
import com.devonfw.tools.ide.tool.repository.ToolRepository;
@@
protected static final Path URLS_PATH = Path.of("src/test/resources/urls");
private static final String PROJECT_INTELLIJ = "intellij";
+
+ private static final String SECURITY_JSON_EQUAL_SEVERITY = """
+ {
+ "issues": [
+ {
+ "id": "CVE-TEST-EQUAL",
+ "severity": 5.0,
+ "versions": [
+ "(0,2025.1.1)"
+ ]
+ },
+ {
+ "id": "CVE-TEST-HIGH",
+ "severity": 9.0,
+ "versions": [
+ "[2025.1.1]"
+ ]
+ }
+ ]
+ }
+ """;
private final IdeTestContext context = newContext(PROJECT_INTELLIJ);
@@ (appended at the end of the class)
+ /**
+ * Install Intellij with a configured version for which a version above it is the nearest safe upgrade. Ensures the documented "nearest" suggestion (see
+ * {@code documentation/security.adoc}) also offers versions greater than the configured one.
+ */
+ @Test
+ void testInstallToolNearestSuggestsMinimalSafeUpgrade() {
+ //arrange
+ IdeTestContext context = newContext(PROJECT_INTELLIJ);
+ context.setUrlsPath(URLS_PATH);
+ context.getFileAccess().writeFileContent("JAVA_VERSION=17.0.10_7\nINTELLIJ_VERSION=2022.3\n", context.getSettingsPath().resolve("ide.properties"));
+ context.reload();
+ Intellij commandlet = new Intellij(context);
+ context.setAnswers("nearest");
+
+ //act
+ commandlet.install();
+
+ //assert
+ assertThat(context.getSoftwarePath().resolve("intellij").resolve(IdeTestContext.FILE_SOFTWARE_VERSION)).exists().hasContent("2022.3.2");
+ }
+
+ /**
+ * Install Intellij where the only allowed version above the configured one has exactly the same CVEs. Such a version must never be offered as "nearest"
+ * since it does not reduce the vulnerabilities - this is the bug reported in <a href="https://github.com/devonfw/IDEasy/issues/2313">#2313</a>.
+ */
+ @Test
+ void testInstallToolWithoutNearestIfNotSaferThanCurrent() {
+ //arrange
+ IdeTestContext context = newContext(PROJECT_INTELLIJ);
+ Path urlsPath = context.getIdeRoot().resolve("cve-urls");
+ FileAccess fileAccess = context.getFileAccess();
+ fileAccess.mkdirs(urlsPath);
+ fileAccess.copy(URLS_PATH, urlsPath, FileCopyMode.COPY_TREE_OVERRIDE_TREE);
+ fileAccess.writeFileContent(SECURITY_JSON_EQUAL_SEVERITY, urlsPath.resolve("urls").resolve("intellij").resolve("security.json"));
+ context.setUrlsPath(urlsPath.resolve("urls"));
+ Intellij commandlet = new Intellij(context);
+ context.setAnswers("current");
+
+ //act
+ commandlet.install();
+
+ //assert
+ assertThat(context).logAtInteraction().hasMessageContaining("latest (2025.1.1.1 - safe)");
+ assertThat(context).logAtInteraction().hasNoMessageContaining("nearest (");
+ }testInstallToolNearestSuggestsMinimalSafeUpgradepins the behaviour the first version of this PR broke. Against the original!version.isGreater(...)variant it errors out (nonearestoption to answer); against the current branch it is green.testInstallToolWithoutNearestIfNotSaferThanCurrentpins the actual #2313 fix. Againstmainit fails withFound unexpected log entry: Option 2: nearest (2024.2.6 - unsafe)- a suggestion with exactly the same max severity as the configured version, i.e. the "why is it asking about 3.14.7?" symptom. Against the current branch it is green.
cli/src/test/java/com/devonfw/tools/ide/tool/python/PythonTest.java:
+ /**
+ * Test that a stale {@code .venv} folder left behind by a previously interrupted installation is removed before {@code uv venv} is invoked, see
+ * <a href="https://github.com/devonfw/IDEasy/issues/2313">#2313</a>.
+ */
+ @Test
+ public void testInstallRemovesStaleVenvFolder(WireMockRuntimeInfo wireMockRuntimeInfo) {
+
+ // arrange
+ IdeTestContext context = newContext(PROJECT_UV, wireMockRuntimeInfo);
+ context.setSystemInfo(SystemInfoMock.MAC_X64);
+ Path staleMarker = context.getSoftwarePath().resolve(".venv").resolve("stale.txt");
+ context.getFileAccess().writeFileContent("stale", staleMarker, true);
+ Python python = context.getCommandletManager().getCommandlet(Python.class);
+
+ // act
+ python.install();
+
+ // assert
+ assertThat(context.getSoftwarePath().resolve("python").resolve("stale.txt")).doesNotExist();
+ assertThat(context.getSoftwarePath().resolve(".venv")).doesNotExist();
+ assertThat(context).logAtSuccess().hasMessageContaining("Successfully installed python");
+ }The marker file is what makes this a real test rather than a tautology: without your fileAccess.delete(venvPath) the mocked uv venv populates the existing .venv, so the stale content is moved into software/python and stale.txt survives the install. Against main the test fails on exactly that; against the current branch it is green.
Feel free to adjust naming or fold the two CVE tests into one - the important part is that both defects are pinned. Note that staleMarker repeats the ".venv" literal, so you may want to make VENV_FOLDER package-private and reuse it in the test.
Minor (non-blocking)
Python.java:33-VENV_FOLDERis a static constant declared after the instance fieldPYTHON_MIN_VERSION, and it has no JavaDoc. See inline comment.Python.java:31- pre-existing, mentioned last time and still open:private final VersionIdentifier PYTHON_MIN_VERSIONis a constant name on a non-static field initialized at its declaration (coding-conventions.adoc, sections Constants and Field Initialization). Nice to clean up while you are in this file.- The
CHANGELOG.adoccheckbox in the PR description is still unticked although the entry exists.
Co-authored-by: Malte Brunnlieb <maybeec@users.noreply.github.com>
Co-authored-by: Malte Brunnlieb <maybeec@users.noreply.github.com>
Co-authored-by: Malte Brunnlieb <maybeec@users.noreply.github.com>
…-installation-fails-with-wired-behaviour' into feature/2313-python-installation-fails-with-wired-behaviour # Conflicts: # cli/src/main/java/com/devonfw/tools/ide/tool/python/Python.java
|
Thanks for the Re-Review and the proposed tests. Regarding your questions: This is the output now, when i run I've also installed & uninstalled Python successfully on Windows and WSL. |
hohwille
left a comment
There was a problem hiding this comment.
@samuelkos17 thanks for your PR. Great analysis, fix and test improvement 👍
@maybeec thanks for your excellent review support.
I left some minor suggestions to keep our code clean but then this is ready to merge.
Functionally all is already perfect and I also tested it and can confirm it fixes the bug.
| Path venvPath = softwarePath.resolve(VENV_FOLDER); | ||
|
|
||
| fileAccess.delete(venvPath); | ||
|
|
There was a problem hiding this comment.
Nothing wrong here. If I understand it correctly, uv.installPython might fail if the .venv folder was already present (e.g. from a previous incomplete setup that was aborted or failed).
Shouldn't this be considered as a bug of installPython method so it should be fixed there?
We can also keep it as is - I just want to understand and challenge this...
There was a problem hiding this comment.
Yes, you understood that correctly. I've encountered this issue while reproducing the bug. And yeah you're right, since this specifically concerns Uv it should be understood as a bug of the installPython() method of Uv.java. However, moving the bugfix there would require a few more changes than if we just keep it this way, what do you say should I move it there or keep it here?
…make it static Co-authored-by: Jörg Hohwiller <hohwille@users.noreply.github.com>
This PR fixes #2313
Implemented changes:
ToolCommandlet.cveCheck(), when a safer version aboveresolvedVersionwas found and assigned tolatest,nearestwas reset to null butnearestVulnerabilitieswas left stale. This caused subsequent versions with anequal vulnerability profile to incorrectly claim the nearest slot via a flawed else if branch that compared against outdated CVE data.nearestVulnerabilities = latestVulnerabilitieswhenever nearest is reset to null to prevent stale comparisons.version.isGreater(resolvedVersion)to!version.isGreater(resolvedVersion)(nearest should only track versions below resolvedVersion, not above it).uv venv --pythoncreates a .venv directory in the software folder before it gets renamed to the target python/ folder. If a previous installation or interrupted install left a stale .venv directory behind, uv venv would fail with "A virtual environment already exists", because only the python/ target was backed up, not the intermediate .venv.Python.performToolInstallation()to delete any existing .venv directory before invokinguv venv.Testing instructions
Please add conscise, understandable instructions on how a reviewer can test/verify the functionality of your contribution here:
build-local-dev.shand runide install python 3.14.0.3.14.0and3.14.7should be printed. Select3.14.0.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.batand notfeature/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
internalpom.xmlfiles or otherwise if runtime dependencies changed, you have updated our LICENSE.asciidoc