#786: Upgrade commandlet #957

salimbouch · 2025-01-20T19:32:51Z

fixes #786

…-commandlet

…Easy into upgrade-commandlet

…-commandlet

salimbouch · 2025-01-21T11:00:17Z

cli/src/main/java/com/devonfw/tools/ide/repo/MavenRepository.java

+    }
+
+    // hardcoding the file extension seems wrong
+    String fileName = artifactId + "-" + version + "-" + classifier + ".tar.gz";


right now just hardcoding the file extension, that means this doesnt work for every maven artifact

Simply add an extra parameter for the extension - it should default to .jar.

Since this method isn't an API (this method is only being used by the method download, which retrieves the download link and downloads the tool), I added a param for extension and a param for a classifier to the download method, so that when we want to download a tool and want to provide an extension and a classifier, we can do that directly through the download method. This caused a couple more changes to the upper hierarchy.

salimbouch · 2025-01-21T11:01:24Z

cli/src/test/java/com/devonfw/tools/ide/commandlet/UpgradeCommandletTest.java

+  public void testSnapshotVersionComparisons() {
+
+    IdeTestContext context = newContext(PROJECT_BASIC);
+    UpgradeCommandlet uc = context.getCommandletManager().getCommandlet(UpgradeCommandlet.class);
+
+    assertThat(uc.isSnapshotNewer("2024.12.002-beta-12_18_02-SNAPSHOT", "2025.01.001-beta-20250118.022832-8")).isTrue();
+    assertThat(uc.isSnapshotNewer("2024.12.002-beta-01_01_02-SNAPSHOT", "2024.12.002-beta-20241218.023429-8")).isTrue();
+    assertThat(
+        uc.isSnapshotNewer("2024.12.002-beta-12_18_02-SNAPSHOT", "2024.12.002-beta-20241218.023429-8")).isFalse();
+    assertThat(uc.isSnapshotNewer("SNAPSHOT", "2024.12.002-beta-20241218.023429-8")).isFalse();
+    assertThat(uc.isSnapshotNewer("someUnknownFormat1", "someUnknownFormat2")).isFalse();
+  }


only testing the snapshot comparison method as I cant really test the run method with all the download and the script launching and extraction and so on ..

salimbouch · 2025-01-21T11:03:51Z

cli/src/main/java/com/devonfw/tools/ide/commandlet/UpgradeCommandlet.java

+    String scriptContent =
+        "@echo off\n" + "ping -n 4 127.0.0.1 > nul\n" + "C:\\Windows\\System32\\tar.exe -xzf \"%~1\" -C \"%~2\"\n"
+            + "del \"%~f0\"";
+


doesnt seem like the most elegant bat file but works. The timeout command doesnt work for some reason so using ping, and I needed to get the whole path of the tar.exe for it to extract.

hohwille

@salimbouch thanks for your PR. Very nice job solving the complex things of this story 👍
I left some review comments for improvement.
Please have a look.

hohwille · 2025-01-21T11:24:35Z

cli/src/main/java/com/devonfw/tools/ide/repo/MavenRepository.java

+  private String getBaseUrl(String groupId, String artifactId) {
+
+    if (isIdeasySnapshot(groupId, artifactId, IdeVersion.get())) {
+      return MAVEN_SNAPSHOTS;
+    }
+    return MAVEN_CENTRAL;
+  }


this and its usage is wrong.
You implemented MavenRepository specific for our use-case of downloading IDEasy.
IdeVersion.get() needs to be used outside of this class to do the version comparison.
In getMetadata you need to use the version argument that you ignoring here.

hohwille · 2025-01-21T11:29:43Z

cli/src/main/java/com/devonfw/tools/ide/repo/MavenRepository.java

+    // If it's a resolved snapshot version (contains timestamp), convert back to -SNAPSHOT format for the path
+    if (pathVersion.matches(".*-\\d{8}\\.\\d{6}-\\d+")) {
+      pathVersion = pathVersion.replaceAll("-\\d{8}\\.\\d{6}-\\d+", "-SNAPSHOT");
+    }


