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.

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

Conversation

mvomiero
Copy link
Contributor

Fixes: #400

-    if (currentDir == null) {
-      info(getMessageIdeHomeNotFound());
-    } else {
+   if (currentDir != null) {

@mvomiero mvomiero self-assigned this Jun 21, 2024
@coveralls
Copy link
Collaborator

coveralls commented Jun 21, 2024

Pull Request Test Coverage Report for Build 9610922840

Details

  • 0 of 0 changed or added relevant lines in 0 files are covered.
  • 71 unchanged lines in 1 file lost coverage.
  • Overall coverage decreased (-0.03%) to 60.64%

Files with Coverage Reduction New Missed Lines %
com/devonfw/tools/ide/context/AbstractIdeContext.java 71 56.55%
Totals Coverage Status
Change from base Build 9610011440: -0.03%
Covered Lines: 4938
Relevant Lines: 7833

💛 - Coveralls

@@ -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:

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

It is already printed here:
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...

@coveralls
Copy link
Collaborator

coveralls commented Jun 27, 2024

Pull Request Test Coverage Report for Build 9694897319

Details

  • 0 of 0 changed or added relevant lines in 0 files are covered.
  • 64 unchanged lines in 1 file lost coverage.
  • Overall coverage decreased (-0.02%) to 60.317%

Files with Coverage Reduction New Missed Lines %
com/devonfw/tools/ide/context/AbstractIdeContext.java 64 56.71%
Totals Coverage Status
Change from base Build 9693903147: -0.02%
Covered Lines: 4984
Relevant Lines: 7950

💛 - Coveralls

@coveralls
Copy link
Collaborator

coveralls commented Jun 27, 2024

Pull Request Test Coverage Report for Build 9694945996

Details

  • 0 of 0 changed or added relevant lines in 0 files are covered.
  • 64 unchanged lines in 1 file lost coverage.
  • Overall coverage decreased (-0.02%) to 60.317%

Files with Coverage Reduction New Missed Lines %
com/devonfw/tools/ide/context/AbstractIdeContext.java 64 56.71%
Totals Coverage Status
Change from base Build 9693903147: -0.02%
Covered Lines: 4984
Relevant Lines: 7950

💛 - Coveralls

@hohwille hohwille self-assigned this Jun 28, 2024
@hohwille hohwille linked an issue Jun 28, 2024 that may be closed by this pull request
@hohwille
Copy link
Member

hohwille commented Jun 28, 2024

Seems with my PR #424 I ran into so many small problems that I fixed, that we overlapped.
I did the same fix as this PR in #424 but my fix is more sophisticated since it also fixes various other problems.

@hohwille
Copy link
Member

@mvomiero thanks for creating this PR and to fix bug #400. 👍
Sorry, that I meanwhile did the same fix. Due to the review discussions, it seemed easier to apply my fix with PR #476. Hence closing this PR.

@hohwille hohwille closed this Jul 12, 2024
@hohwille hohwille added this to the rejected milestone Jul 12, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: ✅ Done
Development

Successfully merging this pull request may close these issues.

ide output outside of an IDEasy installation
3 participants