#2190: IDEasy destroys my python installation - #2384
Conversation
Coverage Report for CI Build 33412176589Coverage decreased (-0.03%) to 73.587%Details
Uncovered ChangesNo uncovered changes found. Coverage Regressions54 previously-covered lines in 3 files lost coverage.
Coverage Stats💛 - Coveralls |
hohwille
left a comment
There was a problem hiding this comment.
@Paras14 thanks for your PR. The way how you detect the python version is perfectly implemented. Test covers the feature nicely and according python mocked script adoption makes sense for testing. 👍
I left some review suggestion how I thought we should connect #2296 with this to fix the problem properly. Please have a look,
| // Restore the missing file | ||
| return createToolInstallation(installationPath, resolvedVersion, false, processContext, additionalInstallation); | ||
| // Version file is missing but tool allows this - restore the file and preserve the installation. | ||
| VersionIdentifier detectedVersion = detectInstalledVersion(installationPath, resolvedVersion); |
There was a problem hiding this comment.
I thought that we would simply use the new infrastructure of #2296 and integrate that directly in computeInstalledEditionAndVersion. This way you will also fix the ide get-version python currently returns a wrong result if the version file is lost.
Also the .ide.software.version file is actually a kind of hack inherited from devonfw-ide (legacy file is .devonfw.software.version.
My plan is to get more away from this on the long run.
The installed edition of regular local tools is actually determined from the link target of the software folder in the software repository and the version can be determined in the same way.
I would therefore suggest, that we simply rely on getInstalledVersion() what is already used in the installation routine.
See here:
IDEasy/cli/src/main/java/com/devonfw/tools/ide/tool/LocalToolCommandlet.java
Lines 369 to 409 in 740bba7
So Python can simply override getInstalledVersion() and if null is returned from super call, simply add fallbacks (what is currently in detectInstalledVersion).
|
|
||
| Release with new features and bugfixes: | ||
|
|
||
| * https://github.com/devonfw/IDEasy/issues/2190[#2190]: IDEasy destroys my python installation |
There was a problem hiding this comment.
I opened the next release. Please shift up.
This PR fixes #2190
Implemented changes:
LocalToolCommandletnow really writes the.ide.software.versionfile when it is missing. The branch added in #2190: IDEasy destroys python installation #2212 logged "restoring it" but never calledwriteVersionFile, so the file stayed missing and every following command showed the warning again and asked for a CVE version selection.detectInstalledVersion(installationPath, resolvedVersion). The default returns the requested version.Pythonoverrides it and determines the version that is really installed, first frompyvenv.cfg(only if it contains a patch version, becauseuvwrites just3.13for its own interpreters) and otherwise by callingpython --version. This prevents writing a version file that claims a version the virtual environment does not have, which can happen becauseuv syncmay recreate the venv with a different python version.createToolInstallation.PythonTestthat fails without these changes. The mocked python binary in theuvtest project now answers--version.Testing instructions
mvn clean testin theclimodule. The new testPythonTest.testInstallRestoresMissingVersionFileAndPreservesPackagescovers this fix. It installs python, adds a file tosite-packages, deletes.ide.software.versionand installs again. It asserts that the version file is restored with the correct version, that the installed packages are preserved and that no "Deleting corrupted installation" happens.LocalToolCommandlet.javaandPython.javatomainand runPythonTestagain. It fails because the version file is not restored.Note for reviewers: the test pins the OS to
SystemInfoMock.LINUX_X64, because the mocked python payload in theuvtest project has a Unix stylebinfolder that collides with thebintoScriptssymlink created on Windows. Therefore the Windows specific parts ofdetectInstalledVersion(bin path resolution,.exesuffix, parsing the real interpreter output) are not covered by automated tests.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