Leverage Maven's -Dstyle.color to avoid coloring instead of stripping the ASCII codes in the client

README.adoc

@@ -173,6 +173,10 @@ $ ls -lh target/mvnd
 
 === Known issues and limitations
 
-* Parallel build, hence `mvnd`, don't play well with `-rf` maven option at the moment. If you use the `-rf` maven option with `mvnd` you have no guarantee that all the modules will have been built (There are some improvements planned on this in the next `mvnd` releases).
+* `mvnd` generally does not play well with `-rf` Maven option at the moment.
+  If your source tree allows for building of some modules in parallel
+  and if the failure happens on some of the parallel execution branches,
+  there is no guarantee that `mvnd` builds all modules skipped due to the previous failure.
+  There are some improvements planned on this in the next `mvnd` releases.
 
-We're happy to improve `mvnd`, so feedback is most welcomed!
+We're happy to improve `mvnd`, so https://github.com/mvndaemon/mvnd/issues[feedback] is most welcomed!

client/src/main/java/org/mvndaemon/mvnd/client/DefaultClient.java

@@ -33,13 +33,15 @@
 import java.util.List;
 import java.util.concurrent.atomic.AtomicReference;
 import org.fusesource.jansi.Ansi;
+import org.fusesource.jansi.internal.CLibrary;
 import org.jline.utils.AttributedString;
 import org.jline.utils.AttributedStyle;
 import org.mvndaemon.mvnd.common.BuildProperties;
 import org.mvndaemon.mvnd.common.DaemonException;
 import org.mvndaemon.mvnd.common.DaemonInfo;
 import org.mvndaemon.mvnd.common.DaemonRegistry;
 import org.mvndaemon.mvnd.common.Environment;
+import org.mvndaemon.mvnd.common.Environment.Color;
 import org.mvndaemon.mvnd.common.Message;
 import org.mvndaemon.mvnd.common.Message.BuildException;
 import org.mvndaemon.mvnd.common.Message.BuildFinished;
@@ -72,21 +74,27 @@ public static void main(String[] argv) throws Exception {
             }
         }
 
-        // Color
-        String color = Environment.COLOR.removeCommandLineOption(args);
-        if (color == null) {
-            color = Environment.COLOR.getDefault();
-        }
-
         // Serial
         if (Environment.SERIAL.removeCommandLineOption(args) != null) {
             System.setProperty(Environment.SERIAL.getProperty(), Boolean.toString(true));
         }
 
         // Batch mode
-        boolean batchMode = Environment.MAVEN_BATCH_MODE.hasCommandLineOption(args)
+        final boolean batchMode = Environment.MAVEN_BATCH_MODE.hasCommandLineOption(args)
                 || Environment.COMPLETION.hasCommandLineOption(args);
 
+        // Color
+        Color styleColor = Color.of(Environment.MAVEN_COLOR.removeCommandLineOption(args)).orElse(Color.auto);
+        if (styleColor == Color.auto) {
+            /* Translate from auto to either always or never */
+            /* stdout is not a terminal e.g. when stdout is redirected to a file */
+            final boolean stdoutIsTerminal = CLibrary.isatty(1) != 0;
+            styleColor = (batchMode || logFile != null || !stdoutIsTerminal) ? Color.never : Color.always;
+        }
+        /* We cannot use Environment.addCommandLineOption() because that one would pass --color to the daemon
+         * and --color is not supported there yet. */
+        args.add("-D" + Environment.MAVEN_COLOR.getProperty() + "=" + styleColor.name());
+
         // System properties
         setSystemPropertiesFromCommandLine(args);
 
