469982 - Produce warning for dynamic modules with ini-templates seen during --add-to-start

+ Chaning Module.isVirtual() to Module.isDynamic()
+ Adding javadoc for Module.isDynamic()
+ Adding warning for builders on start.ini and start.d/*.ini with
  dynamic modules with [ini-template] sections

3 files changed, 35 insertions, 24 deletions

diff --git a/jetty-start/src/main/java/org/eclipse/jetty/start/Module.java b/jetty-start/src/main/java/org/eclipse/jetty/start/Module.java
index 90a6ed8d16..6d5ca285e8 100644
--- a/jetty-start/src/main/java/org/eclipse/jetty/start/Module.java
+++ b/jetty-start/src/main/java/org/eclipse/jetty/start/Module.java
@@ -241,7 +241,12 @@ public class Module extends Node<Module>
         setName(this.fileRef);
     }
 
-    public boolean isVirtual()
+    /**
+     * Indicates a module that is dynamic in nature
+     * 
+     * @return a module where the declared metadata name does not match the filename reference (aka a dynamic module)
+     */
+    public boolean isDynamic()
     {
         return !getName().equals(fileRef);
     }
@@ -289,6 +294,8 @@ public class Module extends Node<Module>
                     // blank lines and comments are valid for ini-template section
                     if ((line.length() == 0) || line.startsWith("#"))
                     {
+                        // Remember ini comments and whitespace (empty lines)
+                        // for the [ini-template] section
                         if ("INI-TEMPLATE".equals(sectionType))
                         {
                             iniTemplate.add(line);
@@ -308,7 +315,7 @@ public class Module extends Node<Module>
                             case "FILES":
                                 files.add(line);
                                 break;
-                            case "DEFAULTS": // old name from 9.2.x
+                            case "DEFAULTS": // old name introduced in 9.2.x
                             case "INI": // new name for 9.3+
                                 defaultConfig.add(line);
                                 hasDefaultConfig = true;
@@ -372,7 +379,7 @@ public class Module extends Node<Module>
     {
         StringBuilder str = new StringBuilder();
         str.append("Module[").append(getName());
-        if (isVirtual())
+        if (isDynamic())
         {
             str.append(",file=").append(fileRef);
         }
diff --git a/jetty-start/src/main/java/org/eclipse/jetty/start/builders/StartDirBuilder.java b/jetty-start/src/main/java/org/eclipse/jetty/start/builders/StartDirBuilder.java
index 51305bee8c..1de331d717 100644
--- a/jetty-start/src/main/java/org/eclipse/jetty/start/builders/StartDirBuilder.java
+++ b/jetty-start/src/main/java/org/eclipse/jetty/start/builders/StartDirBuilder.java
@@ -42,21 +42,24 @@ public class StartDirBuilder implements BaseBuilder.Config
 {
     private final BaseHome baseHome;
     private final Path startDir;
-    
+
     public StartDirBuilder(BaseBuilder baseBuilder) throws IOException
     {
         this.baseHome = baseBuilder.getBaseHome();
         this.startDir = baseHome.getBasePath("start.d");
         FS.ensureDirectoryExists(startDir);
     }
-    
+
     @Override
     public boolean addModule(Module module) throws IOException
     {
-        if (module.isVirtual())
+        if (module.isDynamic())
         {
-            // skip, no need to reference
-            StartLog.info("%-15s skipping (virtual module)",module.getName());
+            if (module.hasIniTemplate())
+            {
+                // warn
+                StartLog.warn("%-15s not adding [ini-template] from dynamic module",module.getName());
+            }
             return false;
         }
 
@@ -67,12 +70,12 @@ public class StartDirBuilder implements BaseBuilder.Config
             mode = "(transitively) ";
         }
 
-        // Create start.d/{name}.ini
-        Path ini = startDir.resolve(module.getName() + ".ini");
-        StartLog.info("%-15s initialised %sin %s",module.getName(),mode,baseHome.toShortForm(ini));
-
         if (module.hasIniTemplate() || !isTransitive)
         {
+            // Create start.d/{name}.ini
+            Path ini = startDir.resolve(module.getName() + ".ini");
+            StartLog.info("%-15s initialised %sin %s",module.getName(),mode,baseHome.toShortForm(ini));
+
             try (BufferedWriter writer = Files.newBufferedWriter(ini,StandardCharsets.UTF_8,StandardOpenOption.CREATE,StandardOpenOption.TRUNCATE_EXISTING))
             {
                 writeModuleSection(writer,module);
@@ -82,15 +85,15 @@ public class StartDirBuilder implements BaseBuilder.Config
 
         return false;
     }
-    
+
     protected void writeModuleSection(BufferedWriter writer, Module module)
     {
         PrintWriter out = new PrintWriter(writer);
 
         out.println("# --------------------------------------- ");
         out.println("# Module: " + module.getName());
-
         out.println("--module=" + module.getName());
+        out.println();
 
         for (String line : module.getIniTemplate())
         {
diff --git a/jetty-start/src/main/java/org/eclipse/jetty/start/builders/StartIniBuilder.java b/jetty-start/src/main/java/org/eclipse/jetty/start/builders/StartIniBuilder.java
index af16921979..035ec20118 100644
--- a/jetty-start/src/main/java/org/eclipse/jetty/start/builders/StartIniBuilder.java
+++ b/jetty-start/src/main/java/org/eclipse/jetty/start/builders/StartIniBuilder.java
@@ -40,8 +40,7 @@ import org.eclipse.jetty.start.graph.OnlyTransitivePredicate;
 /**
  * Management of the <code>${jetty.base}/start.ini</code> based configuration.
  * <p>
- * Implementation of the <code>--add-to-start=[name]</code> command line
- * behavior
+ * Implementation of the <code>--add-to-start=[name]</code> command line behavior
  */
 public class StartIniBuilder implements BaseBuilder.Config
 {
@@ -96,14 +95,17 @@ public class StartIniBuilder implements BaseBuilder.Config
             // skip, already present
             return false;
         }
-        
-        if (module.isVirtual())
+
+        if (module.isDynamic())
         {
-            // skip, no need to reference
-            StartLog.info("%-15s skipping (virtual module)",module.getName());
+            if (module.hasIniTemplate())
+            {
+                // warn
+                StartLog.warn("%-15s not adding [ini-template] from dynamic module",module.getName());
+            }
             return false;
         }
-        
+
         String mode = "";
         boolean isTransitive = module.matches(OnlyTransitivePredicate.INSTANCE);
         if (isTransitive)
@@ -111,10 +113,10 @@ public class StartIniBuilder implements BaseBuilder.Config
             mode = "(transitively) ";
         }
 
-        StartLog.info("%-15s initialised %sin %s",module.getName(),mode,baseHome.toShortForm(startIni));
-        
         if (module.hasIniTemplate() || !isTransitive)
         {
+            StartLog.info("%-15s initialised %sin %s",module.getName(),mode,baseHome.toShortForm(startIni));
+
             // Append to start.ini
             try (BufferedWriter writer = Files.newBufferedWriter(startIni,StandardCharsets.UTF_8,StandardOpenOption.APPEND,StandardOpenOption.CREATE))
             {
@@ -137,7 +139,6 @@ public class StartIniBuilder implements BaseBuilder.Config
 
         for (String line : module.getIniTemplate())
         {
-            // TODO: validate property keys
             out.println(line);
         }