Forrest
  1. Forrest
  2. FOR-752

Forrestbot "build" workstage creates spurious "build/webapp/WEB-INF/logs" directory

    Details

    • Type: Bug Bug
    • Status: Closed
    • Priority: Minor Minor
    • Resolution: Fixed
    • Affects Version/s: 0.7, 0.8, 0.9
    • Fix Version/s: 0.9
    • Component/s: Tool: Forrestbot
    • Labels:
      None
    • Urgency:
      Normal

      Description

      When running Forrestbot with the default value for property "build.work-dir" (namely "work/${ant.project.name}"), "project.build-dir" is set to the same value and consequently, "project.webapp" is set to "work/${ant.project.name}/webapp". However, one of the two "logs" directories continues to be created at "build/webapp/WEB-INF/logs", suggesting that somewhere a hardcoded value is used instead of ${project.webapp}. The mkdir command for this "logs" directory is not in any of the Ant build files in the Forrest distribution; it must be in one of the Java classes, probably in a class related to logging. I forced an I/O failure by turning off all permissions on build/webapp and running "forrest -f build.xml build". This yielded a stacktrace that originated 7 calls before:

      org.apache.avalon.excalibur.logger.factory.FileTargetFactory.createTarget(FileTargetFactory.java:160)

      The remaining 7 invokations were not displayed (just "... 7 more").

      The upshot of this bug is that running "forrest -f build.xml clean" misses the "logs" directory in the unexpected location. My workaround right now is to use a custom "clean-all" target that depends on "clean" and that deletes the spurious "build/webapp" directory.

        Activity

        Hide
        Mathieu Champlon added a comment -
        I just ran into the same issue, not a very important bug but rather annoying.

        The problem is in ForrestLogTargetFactory#contextualize where the string "/build/webapp" is hardcoded and appended to projectHome in order to set the context-root.
        I wonder why the context-root is not simply set to project.webapp ?

        Replacing the line :
                        newContext.put("context-root",projectHome + "/build/webapp");
        With :
                        String webAppDir = ForrestConfUtils.getSystemProperty("project.webapp");
                        newContext.put("context-root",webAppDir);
        (and changing ForrestConfUtils#getSystemProperty visibility to public)
        seems to fix the issue, however I'm not aware enough about forrest internals to clearly see all the possible consequences.

        Along the same idea, isn't the following line taken from ForrestConfUtils#getProjectHome involving another hardcoded path ?
                    projectHome = defaultHome + SystemUtils.FILE_SEPARATOR + "/project";
        Show
        Mathieu Champlon added a comment - I just ran into the same issue, not a very important bug but rather annoying. The problem is in ForrestLogTargetFactory#contextualize where the string "/build/webapp" is hardcoded and appended to projectHome in order to set the context-root. I wonder why the context-root is not simply set to project.webapp ? Replacing the line :                 newContext.put("context-root",projectHome + "/build/webapp"); With :                 String webAppDir = ForrestConfUtils.getSystemProperty("project.webapp");                 newContext.put("context-root",webAppDir); (and changing ForrestConfUtils#getSystemProperty visibility to public) seems to fix the issue, however I'm not aware enough about forrest internals to clearly see all the possible consequences. Along the same idea, isn't the following line taken from ForrestConfUtils#getProjectHome involving another hardcoded path ?             projectHome = defaultHome + SystemUtils.FILE_SEPARATOR + "/project";
        Hide
        Ross Gardler added a comment -
        Uppin gthe urgency a level - not a critical bug, but we really should not have hard coded paths and Matheiu has identified what needs to be changed - not too much for a committer to do now (other than find the time ;-)
        Show
        Ross Gardler added a comment - Uppin gthe urgency a level - not a critical bug, but we really should not have hard coded paths and Matheiu has identified what needs to be changed - not too much for a committer to do now (other than find the time ;-)
        Hide
        David Crossley added a comment -
        Indicating "patch available" inline from Mathieu Champlon.
        Show
        David Crossley added a comment - Indicating "patch available" inline from Mathieu Champlon.
        Hide
        Mathieu Champlon added a comment -
        It's probably not a good solution though, as only properties coming from ant are available as system properties.
        Something better would be at least to ask ForrestConfModule in order to retrieve the needed property, it's probably possible to query via cocoon in some way but I have no idea how to do it.

        Show
        Mathieu Champlon added a comment - It's probably not a good solution though, as only properties coming from ant are available as system properties. Something better would be at least to ask ForrestConfModule in order to retrieve the needed property, it's probably possible to query via cocoon in some way but I have no idea how to do it.
        Hide
        Gavin added a comment -
        Where is the patch for this gone?
        Show
        Gavin added a comment - Where is the patch for this gone?
        Hide
        David Crossley added a comment -
        See my comment at 09/Aug/06 ... the patch is "in-line", i.e. Mathieu explained it in text.

        However see his subsequent comments too.
        Show
        David Crossley added a comment - See my comment at 09/Aug/06 ... the patch is "in-line", i.e. Mathieu explained it in text. However see his subsequent comments too.
        Hide
        David Crossley added a comment -
        Scheduling this for next version 0.10

        However if someone can manage it for upcoming 0.9 that would be better.
        Show
        David Crossley added a comment - Scheduling this for next version 0.10 However if someone can manage it for upcoming 0.9 that would be better.
        Hide
        Brian M Dube added a comment - - edited
        I included this stacktrace because I thought it showed the directory being created in a different part of the code, but I didn't read the issue description closely enough. Leaving it in since the original description is truncated.

          [1] java.io.File.mkdirs (File.java:1,145)
          [2] org.apache.log.output.io.FileTarget.openFile (FileTarget.java:103)
          [3] org.apache.log.output.io.FileTarget.<init> (FileTarget.java:55)
          [4] org.apache.avalon.excalibur.logger.factory.FileTargetFactory.createTarget (FileTargetFactory.java:164)
          [5] org.apache.avalon.excalibur.logger.factory.FileTargetFactory.createTarget (FileTargetFactory.java:145)
          [6] org.apache.avalon.excalibur.logger.DefaultLogTargetManager.configure (DefaultLogTargetManager.java:92)
          [7] org.apache.avalon.framework.container.ContainerUtil.configure (ContainerUtil.java:201)
          [8] org.apache.avalon.excalibur.logger.LogKitLoggerManager.setupTargetManager (LogKitLoggerManager.java:457)
          [9] org.apache.avalon.excalibur.logger.LogKitLoggerManager.configure (LogKitLoggerManager.java:403)
          [10] org.apache.cocoon.bean.CocoonWrapper.initialize (CocoonWrapper.java:144)
          [11] org.apache.cocoon.bean.CocoonBean.initialize (CocoonBean.java:102)
          [12] org.apache.cocoon.Main.main (Main.java:320)
        Show
        Brian M Dube added a comment - - edited I included this stacktrace because I thought it showed the directory being created in a different part of the code, but I didn't read the issue description closely enough. Leaving it in since the original description is truncated.   [1] java.io.File.mkdirs (File.java:1,145)   [2] org.apache.log.output.io.FileTarget.openFile (FileTarget.java:103)   [3] org.apache.log.output.io.FileTarget.<init> (FileTarget.java:55)   [4] org.apache.avalon.excalibur.logger.factory.FileTargetFactory.createTarget (FileTargetFactory.java:164)   [5] org.apache.avalon.excalibur.logger.factory.FileTargetFactory.createTarget (FileTargetFactory.java:145)   [6] org.apache.avalon.excalibur.logger.DefaultLogTargetManager.configure (DefaultLogTargetManager.java:92)   [7] org.apache.avalon.framework.container.ContainerUtil.configure (ContainerUtil.java:201)   [8] org.apache.avalon.excalibur.logger.LogKitLoggerManager.setupTargetManager (LogKitLoggerManager.java:457)   [9] org.apache.avalon.excalibur.logger.LogKitLoggerManager.configure (LogKitLoggerManager.java:403)   [10] org.apache.cocoon.bean.CocoonWrapper.initialize (CocoonWrapper.java:144)   [11] org.apache.cocoon.bean.CocoonBean.initialize (CocoonBean.java:102)   [12] org.apache.cocoon.Main.main (Main.java:320)
        Hide
        Brian M Dube added a comment -
        This seems to work. It's the same idea as Mathieu's proposed change, but it avoids changing the visibility of ForrestConfUtils.getSystemProperty().

        Index: main/java/org/apache/forrest/log/ForrestLogTargetFactory.java
        ===================================================================
        --- main/java/org/apache/forrest/log/ForrestLogTargetFactory.java (revision 1057988)
        +++ main/java/org/apache/forrest/log/ForrestLogTargetFactory.java (working copy)
        @@ -42,7 +42,7 @@
         
                     if(!projectHome.startsWith(ForrestConfUtils.defaultHome)){
                         DefaultContext newContext = new DefaultContext(context);
        - newContext.put("context-root",projectHome + "/build/webapp");
        + newContext.put("context-root", ForrestConfUtils.getProjectWebappHome());
                         currentContext = newContext;
                     }
                 } catch (Exception e) {
        Index: main/java/org/apache/forrest/conf/ForrestConfUtils.java
        ===================================================================
        --- main/java/org/apache/forrest/conf/ForrestConfUtils.java (revision 1057988)
        +++ main/java/org/apache/forrest/conf/ForrestConfUtils.java (working copy)
        @@ -89,6 +89,10 @@
                 return contextHome;
             }
         
        + public static String getProjectWebappHome() {
        + return getSystemProperty("project.webapp");
        + }
        +
             /**
              * For backwards compatibility, alias old skin names to new ones. This must
              * be kept in sync with aliasing in forrest.build.xml/init-props
        Show
        Brian M Dube added a comment - This seems to work. It's the same idea as Mathieu's proposed change, but it avoids changing the visibility of ForrestConfUtils.getSystemProperty(). Index: main/java/org/apache/forrest/log/ForrestLogTargetFactory.java =================================================================== --- main/java/org/apache/forrest/log/ForrestLogTargetFactory.java (revision 1057988) +++ main/java/org/apache/forrest/log/ForrestLogTargetFactory.java (working copy) @@ -42,7 +42,7 @@                if(!projectHome.startsWith(ForrestConfUtils.defaultHome)){                  DefaultContext newContext = new DefaultContext(context); - newContext.put("context-root",projectHome + "/build/webapp"); + newContext.put("context-root", ForrestConfUtils.getProjectWebappHome());                  currentContext = newContext;              }          } catch (Exception e) { Index: main/java/org/apache/forrest/conf/ForrestConfUtils.java =================================================================== --- main/java/org/apache/forrest/conf/ForrestConfUtils.java (revision 1057988) +++ main/java/org/apache/forrest/conf/ForrestConfUtils.java (working copy) @@ -89,6 +89,10 @@          return contextHome;      }   + public static String getProjectWebappHome() { + return getSystemProperty("project.webapp"); + } +      /**       * For backwards compatibility, alias old skin names to new ones. This must       * be kept in sync with aliasing in forrest.build.xml/init-props
        Hide
        Brian M Dube added a comment -
        Contributed patch applied with thanks.
        Show
        Brian M Dube added a comment - Contributed patch applied with thanks.

          People

          • Assignee:
            Unassigned
            Reporter:
            Richard Calmbach
          • Votes:
            1 Vote for this issue
            Watchers:
            1 Start watching this issue

            Dates

            • Created:
              Updated:
              Resolved:

              Development