@@ -99,7 +107,7 @@ public static void main(String[] argv) throws Exception {
 
         int exitCode = 0;
         boolean noBuffering = batchMode || parameters.noBuffering();
-        try (TerminalOutput output = new TerminalOutput(noBuffering, parameters.rollingWindowSize(), logFile, color)) {
+        try (TerminalOutput output = new TerminalOutput(noBuffering, parameters.rollingWindowSize(), logFile)) {
             try {
                 final ExecutionResult result = new DefaultClient(parameters).execute(output, args);
                 exitCode = result.getExitCode();
@@ -149,7 +157,6 @@ public ExecutionResult execute(ClientOutput output, List<String> argv) {
         boolean version = Environment.MAVEN_VERSION.hasCommandLineOption(args);
         boolean showVersion = Environment.MAVEN_SHOW_VERSION.hasCommandLineOption(args);
         boolean debug = Environment.MAVEN_DEBUG.hasCommandLineOption(args);
-        boolean batchMode = Environment.MAVEN_BATCH_MODE.hasCommandLineOption(args);
 
         // Print version if needed
         if (version || showVersion || debug) {
@@ -162,7 +169,8 @@ public ExecutionResult execute(ClientOutput output, List<String> argv) {
                     + "-" + buildProperties.getOsArch()
                     + " (" + buildProperties.getRevision() + ")";
 
-            final String v = batchMode
+            boolean isColored = !"never".equals(Environment.MAVEN_COLOR.getCommandLineOption(args));
+            final String v = isColored
                     ? mvndVersionString
                     : Ansi.ansi().bold().a(mvndVersionString).reset().toString();
             output.accept(Message.log(v));

common/src/main/java/org/mvndaemon/mvnd/common/Environment.java

@@ -58,8 +58,6 @@ public enum Environment {
     STOP(null, null, null, OptionType.VOID, Flags.OPTIONAL, "mvnd:--stop"),
     /** Use one thread, no log buffering and the default project builder to behave like a standard maven */
     SERIAL("mvnd.serial", null, Boolean.FALSE, OptionType.VOID, Flags.OPTIONAL, "mvnd:-1", "mvnd:--serial"),
-    /** Manage color output, can be either auto (the default), always or never */
-    COLOR(null, null, "auto", OptionType.STRING, Flags.OPTIONAL, "mvnd:--color"),
 
     //
     // Log properties
@@ -108,6 +106,8 @@ public enum Environment {
     MAVEN_SHOW_VERSION(null, null, null, OptionType.BOOLEAN, Flags.INTERNAL, "mvn:-V", "mvn:--show-version"),
     /** Define */
     MAVEN_DEFINE(null, null, null, OptionType.STRING, Flags.INTERNAL, "mvn:-D", "mvn:--define"),
+    /** Whether the output should be styled using ANSI color codes; possible values: auto, always, never */
+    MAVEN_COLOR("style.color", null, "auto", OptionType.STRING, Flags.OPTIONAL),
 
     //
     // mvnd properties
@@ -379,13 +379,23 @@ public boolean hasCommandLineOption(Collection<String> args) {
         return args.stream().anyMatch(arg -> Stream.of(prefixes).anyMatch(arg::startsWith));
     }
 
+    public String getCommandLineOption(Collection<String> args) {
+        return getCommandLineOption(args, false);
+    }
+
     public String removeCommandLineOption(Collection<String> args) {
+        return getCommandLineOption(args, true);
+    }
+
+    String getCommandLineOption(Collection<String> args, boolean remove) {
         final String[] prefixes = getPrefixes();
         String value = null;
         for (Iterator<String> it = args.iterator(); it.hasNext();) {
             String arg = it.next();
             if (Stream.of(prefixes).anyMatch(arg::startsWith)) {
-                it.remove();
+                if (remove) {
+                    it.remove();
+                }
                 if (type == OptionType.VOID) {
                     value = "";
                 } else {
@@ -395,7 +405,9 @@ public String removeCommandLineOption(Collection<String> args) {
                     if (value.isEmpty()) {
                         if (it.hasNext()) {
                             value = it.next();
-                            it.remove();
+                            if (remove) {
+                                it.remove();
+                            }
                         }
                     } else {
                         if (value.charAt(0) == '=') {
@@ -461,6 +473,17 @@ private OptionOrigin() {
         }
     }
 
+    /**
+     * The values of {@link Environment#MAVEN_COLOR} option.
+     */
+    public enum Color {
+        always, never, auto;
+
+        public static Optional<Color> of(String color) {
+            return color == null ? Optional.empty() : Optional.of(Color.valueOf(color));
+        }
+    }
+
     public static class DocumentedEnumEntry<E> {
 
         private final E entry;

common/src/main/java/org/mvndaemon/mvnd/common/logging/TerminalOutput.java

@@ -35,12 +35,10 @@
 import java.util.function.Consumer;
 import java.util.stream.Collector;
 import java.util.stream.Collectors;
-import org.fusesource.jansi.internal.CLibrary;
 import org.jline.terminal.Size;
 import org.jline.terminal.Terminal;
 import org.jline.terminal.TerminalBuilder;
 import org.jline.terminal.impl.AbstractPosixTerminal;
-import org.jline.utils.AnsiWriter;
 import org.jline.utils.AttributedString;
 import org.jline.utils.AttributedStringBuilder;
 import org.jline.utils.AttributedStyle;
@@ -88,10 +86,6 @@ public class TerminalOutput implements ClientOutput {
      */
     public static final int KEY_CTRL_M = 'M' & 0x1f;
 
-    public static final String COLOR_AUTO = "auto";
-    public static final String COLOR_ALWAYS = "always";
-    public static final String COLOR_NEVER = "never";
-
     private static final AttributedStyle GREEN_FOREGROUND = new AttributedStyle().foreground(AttributedStyle.GREEN);
     private static final AttributedStyle CYAN_FOREGROUND = new AttributedStyle().foreground(AttributedStyle.CYAN);
 
@@ -149,7 +143,7 @@ public Project(String id) {
         }
     }
 
-    public TerminalOutput(boolean noBuffering, int rollingWindowSize, Path logFile, String color) throws IOException {
+    public TerminalOutput(boolean noBuffering, int rollingWindowSize, Path logFile) throws IOException {
         this.start = System.currentTimeMillis();
         this.terminal = TerminalBuilder.terminal();
         this.dumb = terminal.getType().startsWith("dumb");
@@ -165,7 +159,7 @@ public TerminalOutput(boolean noBuffering, int rollingWindowSize, Path logFile,
         this.previousIntHandler = terminal.handle(Terminal.Signal.INT,
                 sig -> daemonDispatch.accept(Message.BareMessage.CANCEL_BUILD_SINGLETON));
         this.display = new Display(terminal, false);
-        this.log = logFile == null ? new MessageCollector(color) : new FileLog(logFile, color);
+        this.log = logFile == null ? new MessageCollector() : new FileLog(logFile);
         if (!dumb) {
             final Thread r = new Thread(this::readInputLoop);
             r.start();
@@ -771,15 +765,10 @@ static class FileLog implements ClientLog {
 
         private final Writer out;
         private final Path logFile;
-        private boolean closed;
 
-        public FileLog(Path logFile, String color) throws IOException {
+        public FileLog(Path logFile) throws IOException {
             super();
-            if (COLOR_ALWAYS.equalsIgnoreCase(color)) {
-                this.out = Files.newBufferedWriter(logFile, StandardCharsets.UTF_8);
-            } else {
-                this.out = new AnsiWriter(Files.newBufferedWriter(logFile, StandardCharsets.UTF_8));
-            }
+            this.out = Files.newBufferedWriter(logFile, StandardCharsets.UTF_8);
             this.logFile = logFile;
         }
 
@@ -800,10 +789,7 @@ public void flush() throws IOException {
 
         @Override
         public void close() throws IOException {
-            if (!closed) {
-                out.close();
-                closed = true;
-            }
+            out.close();
         }
 
     }
@@ -815,23 +801,6 @@ public void close() throws IOException {
     class MessageCollector implements ClientLog {
 
         private final List<String> messages = new ArrayList<>();
-        private final boolean strip;
-
-        public MessageCollector(String color) {
-            if (COLOR_NEVER.equalsIgnoreCase(color)) {
-                this.strip = true;
-            } else if (COLOR_ALWAYS.equalsIgnoreCase(color)) {
-                this.strip = false;
-            } else {
-                boolean strip;
-                try {
-                    strip = CLibrary.isatty(1) == 0;
-                } catch (Throwable t) {
-                    strip = true;
-                }
-                this.strip = strip;
-            }
-        }
 
         @Override
         public void accept(String message) {
@@ -841,7 +810,7 @@ public void accept(String message) {
         @Override
         public void flush() {
             clearDisplay();
-            messages.forEach(s -> terminal.writer().println(strip ? AttributedString.stripAnsi(s) : s));
+            messages.forEach(terminal.writer()::println);
             messages.clear();
             terminal.flush();
         }

daemon/src/test/java/org/mvndaemon/mvnd/cache/impl/CacheFactoryTest.java

@@ -19,6 +19,7 @@
 import java.nio.charset.StandardCharsets;
 import java.nio.file.Files;
 import java.nio.file.Path;
+import java.nio.file.attribute.BasicFileAttributes;
 import java.util.Arrays;
 import java.util.List;
 import java.util.stream.Stream;
@@ -64,6 +65,12 @@ public void assertPutGet(Path tempDir, final Cache<String, CacheRecord> cache, i
         Assertions.assertEquals(record2, cache.get(k2));
         Assertions.assertFalse(record2.invalidated);
 
+        /* Make sure the new content is written a couple of ms later than the original lastModifiedTime */
+        final long deadline = Files.readAttributes(file1, BasicFileAttributes.class).lastModifiedTime().toMillis() + 10;
+        while (System.currentTimeMillis() < deadline) {
+            Thread.sleep(1);
+        }
+
         Files.write(file1, "content1.1".getBytes(StandardCharsets.UTF_8));
         if (asyncOpDelayMs > 0) {
             Thread.sleep(asyncOpDelayMs);

daemon/src/test/java/org/mvndaemon/mvnd/daemon/ServerTest.java

@@ -18,7 +18,6 @@
 import java.util.Arrays;
 import java.util.concurrent.BlockingQueue;
 import java.util.concurrent.PriorityBlockingQueue;
-
 import org.junit.jupiter.api.Test;
 import org.mvndaemon.mvnd.common.Message;
 
@@ -32,8 +31,7 @@ void testMessageOrdering() {
         messages.addAll(Arrays.asList(
                 Message.projectStopped("projectId"),
                 Message.projectStarted("projectId"),
-                Message.log("projectId", "message"))
-        );
+                Message.log("projectId", "message")));
 
         assertEquals(Message.PROJECT_STARTED, messages.remove().getType());
         assertEquals(Message.PROJECT_LOG_MESSAGE, messages.remove().getType());

dist/src/main/distro/bin/mvnd-bash-completion.bash

@@ -146,8 +146,8 @@ _mvnd()
     _get_comp_words_by_ref -n : cur prev
 
     local mvnd_opts="-1"
-    local mvnd_long_opts="--color|--completion|--purge|--serial|--status|--stop"
-    local mvnd_properties="-Djava.home|-Dmaven.multiModuleProjectDirectory|-Dmaven.repo.local|-Dmaven.settings|-Dmvnd.builder|-Dmvnd.daemonStorage|-Dmvnd.debug|-Dmvnd.duplicateDaemonGracePeriod|-Dmvnd.enableAssertions|-Dmvnd.expirationCheckDelay|-Dmvnd.home|-Dmvnd.idleTimeout|-Dmvnd.jvmArgs|-Dmvnd.keepAlive|-Dmvnd.logPurgePeriod|-Dmvnd.logback|-Dmvnd.maxHeapSize|-Dmvnd.maxLostKeepAlive|-Dmvnd.minHeapSize|-Dmvnd.minThreads|-Dmvnd.noBuffering|-Dmvnd.noDaemon|-Dmvnd.propertiesPath|-Dmvnd.registry|-Dmvnd.rollingWindowSize|-Dmvnd.serial|-Dmvnd.threads|-Duser.dir|-Duser.home"
+    local mvnd_long_opts="--completion|--purge|--serial|--status|--stop"
+    local mvnd_properties="-Djava.home|-Dmaven.multiModuleProjectDirectory|-Dmaven.repo.local|-Dmaven.settings|-Dmvnd.builder|-Dmvnd.daemonStorage|-Dmvnd.debug|-Dmvnd.duplicateDaemonGracePeriod|-Dmvnd.enableAssertions|-Dmvnd.expirationCheckDelay|-Dmvnd.home|-Dmvnd.idleTimeout|-Dmvnd.jvmArgs|-Dmvnd.keepAlive|-Dmvnd.logPurgePeriod|-Dmvnd.logback|-Dmvnd.maxHeapSize|-Dmvnd.maxLostKeepAlive|-Dmvnd.minHeapSize|-Dmvnd.minThreads|-Dmvnd.noBuffering|-Dmvnd.noDaemon|-Dmvnd.propertiesPath|-Dmvnd.registry|-Dmvnd.rollingWindowSize|-Dmvnd.serial|-Dmvnd.threads|-Dstyle.color|-Duser.dir|-Duser.home"
     local opts="-am|-amd|-B|-C|-c|-cpu|-D|-e|-emp|-ep|-f|-fae|-ff|-fn|-gs|-h|-l|-N|-npr|-npu|-nsu|-o|-P|-pl|-q|-rf|-s|-T|-t|-U|-up|-V|-v|-X|${mvnd_opts}"
     local long_opts="--also-make|--also-make-dependents|--batch-mode|--strict-checksums|--lax-checksums|--check-plugin-updates|--define|--errors|--encrypt-master-password|--encrypt-password|--file|--fail-at-end|--fail-fast|--fail-never|--global-settings|--help|--log-file|--non-recursive|--no-plugin-registry|--no-plugin-updates|--no-snapshot-updates|--offline|--activate-profiles|--projects|--quiet|--resume-from|--settings|--threads|--toolchains|--update-snapshots|--update-plugins|--show-version|--version|--debug|${mvnd_long_opts}"
 

integration-tests/pom.xml

@@ -38,6 +38,7 @@
             org/apache/maven/surefire/surefire-junit-platform/${surefire.version}
             org/junit/platform/junit-platform-launcher/${junit-platform-launcher.version}
             org/junit/platform/junit-platform-engine/${junit-platform-launcher.version}
+            org/junit/platform/junit-platform-commons/${junit-platform-launcher.version}
             org/junit/jupiter/junit-jupiter/${junit.jupiter.version}
             org/junit/jupiter/junit-jupiter-api/${junit.jupiter.version}
         </preinstall.artifacts>

integration-tests/src/test/java/org/mvndaemon/mvnd/it/EnvironmentTest.java

@@ -53,6 +53,9 @@ void cleanInstall() throws IOException, InterruptedException {
             cl.execute(output, "org.apache.maven.plugins:maven-help-plugin:3.2.0:evaluate",
                     "-Dexpression=user.dir", "-e").assertSuccess();
             assertDaemonRegistrySize(1);
+            /* Wait, till the existing instance becomes idle so that the next iteration does not start a new instance */
+            registry.awaitIdle(registry.getAll().get(0).getId());
+
             String pathStr = dir.toAbsolutePath().toString();
             assertTrue(output.getMessages().stream()
                     .anyMatch(m -> m.toString().contains(pathStr)), "Output should contain " + pathStr);