This is also misplaced here.
getMetadata(IDEASY_GROUP_ID, IDEASY_ARTIFACT_ID, VersionIdentifier.of("2024.12.002-beta")) should give us
https://repo1.maven.org/maven2/com/devonfw/tools/IDEasy/ide-cli/2024.12.002-beta/ide-cli-2024.12.002-beta.jar
The variant getMetadata(IDEASY_GROUP_ID, IDEASY_ARTIFACT_ID, VersionIdentifier.of("2024.12.002-beta"), "windows-x64", ".tar.gz") should give
https://repo1.maven.org/maven2/com/devonfw/tools/IDEasy/ide-cli/2024.12.002-beta/ide-cli-2024.12.002-beta-windows-x64.tar.gz
Where windows-x64 is called classifier in maven terminology and .tar.gz is the extension (aka type in maven POM but there excluding the dot if you want to make it POM compliant just pass tar.gz with jar as default and add the dot internally in method impl).

Now getMetadata behaves as you stated. However, I still can't really get rid of this regex. The passed version to the method getMetadata if the version is a snapshot looks like this: 2025.01.001-beta-20250121.023134-9.
We need to make a path out of it, that means we take that version and make 2025.01.001-beta-SNAPSHOT out of it so that the download link is sonatype.org/..../2025.01.001-beta-SNAPSHOT/...2025.01.001-beta-20250121.023134-9...

hohwille · 2025-01-21T11:30:55Z

cli/src/main/java/com/devonfw/tools/ide/repo/MavenRepository.java

+    SystemInfo sys = this.context.getSystemInfo();
+    String classifier = sys.getOs() + "-" + sys.getArchitecture();


Also this code is misplaced here and needs to be moved outside of MavenRepository.

done, now passing the classifier as arg to the download method.

hohwille · 2025-01-21T11:33:35Z

cli/src/main/java/com/devonfw/tools/ide/repo/MavenRepository.java

+      Path tmpDownloadFile = createTempDownload(MAVEN_METADATA_XML);
+
+      try {
+        this.context.getFileAccess().download(metadataUrl, tmpDownloadFile);
+        DocumentBuilderFactory factory = DocumentBuilderFactory.newInstance();
+        DocumentBuilder builder = factory.newDocumentBuilder();
+        Document doc = builder.parse(tmpDownloadFile.toFile());


It does not make sense to write the XML to the disc just to then read it.
We should directly load the file into XML in memory without writing temporary files to the disc.

done, not really sure if I used the right libraries for it, see method fetchXmlMetadata.

hohwille · 2025-01-21T11:34:35Z

cli/src/main/java/com/devonfw/tools/ide/repo/MavenRepository.java

+        String version = Optional.ofNullable(doc.getElementsByTagName("latest").item(0)).map(Element.class::cast)
+            .map(Element::getTextContent).orElseGet(
+                () -> Optional.ofNullable(doc.getElementsByTagName("release").item(0)).map(Element.class::cast)
+                    .map(Element::getTextContent)
+                    .orElseThrow(() -> new IllegalStateException("No latest or release version found in metadata")));


wouldn't XPath help here to make the code more compact and elegant?

hohwille · 2025-01-21T11:40:43Z

cli/src/main/java/com/devonfw/tools/ide/repo/MavenRepository.java

+        if (isIdeasySnapshot(groupId, artifactId, version)) {
+          version = resolveSnapshotVersion(groupId, artifactId, version);
+        }


this should also not be specific for IDEasy.
If someone provides a version ending with -SNAPSHOT then you should resolve it the way you are doing that for any artifact no matter if IDEasy or not.
Outside when you use all this specific for IDEasy and you have version 2025.01.001-beta-01_21_02-SNAPSHOT simply convert this back to 2025.01.001-beta-SNAPSHOT what seems easy and pass that as version to this method to make it work.

Following Separation of Concerns this class should be generic and make sense for any maven artfact so we can reuse it also for gcviewer, gcloganalyzer, cobigen, or whatever we like.
Everything that is specific for IDEasy and the way we build our version printed by ide -v needs to go to UpgradeCommandlet and related code outside of this repo package.

hohwille · 2025-01-21T11:41:18Z

cli/src/main/java/com/devonfw/tools/ide/repo/MavenRepository.java

+      } finally {
+        this.context.getFileAccess().delete(tmpDownloadFile);


will also be pointless then.

hohwille · 2025-01-21T11:42:09Z

cli/src/main/java/com/devonfw/tools/ide/repo/MavenRepository.java

+      Path tmpDownloadFile = createTempDownload(MAVEN_METADATA_XML);
+
+      try {
+        this.context.getFileAccess().download(metadataUrl, tmpDownloadFile);
+        DocumentBuilderFactory factory = DocumentBuilderFactory.newInstance();
+        DocumentBuilder builder = factory.newDocumentBuilder();
+        Document doc = builder.parse(tmpDownloadFile.toFile());
+
+        String timestamp = Optional.ofNullable(doc.getElementsByTagName("timestamp").item(0)).map(Element.class::cast)
+            .map(Element::getTextContent)
+            .orElseThrow(() -> new IllegalStateException("No timestamp found in snapshot metadata"));
+
+        String buildNumber = Optional.ofNullable(doc.getElementsByTagName("buildNumber").item(0))
+            .map(Element.class::cast).map(Element::getTextContent)
+            .orElseThrow(() -> new IllegalStateException("No buildNumber found in snapshot metadata"));


same here. Also this looks like code duplication that should be avoided by reusing code from a private method.

hohwille · 2025-01-21T11:43:13Z

cli/src/main/java/com/devonfw/tools/ide/repo/MavenRepository.java

+  private boolean isIdeasySnapshot(String groupId, String artifactId, String version) {
+
+    return IDEASY_GROUP_ID.equals(groupId) && IDEASY_ARTIFACT_ID.equals(artifactId) && version != null
+        && version.contains("SNAPSHOT");
+  }


As stated above

Suggested change

private boolean isIdeasySnapshot(String groupId, String artifactId, String version) {

return IDEASY_GROUP_ID.equals(groupId) && IDEASY_ARTIFACT_ID.equals(artifactId) && version != null

&& version.contains("SNAPSHOT");

}

private boolean isSnapshotVersion(String version) {

return version.endsWith("-SNAPSHOT");

}

hohwille · 2025-01-21T11:46:17Z

cli/src/main/java/com/devonfw/tools/ide/repo/MavenRepository.java

+  @Override
+  public Collection<ToolDependency> findDependencies(String groupId, String artifactId, VersionIdentifier version) {
+
+    return Collections.emptyList();


Suggested change

return Collections.emptyList();

// We could read POM here and find dependencies but we do not want to reimplement maven here.

// For our use-case we only download bundled packages from maven central so we do KISS for now.

return Collections.emptyList();

…-commandlet

…Easy into upgrade-commandlet

coveralls · 2025-01-22T18:57:42Z

Pull Request Test Coverage Report for Build 12915070560

Details

0 of 0 changed or added relevant lines in 0 files are covered.
174 unchanged lines in 4 files lost coverage.
Overall coverage decreased (-0.6%) to 67.819%

Files with Coverage Reduction	New Missed Lines	%
com/devonfw/tools/ide/commandlet/CommandletManagerImpl.java	9	88.46%
com/devonfw/tools/ide/context/IdeContext.java	15	63.92%
com/devonfw/tools/ide/repo/AbstractToolRepository.java	25	61.21%
com/devonfw/tools/ide/context/AbstractIdeContext.java	125	59.22%

Totals
Change from base Build 12891187018:	-0.6%
Covered Lines:	7234
Relevant Lines:	10277

💛 - Coveralls

salimbouch · 2025-01-22T19:12:14Z

Also changed the way I retrieve the latest version from metadata.xml. Have a look at this.
The latest element doesn't give the real latest snapshot version. Now I iterate over all version elements and get the latest.

salimbouch and others added 16 commits December 16, 2024 20:26

created the to be implemented class

65f66cc

added mavenrepo

2046171

implemented untested logic to maven repo download

ed00c87

minor changes

4a0386d

Merge branch 'main' of https://github.com/devonfw/ideasy into upgrade…

94b12b8

…-commandlet

Merge branch 'upgrade-commandlet' of https://github.com/salimbouch/ID…

6fd391a

…Easy into upgrade-commandlet

implemented ugrade comdlet

0e145e3

minor change

3892233

Merge branch 'main' of https://github.com/devonfw/ideasy into upgrade…

753758d

…-commandlet

added script to upgrade

fd6a287

fixed snapshot comparison, fixed bug in path construction

3f71234

added test

b754cef

Merge branch 'main' of https://github.com/devonfw/ideasy into upgrade…

82934bb

…-commandlet

added commandlet to nls

bc7b28d

added javadoc

b5525df

added missing jdoc

607210b

salimbouch self-assigned this Jan 21, 2025

salimbouch marked this pull request as ready for review January 21, 2025 10:58

salimbouch commented Jan 21, 2025

View reviewed changes

hohwille requested changes Jan 21, 2025

View reviewed changes

salimbouch added 4 commits January 22, 2025 15:16

Merge branch 'main' of https://github.com/devonfw/ideasy into upgrade…

a082f0f

…-commandlet

Merge branch 'upgrade-commandlet' of https://github.com/salimbouch/ID…

3d4f73c

…Easy into upgrade-commandlet

implemented requested changes

703b0a9

fixed test

bc519fb

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

#786: Upgrade commandlet #957

#786: Upgrade commandlet #957

salimbouch commented Jan 20, 2025

salimbouch Jan 21, 2025

hohwille Jan 21, 2025

salimbouch Jan 22, 2025 •

edited

Loading

salimbouch Jan 21, 2025

salimbouch Jan 21, 2025

hohwille left a comment

hohwille Jan 21, 2025

salimbouch Jan 22, 2025

hohwille Jan 21, 2025

salimbouch Jan 22, 2025 •

edited

Loading

hohwille Jan 21, 2025

salimbouch Jan 22, 2025

hohwille Jan 21, 2025

salimbouch Jan 22, 2025 •

edited

Loading

hohwille Jan 21, 2025

salimbouch Jan 22, 2025

hohwille Jan 21, 2025

salimbouch Jan 22, 2025

hohwille Jan 21, 2025

salimbouch Jan 22, 2025

hohwille Jan 21, 2025

salimbouch Jan 22, 2025

hohwille Jan 21, 2025

salimbouch Jan 22, 2025

hohwille Jan 21, 2025

salimbouch Jan 22, 2025

coveralls commented Jan 22, 2025

salimbouch commented Jan 22, 2025

		SystemInfo sys = this.context.getSystemInfo();
		String classifier = sys.getOs() + "-" + sys.getArchitecture();

		} finally {
		this.context.getFileAccess().delete(tmpDownloadFile);

#786: Upgrade commandlet #957

Are you sure you want to change the base?

#786: Upgrade commandlet #957

Conversation

salimbouch commented Jan 20, 2025

Choose a reason for hiding this comment

Choose a reason for hiding this comment

salimbouch Jan 22, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hohwille left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

salimbouch Jan 22, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

salimbouch Jan 22, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

coveralls commented Jan 22, 2025

Pull Request Test Coverage Report for Build 12915070560

Details

💛 - Coveralls

salimbouch commented Jan 22, 2025

salimbouch Jan 22, 2025 •

edited

Loading

salimbouch Jan 22, 2025 •

edited

Loading

salimbouch Jan 22, 2025 •

edited

Loading