#1981: fix ide update failing on windows arm64 - #2254
Conversation
ad55a35 to
4d5c230
Compare
13cba4a to
3af7c8d
Compare
Coverage Report for CI Build 32706969753Coverage increased (+0.1%) to 73.574%Details
Uncovered ChangesNo uncovered changes found. Coverage Regressions27 previously-covered lines in 2 files lost coverage.
Coverage Stats💛 - Coveralls |
3af7c8d to
2b4c966
Compare
maybeec
left a comment
There was a problem hiding this comment.
Thanks for picking this one up 👍 The diagnosis matches the analysis in #1981 exactly — read the snapshot maven-metadata.xml, check which classifiers actually exist, and fall back — and reusing CachedValue for the parsed Document is the right building block. Commits follow #«issue-id»: «summary», the CHANGELOG entry sits under the correct upcoming milestone, and the fallback is deliberately kept narrow so Linux/macOS resolution is untouched. 👍
No blockers, so this is a comment review rather than request-changes. There is however one real scope gap and a chunk of logic that duplicates helpers MvnArtifact already provides.
Should-fix
- The reported command is still broken.
getSnapshotBaseVersion(...)returnsnullfor release versions, so the fallback only ever applies to--mode=snapshot. Plainide upgradeon a non-SNAPSHOT installation resolvesUpgradeMode.STABLE/UNSTABLE(IdeasyCommandlet#getConfiguredVersion), which yields a release version — and that path still produces the 404 from the issue. The issue's reproduction is "1. Install IDEasy on Windows on ARM. 2.ide upgrade", so as it standsfixes #1981is only half true. See the inline comment ongetMetadata. - Duplicated snapshot-version logic.
PATTERN_TIMESTAMPED_SNAPSHOTandgetSnapshotBaseVersion(String)re-implementMvnArtifact.SNAPSHOT_VERSION_PATTERN/MvnArtifact#isSnapshot()/MvnArtifact#getBaseVersion(), which already exist and are already exercised by the download-URL path. Both new members can be deleted — inline suggestion below. - The actual bug path has no test. All three new tests call the package-private
resolveSnapshotClassifier(Document, String, String)directly. The wiring ingetMetadataand the metadata-URL derivation are untested, which is why coverage moved only +0.02%. Per the TDD rule inAGENTS.mdthe first test should be the one that reproduces the bug — andSystemInfoMock+IdeTestContext#setSystemInfo+MvnRepositoryMock(WireMock-backed, already servesmaven-metadata.xmlfrom test resources) make that straightforward. - Probing metadata is not best-effort. If the extra
maven-metadata.xmlfetch fails,fetchXmlMetadatathrowsCliException("Failed to determine the latest version from ...")and aborts the whole upgrade with a message that no longer describes what was happening. It also means Windows ARM64 now always needs network ingetMetadata, even when the artifact is already in the local.m2repo. - Manual QA. CI has no windows-arm64 runner, so green checks prove nothing here. The
-Dos.arch=arm64simulation in the testing instructions exercises resolution but not actually running the x64 binary under emulation — this needs one real pass on Windows on ARM before merge.
PR hygiene
- The branch is BEHIND
main(mergeStateStatus: BEHIND) — please update it before merge. - Neither the PR nor #1981 has a milestone set; per
documentation/contributing/DoD.adocthat needs to be set before the issue is closed. - Minor: the CHANGELOG line restates the bug title ("ide update fails on windows@arm64") rather than the fix. Most neighbouring entries read as what changed, e.g.
Fall back to windows-x64 when no windows-arm64 artifact is published.
Details and concrete suggestions inline.
|
Automated tests cover Windows ARM64 artifact resolution and x64 fallback. I could not perform a runtime validation on physical Windows ARM64 hardware, so execution under Windows-on-ARM emulation still needs manual verification before merge. |
975885e to
e9f7bb9
Compare
e9f7bb9 to
a5ae61c
Compare
This PR fixes #1981
Implemented changes:
Testing instructions
Run the focused Maven repository tests:
mvn -pl cli -Dtest=MvnRepositoryTest testRun the complete test suite:
mvn clean testBuild and install the modified IDEasy version locally:
./build-local-dev.shOpen a new PowerShell session and simulate Windows ARM64:
$env:IDE_OPTIONS = "-Dos.arch=arm64"Verify the simulated architecture:
ide statusRun the previously failing snapshot upgrade:
ide upgrade --mode=snapshotVerify that IDEasy falls back to the windows-x64 artifact instead of failing with a 404 for windows-arm64.
Remove the architecture override after testing:
Remove-Item Env:IDE_OPTIONS -ErrorAction SilentlyContinueChecklist 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
internalChecklist for tool commandlets
Have you added a new
«tool»as commandlet? There are the following additional checks:«tool»«TOOL»_VERSIONand«TOOL»_EDITIONare honored by your commandlet