-
Notifications
You must be signed in to change notification settings - Fork 81
#1987: Fixed GUI not launching for snapshot versions #2036
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Changes from all commits
9cff824
2cad107
954a923
c4bca9f
a95c469
7ee54ba
c706386
90768ed
c5b014a
656193a
9264ef9
21c37d6
717c492
8cdd5de
e8a9a2c
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change | ||||||||||||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
|
@@ -11,8 +11,8 @@ | |||||||||||||||||||||||||||
| import com.devonfw.tools.ide.commandlet.Commandlet; | ||||||||||||||||||||||||||||
| import com.devonfw.tools.ide.context.IdeContext; | ||||||||||||||||||||||||||||
| import com.devonfw.tools.ide.process.ProcessContext; | ||||||||||||||||||||||||||||
| import com.devonfw.tools.ide.process.ProcessContextImpl; | ||||||||||||||||||||||||||||
| import com.devonfw.tools.ide.process.ProcessMode; | ||||||||||||||||||||||||||||
| import com.devonfw.tools.ide.property.FlagProperty; | ||||||||||||||||||||||||||||
| import com.devonfw.tools.ide.tool.ToolEditionAndVersion; | ||||||||||||||||||||||||||||
| import com.devonfw.tools.ide.tool.ToolInstallRequest; | ||||||||||||||||||||||||||||
| import com.devonfw.tools.ide.tool.ToolInstallation; | ||||||||||||||||||||||||||||
|
|
@@ -27,13 +27,16 @@ public class Gui extends Commandlet { | |||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||
| private static final Logger LOG = LoggerFactory.getLogger(Gui.class); | ||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||
| final FlagProperty enableExtendedLogging; | ||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||
| /** | ||||||||||||||||||||||||||||
| * @param context the {@link IdeContext}. | ||||||||||||||||||||||||||||
| */ | ||||||||||||||||||||||||||||
| public Gui(IdeContext context) { | ||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||
| super(context); | ||||||||||||||||||||||||||||
| addKeyword(getName()); | ||||||||||||||||||||||||||||
| enableExtendedLogging = add(new FlagProperty("--enableLogging", false, "-l")); | ||||||||||||||||||||||||||||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. For logging we already have properties like After reading the code, I figured out that the property is actually changing the process mode and enforces that we do not run in background.
Suggested change
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. BTW: For such special flags avoid adding short options like |
||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||
| @Override | ||||||||||||||||||||||||||||
|
|
@@ -45,7 +48,7 @@ public String getName() { | |||||||||||||||||||||||||||
| @Override | ||||||||||||||||||||||||||||
| protected void doRun() { | ||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||
| ProcessContext processContext = new ProcessContextImpl(this.context); | ||||||||||||||||||||||||||||
| ProcessContext processContext = context.newProcess(); | ||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||
| Java java = this.context.getCommandletManager().getCommandlet(Java.class); | ||||||||||||||||||||||||||||
| Mvn mvn = this.context.getCommandletManager().getCommandlet(Mvn.class); | ||||||||||||||||||||||||||||
|
|
@@ -58,9 +61,16 @@ protected void doRun() { | |||||||||||||||||||||||||||
| new ToolEditionAndVersion(VersionIdentifier.of("25.*")) | ||||||||||||||||||||||||||||
| ); | ||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||
| mvn.installTool(mavenToolInstallRequest); | ||||||||||||||||||||||||||||
| ToolInstallation mvnToolInstallation = mvn.installTool(mavenToolInstallRequest); | ||||||||||||||||||||||||||||
| ToolInstallation javaInstallation = java.installTool(javaToolInstallRequest); | ||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||
| /* Register the freshly installed mvn on the IDEasy managed PATH so that the IDEasy controlled maven is used to launch | ||||||||||||||||||||||||||||
| the GUI instead of any maven that happens to be on the system PATH. We install via installTool (software repository | ||||||||||||||||||||||||||||
| only) and therefore have to register the bin directory ourselves (install() would normally do this). I tried to achieve this alternatively via .withPathEntry(), however, this did not work as expected. | ||||||||||||||||||||||||||||
| This was tested on a Mac; potentially, withPathVariable() works correctly on Windows. | ||||||||||||||||||||||||||||
| */ | ||||||||||||||||||||||||||||
|
Comment on lines
+67
to
+71
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Per the Code-Documentation conventions, implementation comments should factually explain the why. This drops the first-person/speculative wording and wraps the long line to match the rest of the file:
Suggested change
|
||||||||||||||||||||||||||||
| context.getPath().setPath(mvn.getName(), mvnToolInstallation.binDir()); | ||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||
| LOG.debug("Starting GUI via commandlet"); | ||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||
| Path pomPath = context.getIdeInstallationPath().resolve("gui/pom.xml"); | ||||||||||||||||||||||||||||
|
|
@@ -69,9 +79,10 @@ protected void doRun() { | |||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||
| List<String> args = List.of( | ||||||||||||||||||||||||||||
| "-f", | ||||||||||||||||||||||||||||
| "-U", //required for latest snapshot versions | ||||||||||||||||||||||||||||
| "-f", //use specified POM file | ||||||||||||||||||||||||||||
| pomPath.toString(), | ||||||||||||||||||||||||||||
| "exec:exec", | ||||||||||||||||||||||||||||
| "org.codehaus.mojo:exec-maven-plugin:3.1.0:exec", | ||||||||||||||||||||||||||||
| "-Dexec.executable=java", | ||||||||||||||||||||||||||||
| "-Dexec.classpathScope=compile", | ||||||||||||||||||||||||||||
| "-Dexec.args=-classpath %classpath com.devonfw.ide.gui.AppLauncher" | ||||||||||||||||||||||||||||
|
|
@@ -81,6 +92,13 @@ protected void doRun() { | |||||||||||||||||||||||||||
| * We manually update the PATH entry with our java version, as by default IDEasy includes the SymLink under /projectname/software/java/bin in the PATH | ||||||||||||||||||||||||||||
| * In case of projects using older Java Versions, this is important as the java version of the project could potentially older. | ||||||||||||||||||||||||||||
| */ | ||||||||||||||||||||||||||||
| mvn.runTool(processContext.withPathEntry(javaInstallation.binDir()), ProcessMode.BACKGROUND_SILENT, args); | ||||||||||||||||||||||||||||
| ProcessMode processMode = this.enableExtendedLogging.isTrue() ? ProcessMode.DEFAULT : ProcessMode.BACKGROUND_SILENT; | ||||||||||||||||||||||||||||
| try { | ||||||||||||||||||||||||||||
| mvn.runTool(processContext.withPathEntry(javaInstallation.binDir()), processMode, args); | ||||||||||||||||||||||||||||
| } catch (Exception e) { | ||||||||||||||||||||||||||||
| LOG.error( | ||||||||||||||||||||||||||||
| "Failed to launch the GUI. If maven states issues with dependency resolution, check whether the maven M2 repo is enabled in your project.", | ||||||||||||||||||||||||||||
| e); | ||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||
|
Comment on lines
+96
to
+102
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. As written, any failure is logged and then swallowed, so
Suggested change
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Instead of adding an extra option flag or a log-level based hack - can't we always run maven in foreground but tell it to run BTW: All this feature about foreground vs. background is not related to the bugfix that snapshots cannot open GUI. It is easier to create two separate issues and according PRs. Maybe then we could already merge the snapshot bugfix and include it into the current release while now the PR is stuck. |
||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,45 @@ | ||
| package com.devonfw.tools.ide.tool.gui; | ||
|
|
||
| import org.junit.jupiter.api.Test; | ||
|
|
||
| import com.devonfw.tools.ide.context.AbstractIdeContextTest; | ||
| import com.devonfw.tools.ide.context.IdeTestContext; | ||
| import com.github.tomakehurst.wiremock.junit5.WireMockRuntimeInfo; | ||
| import com.github.tomakehurst.wiremock.junit5.WireMockTest; | ||
|
|
||
| /** | ||
| * Tests for {@link Gui}. | ||
| */ | ||
| @WireMockTest | ||
| class GuiTest extends AbstractIdeContextTest { | ||
|
|
||
| private static final String PROJECT_GUI = "gui"; | ||
|
|
||
| @Test | ||
| void testEnableLoggingUsesDefaultProcessMode(WireMockRuntimeInfo wmRuntimeInfo) { | ||
| // arrange | ||
| IdeTestContext context = newContext(PROJECT_GUI, wmRuntimeInfo); | ||
| Gui gui = new Gui(context); | ||
| gui.enableExtendedLogging.setValue(true); | ||
|
|
||
| // act | ||
| gui.run(); | ||
|
|
||
| // assert: with ProcessMode.DEFAULT, process output is captured as INFO log | ||
| assertThat(context).logAtInfo().hasMessage("Scanning for projects..."); | ||
| } | ||
|
|
||
| @Test | ||
| void testDisabledLoggingUsesBackgroundSilentMode(WireMockRuntimeInfo wmRuntimeInfo) { | ||
| // arrange | ||
| IdeTestContext context = newContext(PROJECT_GUI, wmRuntimeInfo); | ||
| Gui gui = new Gui(context); | ||
| // enableExtendedLogging is false by default → ProcessMode.BACKGROUND_SILENT | ||
|
|
||
| // act | ||
| gui.run(); | ||
|
|
||
| // assert: with ProcessMode.BACKGROUND_SILENT, process output is suppressed | ||
| assertThat(context).logAtInfo().hasNoMessage("Scanning for projects..."); | ||
| } | ||
| } |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1 @@ | ||
| <PlaceholderFile/> |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1 @@ | ||
| ${testbaseurl}/download/java/java/25.0.2/java-25.0.2.tgz |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1 @@ | ||
| ${testbaseurl}/download/mvn/mvn/3.9.0/mvn-3.9.0.tgz |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1 @@ | ||
| MAVEN_VERSION=3.9.0 |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,2 @@ | ||
| #!/bin/bash | ||
| echo java $* |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,2 @@ | ||
| #!/bin/bash | ||
| echo "Scanning for projects..." |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Other commandlet property fields are declared
public final(see e.g.AbstractUpdateCommandlet,CreateCommandlet). For consistency: