You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@maven.apache.org by GitBox <gi...@apache.org> on 2021/05/14 12:55:07 UTC

[GitHub] [maven] MartinKanters opened a new pull request #472: [MNG-6915] Adapt the logging width to the terminal width, including sensible limits.

MartinKanters opened a new pull request #472:
URL: https://github.com/apache/maven/pull/472


   This PR uses the newly introduced Jansi's `getTerminalWidth()` to make the terminal a little bit more responsive.
   We used sensible defaults to ensure the logging output makes sense.
   This is especially useful for bigger screens.
   
   By the way, this does not fix issues around characters which are larger than one column. That is still working the same as in master. 
   
   Following this checklist to help us incorporate your 
   contribution quickly and easily:
   
    - [x] Make sure there is a [JIRA issue](https://issues.apache.org/jira/browse/MNG-6915) filed 
          for the change (usually before you start working on it).  Trivial changes like typos do not 
          require a JIRA issue.  Your pull request should address just this issue, without 
          pulling in other changes.
    - [x] Each commit in the pull request should have a meaningful subject line and body.
    - [x] Format the pull request title like `[MNG-XXX] - Fixes bug in ApproximateQuantiles`,
          where you replace `MNG-XXX` with the appropriate JIRA issue. Best practice
          is to use the JIRA issue title in the pull request title and in the first line of the 
          commit message.
    - [x] Write a pull request description that is detailed enough to understand what the pull request does, how, and why.
    - [x] Run `mvn clean verify` to make sure basic checks pass. A more thorough check will 
          be performed on your pull request automatically.
    - [x] You have run the [Core IT][core-its] successfully.
   
   If your pull request is about ~20 lines of code you don't need to sign an
   [Individual Contributor License Agreement](https://www.apache.org/licenses/icla.pdf) if you are unsure
   please ask on the developers list.
   
   To make clear that you license your contribution under 
   the [Apache License Version 2.0, January 2004](http://www.apache.org/licenses/LICENSE-2.0)
   you have to acknowledge this by using the following check-box.
   
    - [x] I hereby declare this contribution to be licenced under the [Apache License Version 2.0, January 2004](http://www.apache.org/licenses/LICENSE-2.0)
   
    - [x] In any other case, please file an [Apache Individual Contributor License Agreement](https://www.apache.org/licenses/icla.pdf).
   
   [core-its]: https://maven.apache.org/core-its/core-it-suite/
   


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [maven] gnodet edited a comment on pull request #472: [MNG-6915] Adapt the logging width to the terminal width, including sensible limits.

Posted by GitBox <gi...@apache.org>.
gnodet edited a comment on pull request #472:
URL: https://github.com/apache/maven/pull/472#issuecomment-844052080


   @MartinKanters I propose a slightly modified version at #473 .
   The reason is that the `ExecutionEventLogger` is not a singleton as I thought, so we can simply compute the values depending on the terminal width in fields.  Way easier and will work great with `mvnd` (where the terminal width is actually passed from the client to the daemon). 


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [maven] mthmulders commented on pull request #472: [MNG-6915] Adapt the logging width to the terminal width, including sensible limits.

Posted by GitBox <gi...@apache.org>.
mthmulders commented on pull request #472:
URL: https://github.com/apache/maven/pull/472#issuecomment-846377504


   Superseded by #473.


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [maven] gnodet commented on a change in pull request #472: [MNG-6915] Adapt the logging width to the terminal width, including sensible limits.

Posted by GitBox <gi...@apache.org>.
gnodet commented on a change in pull request #472:
URL: https://github.com/apache/maven/pull/472#discussion_r634395166



##########
File path: maven-embedder/src/main/java/org/apache/maven/cli/event/ExecutionEventLogger.java
##########
@@ -53,9 +54,15 @@
 {
     private final Logger logger;
 
-    private static final int LINE_LENGTH = 72;
+    private static final int MAX_LOG_PREFIX_SIZE = 8; // "[ERROR] "
+    private static final int PROJECT_STATUS_SUFFIX_SIZE = 20; // "SUCCESS [  0.000 s]"
+    private static final int MIN_TERMINAL_WIDTH = 60;
+    private static final int MAX_TERMINAL_WIDTH = 130;
+    private static final int TERMINAL_WIDTH =
+            Math.min( MAX_TERMINAL_WIDTH, Math.max( MessageUtils.getTerminalWidth(), MIN_TERMINAL_WIDTH ) );

Review comment:
       @MartinKanters maybe the `MavenExecutionRequest` would be a better place as it contains already a bunch of things somewhat related to cli output: loggingLevel, showError, etc...




-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [maven] MartinKanters commented on a change in pull request #472: [MNG-6915] Adapt the logging width to the terminal width, including sensible limits.

Posted by GitBox <gi...@apache.org>.
MartinKanters commented on a change in pull request #472:
URL: https://github.com/apache/maven/pull/472#discussion_r634710264



##########
File path: maven-embedder/src/main/java/org/apache/maven/cli/event/ExecutionEventLogger.java
##########
@@ -53,9 +54,15 @@
 {
     private final Logger logger;
 
-    private static final int LINE_LENGTH = 72;
+    private static final int MAX_LOG_PREFIX_SIZE = 8; // "[ERROR] "
+    private static final int PROJECT_STATUS_SUFFIX_SIZE = 20; // "SUCCESS [  0.000 s]"
+    private static final int MIN_TERMINAL_WIDTH = 60;
+    private static final int MAX_TERMINAL_WIDTH = 130;
+    private static final int TERMINAL_WIDTH =
+            Math.min( MAX_TERMINAL_WIDTH, Math.max( MessageUtils.getTerminalWidth(), MIN_TERMINAL_WIDTH ) );

Review comment:
       Hmm, it's definitely better, but I think it would be best if we could keep this term width state in `maven-embedder`.
   What if we store it as a regular field in `ExecutionEventLogger`? Or are you reusing the same instance of that as well in separate runs? Otherwise we could find some way to recalculate it every time `projectDiscoveryStarted()` is called or introduce a new method for it..




-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [maven] mthmulders closed pull request #472: [MNG-6915] Adapt the logging width to the terminal width, including sensible limits.

Posted by GitBox <gi...@apache.org>.
mthmulders closed pull request #472:
URL: https://github.com/apache/maven/pull/472


   


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [maven] gnodet commented on a change in pull request #472: [MNG-6915] Adapt the logging width to the terminal width, including sensible limits.

Posted by GitBox <gi...@apache.org>.
gnodet commented on a change in pull request #472:
URL: https://github.com/apache/maven/pull/472#discussion_r633265813



##########
File path: maven-embedder/src/main/java/org/apache/maven/cli/event/ExecutionEventLogger.java
##########
@@ -53,9 +54,15 @@
 {
     private final Logger logger;
 
-    private static final int LINE_LENGTH = 72;
+    private static final int MAX_LOG_PREFIX_SIZE = 8; // "[ERROR] "
+    private static final int PROJECT_STATUS_SUFFIX_SIZE = 20; // "SUCCESS [  0.000 s]"
+    private static final int MIN_TERMINAL_WIDTH = 60;
+    private static final int MAX_TERMINAL_WIDTH = 130;
+    private static final int TERMINAL_WIDTH =
+            Math.min( MAX_TERMINAL_WIDTH, Math.max( MessageUtils.getTerminalWidth(), MIN_TERMINAL_WIDTH ) );

Review comment:
       Actually, this use case it not really supported yet.  For this to work, we'd need the information about the terminal width to be set publicly on the MavenSession.  The `MavenCLI` would set it based on a call to `MessageUtils.getTerminalWidth()`.




-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [maven] gnodet commented on pull request #472: [MNG-6915] Adapt the logging width to the terminal width, including sensible limits.

Posted by GitBox <gi...@apache.org>.
gnodet commented on pull request #472:
URL: https://github.com/apache/maven/pull/472#issuecomment-844052080


   I propose a slightly modified version at https://github.com/gnodet/maven/commit/10cf52c69c175863b0382098a5bf840c4dd74862
   The reason is that the `ExecutionEventLogger` is not a singleton as I thought, so we can simply compute the values depending on the terminal width in fields.  Way easier and will work great with `mvnd` (where the terminal width is actually passed from the client to the daemon). 


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [maven] gnodet commented on a change in pull request #472: [MNG-6915] Adapt the logging width to the terminal width, including sensible limits.

Posted by GitBox <gi...@apache.org>.
gnodet commented on a change in pull request #472:
URL: https://github.com/apache/maven/pull/472#discussion_r633265813



##########
File path: maven-embedder/src/main/java/org/apache/maven/cli/event/ExecutionEventLogger.java
##########
@@ -53,9 +54,15 @@
 {
     private final Logger logger;
 
-    private static final int LINE_LENGTH = 72;
+    private static final int MAX_LOG_PREFIX_SIZE = 8; // "[ERROR] "
+    private static final int PROJECT_STATUS_SUFFIX_SIZE = 20; // "SUCCESS [  0.000 s]"
+    private static final int MIN_TERMINAL_WIDTH = 60;
+    private static final int MAX_TERMINAL_WIDTH = 130;
+    private static final int TERMINAL_WIDTH =
+            Math.min( MAX_TERMINAL_WIDTH, Math.max( MessageUtils.getTerminalWidth(), MIN_TERMINAL_WIDTH ) );

Review comment:
       Actually, this use case it not really supported yet.  For this to work, we'd need the information about the terminal width to be set publicly on the MavenSession.  The `MavenCLI` would set it based on a call to `MessageUtils.getTerminalWidth()`.




-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [maven] gnodet edited a comment on pull request #472: [MNG-6915] Adapt the logging width to the terminal width, including sensible limits.

Posted by GitBox <gi...@apache.org>.
gnodet edited a comment on pull request #472:
URL: https://github.com/apache/maven/pull/472#issuecomment-844052080


   @MartinKanters I propose a slightly modified version at https://github.com/gnodet/maven/commit/10cf52c69c175863b0382098a5bf840c4dd74862
   The reason is that the `ExecutionEventLogger` is not a singleton as I thought, so we can simply compute the values depending on the terminal width in fields.  Way easier and will work great with `mvnd` (where the terminal width is actually passed from the client to the daemon). 


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [maven] MartinKanters commented on pull request #472: [MNG-6915] Adapt the logging width to the terminal width, including sensible limits.

Posted by GitBox <gi...@apache.org>.
MartinKanters commented on pull request #472:
URL: https://github.com/apache/maven/pull/472#issuecomment-841226247


   @gnodet @rfscholte Could you please take a look at this?


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [maven] gnodet commented on a change in pull request #472: [MNG-6915] Adapt the logging width to the terminal width, including sensible limits.

Posted by GitBox <gi...@apache.org>.
gnodet commented on a change in pull request #472:
URL: https://github.com/apache/maven/pull/472#discussion_r634742825



##########
File path: maven-embedder/src/main/java/org/apache/maven/cli/event/ExecutionEventLogger.java
##########
@@ -53,9 +54,15 @@
 {
     private final Logger logger;
 
-    private static final int LINE_LENGTH = 72;
+    private static final int MAX_LOG_PREFIX_SIZE = 8; // "[ERROR] "
+    private static final int PROJECT_STATUS_SUFFIX_SIZE = 20; // "SUCCESS [  0.000 s]"
+    private static final int MIN_TERMINAL_WIDTH = 60;
+    private static final int MAX_TERMINAL_WIDTH = 130;
+    private static final int TERMINAL_WIDTH =
+            Math.min( MAX_TERMINAL_WIDTH, Math.max( MessageUtils.getTerminalWidth(), MIN_TERMINAL_WIDTH ) );

Review comment:
       Then reusing the cache from the `MavenSession` simply to store the value might be the easiest way.  It does not have to leak outside the `ExecutionEventLogger`.




-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [maven] MartinKanters commented on a change in pull request #472: [MNG-6915] Adapt the logging width to the terminal width, including sensible limits.

Posted by GitBox <gi...@apache.org>.
MartinKanters commented on a change in pull request #472:
URL: https://github.com/apache/maven/pull/472#discussion_r633294353



##########
File path: maven-embedder/src/main/java/org/apache/maven/cli/event/ExecutionEventLogger.java
##########
@@ -53,9 +54,15 @@
 {
     private final Logger logger;
 
-    private static final int LINE_LENGTH = 72;
+    private static final int MAX_LOG_PREFIX_SIZE = 8; // "[ERROR] "
+    private static final int PROJECT_STATUS_SUFFIX_SIZE = 20; // "SUCCESS [  0.000 s]"
+    private static final int MIN_TERMINAL_WIDTH = 60;
+    private static final int MAX_TERMINAL_WIDTH = 130;
+    private static final int TERMINAL_WIDTH =
+            Math.min( MAX_TERMINAL_WIDTH, Math.max( MessageUtils.getTerminalWidth(), MIN_TERMINAL_WIDTH ) );

Review comment:
       Ah, I didn't realize that tools would create new `MavenSession`'s while reusing the classloaders, thanks for explaining!
   
   I assume the terminal width would have to be publicly available on the session, in order for `mvnd` to create new sessions with the same width. For Eclipse, I assume that they would have to set the width to their internal terminal/logger window.
   I'm just not sure whether `MavenSession` is the right class for this, because afaik it's currently cli-unaware. Perhaps there is a better place, but I don't know that by heart.




-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [maven] gnodet commented on a change in pull request #472: [MNG-6915] Adapt the logging width to the terminal width, including sensible limits.

Posted by GitBox <gi...@apache.org>.
gnodet commented on a change in pull request #472:
URL: https://github.com/apache/maven/pull/472#discussion_r632533500



##########
File path: maven-embedder/src/main/java/org/apache/maven/cli/event/ExecutionEventLogger.java
##########
@@ -53,9 +54,15 @@
 {
     private final Logger logger;
 
-    private static final int LINE_LENGTH = 72;
+    private static final int MAX_LOG_PREFIX_SIZE = 8; // "[ERROR] "
+    private static final int PROJECT_STATUS_SUFFIX_SIZE = 20; // "SUCCESS [  0.000 s]"
+    private static final int MIN_TERMINAL_WIDTH = 60;
+    private static final int MAX_TERMINAL_WIDTH = 130;
+    private static final int TERMINAL_WIDTH =
+            Math.min( MAX_TERMINAL_WIDTH, Math.max( MessageUtils.getTerminalWidth(), MIN_TERMINAL_WIDTH ) );

Review comment:
       I don't think this is a good idea to use a constant for the terminal width.  In the case where the maven class loader is reused, the terminal could change its width.  What about computing this variable and caching it for the `MavenSession` ?




-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [maven] gnodet commented on a change in pull request #472: [MNG-6915] Adapt the logging width to the terminal width, including sensible limits.

Posted by GitBox <gi...@apache.org>.
gnodet commented on a change in pull request #472:
URL: https://github.com/apache/maven/pull/472#discussion_r632627280



##########
File path: maven-embedder/src/main/java/org/apache/maven/cli/event/ExecutionEventLogger.java
##########
@@ -53,9 +54,15 @@
 {
     private final Logger logger;
 
-    private static final int LINE_LENGTH = 72;
+    private static final int MAX_LOG_PREFIX_SIZE = 8; // "[ERROR] "
+    private static final int PROJECT_STATUS_SUFFIX_SIZE = 20; // "SUCCESS [  0.000 s]"
+    private static final int MIN_TERMINAL_WIDTH = 60;
+    private static final int MAX_TERMINAL_WIDTH = 130;
+    private static final int TERMINAL_WIDTH =
+            Math.min( MAX_TERMINAL_WIDTH, Math.max( MessageUtils.getTerminalWidth(), MIN_TERMINAL_WIDTH ) );

Review comment:
       > If we understand correctly, this would make sure that if a terminal gets resized _during_ a Maven build, the output width would be recomputed accordingly?
   > If that is the case, we don't understand how storing this value in a `MavenSession` instance rather than a constant would solve this. As far as we know, the `MavenSession` lifetime roughly matches the lifetime of the JVM process, and as such, of the classloader.
   
   I was thinking about embedded maven in Eclipse and Mvnd, where the maven class loader and classes are reused, but a new session is created for a given build.  I don't think adjusting to the terminal size inside a given build is a good idea.




-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [maven] mthmulders commented on a change in pull request #472: [MNG-6915] Adapt the logging width to the terminal width, including sensible limits.

Posted by GitBox <gi...@apache.org>.
mthmulders commented on a change in pull request #472:
URL: https://github.com/apache/maven/pull/472#discussion_r632551481



##########
File path: maven-embedder/src/main/java/org/apache/maven/cli/event/ExecutionEventLogger.java
##########
@@ -53,9 +54,15 @@
 {
     private final Logger logger;
 
-    private static final int LINE_LENGTH = 72;
+    private static final int MAX_LOG_PREFIX_SIZE = 8; // "[ERROR] "
+    private static final int PROJECT_STATUS_SUFFIX_SIZE = 20; // "SUCCESS [  0.000 s]"
+    private static final int MIN_TERMINAL_WIDTH = 60;
+    private static final int MAX_TERMINAL_WIDTH = 130;
+    private static final int TERMINAL_WIDTH =
+            Math.min( MAX_TERMINAL_WIDTH, Math.max( MessageUtils.getTerminalWidth(), MIN_TERMINAL_WIDTH ) );

Review comment:
       If we understand correctly, this would make sure that if a terminal gets resized _during_ a Maven build, the output width would be recomputed accordingly?
   If that is the case, we don't understand how storing this value in a `MavenSession` instance rather than a constant would solve this. As far as we know, the `MavenSession` lifetime roughly matches the lifetime of the JVM process, and as such, of the classloader.




-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [maven] MartinKanters commented on a change in pull request #472: [MNG-6915] Adapt the logging width to the terminal width, including sensible limits.

Posted by GitBox <gi...@apache.org>.
MartinKanters commented on a change in pull request #472:
URL: https://github.com/apache/maven/pull/472#discussion_r633294353



##########
File path: maven-embedder/src/main/java/org/apache/maven/cli/event/ExecutionEventLogger.java
##########
@@ -53,9 +54,15 @@
 {
     private final Logger logger;
 
-    private static final int LINE_LENGTH = 72;
+    private static final int MAX_LOG_PREFIX_SIZE = 8; // "[ERROR] "
+    private static final int PROJECT_STATUS_SUFFIX_SIZE = 20; // "SUCCESS [  0.000 s]"
+    private static final int MIN_TERMINAL_WIDTH = 60;
+    private static final int MAX_TERMINAL_WIDTH = 130;
+    private static final int TERMINAL_WIDTH =
+            Math.min( MAX_TERMINAL_WIDTH, Math.max( MessageUtils.getTerminalWidth(), MIN_TERMINAL_WIDTH ) );

Review comment:
       Ah, I didn't realize that tools would create new `MavenSession`'s while reusing the classloaders, thanks for explaining!
   
   I assume the terminal width would have to be publicly available on the session, in order for `mvnd` to create new sessions with the same width. For Eclipse, I assume that they would have to set the width to their internal terminal/logger window.
   I'm just not sure whether `MavenSession` is the right class for this, because afaik it's currently cli-unaware. Perhaps there is a better place, but I don't know that by heart.




-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org