Skip to content
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.

Sign up for GitHub

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

#400: ide output outside IDEasy installation #405

Closed
wants to merge 7 commits into from
Closed

#400: ide output outside IDEasy installation #405

Changes from 3 commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
603deda
bugfix
mvomiero Jun 20, 2024
4b6238d
remove log idehomenotfound
mvomiero Jun 20, 2024
f5b0765
Merge branch 'main' into bug/400-IdeOutputOutsideInstallation
mvomiero Jun 21, 2024
b124f7d
Merge branch 'main' into bug/400-IdeOutputOutsideInstallation
mvomiero Jun 26, 2024
5fdac3c
Merge remote-tracking branch 'upstream/main' into bug/400-IdeOutputOu…
mvomiero Jun 27, 2024
791c321
reformat code
mvomiero Jun 27, 2024
8121fe7
reformat code
mvomiero Jun 27, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
15 changes: 7 additions & 8 deletions cli/src/main/java/com/devonfw/tools/ide/context/AbstractIdeContext.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -180,9 +180,7 @@ public AbstractIdeContext(IdeLogLevel minLogLevel, Function<IdeLogLevel, IdeSubL

// detection completed, initializing variables
Path ideRootPath = null;
if (currentDir == null) {
info(getMessageIdeHomeNotFound());
} else {
if (currentDir != null) {
debug(getMessageIdeHomeFound());
ideRootPath = currentDir.getParent();
}
Expand All @@ -200,7 +198,7 @@ public AbstractIdeContext(IdeLogLevel minLogLevel, Function<IdeLogLevel, IdeSubL
}
}
if (ideRootPath == null || !Files.isDirectory(ideRootPath)) {
error("IDE_ROOT is not set or not a valid directory.");
error("You are not inside an IDEasy installation: " + System.getProperty("user.dir"));
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actually this is a different error/warning. Even if IDE_HOME is not found because you are outside of an IDEasy project, the installer should set a environment variable IDE_ROOT on your OS (e.g. in Windows environment variables for user).
What you are actually looking for is this one:

IDEasy/cli/src/main/java/com/devonfw/tools/ide/context/AbstractIdeContext.java

Line 283 in ee596da

return "You are not inside an IDE installation: " + this.cwd;

It is already printed here:

IDEasy/cli/src/main/java/com/devonfw/tools/ide/context/AbstractIdeContext.java

Lines 183 to 184 in ee596da

if (currentDir == null) {
info(getMessageIdeHomeNotFound());

IMHO we need to investigate this further, because you removed the actual code that should print the expected message.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I rather think that the only problem is that info needs to be changed to error since the first is writing to standard out while the latter is writing to standard error.
Standard out is written into the variable for the variables to set using eval command.
If the exit code is not 0 that output is discarded by wrapper script.
You can test that is works properly in IntelliJ but the output is hidden by the wrapper script.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

changed info to error, the log was printed anyways two times.

I went for this new solution that avoids redundant code as well (calling getMessageIdeHomeNotFound() two times) :

-    if (ideRootPath == null || !Files.isDirectory(ideRootPath)) {
-      error("IDE_ROOT is not set or not a valid directory.");
-    }
+    if (ideRootPath == null || !Files.isDirectory(ideRootPath) || currentDir != null) {
+      error(getMessageIdeHomeNotFound());
+    }
  • Note: getMessageIdeHomeNotFound() hat to be changed because in some cases this.cwd was equal to null

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I already fixed this in PR #424 so that would replace this PR.
However, there is a strange bug happening on github that cannot be reproduced locally on any OS including linux preventing the merge of PR #424. Since I do not have time currently to investigate this further, I wanted to merge this PR and later rebase PR #424.
However, this is still incorrect: There are two more or less independent warnings:

  • IDE_ROOT is not defined
  • IDE_HOME is required but not defined

I do not see a duplication of messages and still think you are removing an error log message that was still intended.

$ ideasy --env
You are not inside an IDE installation: null
IDE_ROOT is not set or not a valid directory.

The actual problem is the null and the fact that our wrapper script is kind of buggy or the log-level of the IdeHome message is info but should be error.
Maybe I can find the time to reduce one of my PRs and get the actual bash-completion stuff out until it gets green and I can reuse it to fix #400. That would most probably be the best way to go to get this all sorted out quickly...

}
this.ideRoot = ideRootPath;

Expand Down Expand Up @@ -876,10 +874,10 @@ public int run(CliArguments arguments) {
}

/**
* @param cmd the potential {@link Commandlet} to
* {@link #apply(CliArguments, Commandlet, CompletionCandidateCollector) apply} and {@link Commandlet#run() run}.
* @return {@code true} if the given {@link Commandlet} matched and did {@link Commandlet#run() run} successfully,
* {@code false} otherwise (the {@link Commandlet} did not match and we have to try a different candidate).
* @param cmd the potential {@link Commandlet} to {@link #apply(CliArguments, Commandlet, CompletionCandidateCollector) apply} and
* {@link Commandlet#run() run}.
* @return {@code true} if the given {@link Commandlet} matched and did {@link Commandlet#run() run} successfully, {@code false} otherwise (the
* {@link Commandlet} did not match and we have to try a different candidate).
*/
private boolean applyAndRun(CliArguments arguments, Commandlet cmd) {

Expand Down Expand Up @@ -1044,6 +1042,7 @@ private String findBashOnWindows() {

@Override
public WindowsPathSyntax getPathSyntax() {

return this.pathSyntax;
}

Expand Down
Loading