Uploaded image for project: 'OFBiz'
  1. OFBiz
  2. OFBIZ-8250

SvnInfo.ftl and GitInfo.ftl are not generated by default

    Details

    • Type: Bug
    • Status: Closed
    • Priority: Minor
    • Resolution: Fixed
    • Affects Version/s: Trunk
    • Fix Version/s: 16.11.01
    • Component/s: ALL APPLICATIONS
    • Labels:
      None
    • Sprint:
      Community Day 3 - 2016

      Description

      The two files are not generated which results in an FTL stacktrace in the footer of the backend applications.

      1. Image 014.png
        6 kB
        Jacques Le Roux
      2. OFBIZ-8250.patch
        2 kB
        Michael Brohl
      3. OFBIZ-8250-2.patch
        2 kB
        Michael Brohl

        Issue Links

          Activity

          Hide
          mbrohl Michael Brohl added a comment -

          The attached patch defines a task to generate default empty files and a cleanup task to remove them.

          Show
          mbrohl Michael Brohl added a comment - The attached patch defines a task to generate default empty files and a cleanup task to remove them.
          Hide
          mbrohl Michael Brohl added a comment -

          Taher Alkhateeb,

          can you please review the patch if it is correct and follows best practices for the gradle build?

          The task for file generation should be executed on every gradle execution, that happens e.g. when ./gradlew ofbiz is executed.
          The cleanFooterFiles task can be executed standalone and should be executed also when cleanAll is run.

          I have defined the files in the upper section of the gradle build file and removed them in the existing generation tasks for the git/svn info.

          Thanks for feedback,
          Michael

          Show
          mbrohl Michael Brohl added a comment - Taher Alkhateeb , can you please review the patch if it is correct and follows best practices for the gradle build? The task for file generation should be executed on every gradle execution, that happens e.g. when ./gradlew ofbiz is executed. The cleanFooterFiles task can be executed standalone and should be executed also when cleanAll is run. I have defined the files in the upper section of the gradle build file and removed them in the existing generation tasks for the git/svn info. Thanks for feedback, Michael
          Hide
          mbrohl Michael Brohl added a comment -

          This patch is an improvement to the first one. The creation of the files is not done in a task but in a function during the configuration phase. Thanks Taher for the guidance.

          Show
          mbrohl Michael Brohl added a comment - This patch is an improvement to the first one. The creation of the files is not done in a task but in a function during the configuration phase. Thanks Taher for the guidance.
          Hide
          taher Taher Alkhateeb added a comment -

          looks good, +1

          Show
          taher Taher Alkhateeb added a comment - looks good, +1
          Hide
          jacques.le.roux Jacques Le Roux added a comment -

          I'd like to review this patch but I'm short on time. I'm not questionning the patch but the reason why it's done. Maybe you can explain? Else please wait before committing, thanks!

          Show
          jacques.le.roux Jacques Le Roux added a comment - I'd like to review this patch but I'm short on time. I'm not questionning the patch but the reason why it's done. Maybe you can explain? Else please wait before committing, thanks!
          Hide
          taher Taher Alkhateeb added a comment -

          Hi Jacques. Michael discovered that by default on a clean checkout of OFBiz the files SvnInfo.ftl and GitInfo.ftl are not generated (but they used to be generated during the days of Ant). So this means, on a new checkout doing ./gradlew cleanAll loadDefault "ofbiz --start" would yield exceptions at the bottom of the page because of missing footer files.

          The purpose of the patch designed by Michael as far as I see is to generate these files (empty) if they are missing to avoid this exception stack-trace in the user interface, and he also added a clean task to remove the files when calling cleanAll. So what Michael did is make things nice and automatic so that users don't have to worry or intentionally call svnInfoFooter and gitInfoFooter.

          Show
          taher Taher Alkhateeb added a comment - Hi Jacques. Michael discovered that by default on a clean checkout of OFBiz the files SvnInfo.ftl and GitInfo.ftl are not generated (but they used to be generated during the days of Ant). So this means, on a new checkout doing ./gradlew cleanAll loadDefault "ofbiz --start" would yield exceptions at the bottom of the page because of missing footer files. The purpose of the patch designed by Michael as far as I see is to generate these files (empty) if they are missing to avoid this exception stack-trace in the user interface, and he also added a clean task to remove the files when calling cleanAll. So what Michael did is make things nice and automatic so that users don't have to worry or intentionally call svnInfoFooter and gitInfoFooter.
          Hide
          jacques.le.roux Jacques Le Roux added a comment -

          Thanks Taher, it will be much easy to review the patch . BTW Michael, please no Eclipse headers in patches

          Show
          jacques.le.roux Jacques Le Roux added a comment - Thanks Taher, it will be much easy to review the patch . BTW Michael, please no Eclipse headers in patches
          Hide
          taher Taher Alkhateeb added a comment -

          Unless you caught an error, I don't think you need to slow down this JIRA with a review. Michael checked it, I checked it, it runs, it works. You said yourself that your objective is to understand the reason not correctness of the patch which I explained above (maybe Michael can add more). This patch is simply a bug fix for not generating the footer files.

          Show
          taher Taher Alkhateeb added a comment - Unless you caught an error, I don't think you need to slow down this JIRA with a review. Michael checked it, I checked it, it runs, it works. You said yourself that your objective is to understand the reason not correctness of the patch which I explained above (maybe Michael can add more). This patch is simply a bug fix for not generating the footer files.
          Hide
          jacques.le.roux Jacques Le Roux added a comment -

          Please don't tell me what I have to do or not, thanks!

          Show
          jacques.le.roux Jacques Le Roux added a comment - Please don't tell me what I have to do or not, thanks!
          Hide
          jacques.le.roux Jacques Le Roux added a comment - - edited

          My point is we just need to add ignore_missing=true in #include directives in footers and ignore this patch.

          http://freemarker.org/docs/ref_directive_include.html

          Show
          jacques.le.roux Jacques Le Roux added a comment - - edited My point is we just need to add ignore_missing=true in #include directives in footers and ignore this patch. http://freemarker.org/docs/ref_directive_include.html
          Hide
          taher Taher Alkhateeb added a comment -

          You make it sound like I gave an order Do whatever you like

          Show
          taher Taher Alkhateeb added a comment - You make it sound like I gave an order Do whatever you like
          Hide
          taher Taher Alkhateeb added a comment -

          Hmmm, we are still left with the problem of generated files from svnInfoFooter and gitInfoFooter not cleaned up. Anyway, I leave this discussion for you and Michael to continue to devise a solution.

          Show
          taher Taher Alkhateeb added a comment - Hmmm, we are still left with the problem of generated files from svnInfoFooter and gitInfoFooter not cleaned up. Anyway, I leave this discussion for you and Michael to continue to devise a solution.
          Hide
          mbrohl Michael Brohl added a comment - - edited

          This patch basically implements a functionality we had in the old ant build target:

              <target name="build" depends="ofbiz-init">
                  <echo message="[build] ========== Start Building (Compile) =========="/>
                  <antcall target="build-dev"/>
          
                  <antcall target="build-framework"/>
                  <antcall target="build-applications"/>
                  <antcall target="build-specialpurpose"/>
                  <externalsubant>
                      <fileset dir="${basedir}/themes">
                          <include name="*/build.xml" />
                      </fileset>
                  </externalsubant>
          
                  <hotdeployant/>
                  <antcall target="clean-svninfo"/>
                  <antcall target="clean-gitinfo"/>
          
                  <echo message="[build] ========== Done Building (Compile) =========="/>
              </target>
          
          
              <target name="clean-svninfo">
                  <echo message="Resetting svninfo..."/>
                  <echo message=" " file="runtime/svninfo.ftl"/>
                  <echo message="Done!"/>
              </target>
          
          Show
          mbrohl Michael Brohl added a comment - - edited This patch basically implements a functionality we had in the old ant build target: <target name= "build" depends= "ofbiz-init" > <echo message= "[build] ========== Start Building (Compile) ==========" /> <antcall target= "build-dev" /> <antcall target= "build-framework" /> <antcall target= "build-applications" /> <antcall target= "build-specialpurpose" /> <externalsubant> <fileset dir= "${basedir}/themes" > <include name= "*/build.xml" /> </fileset> </externalsubant> <hotdeployant/> <antcall target= "clean-svninfo" /> <antcall target= "clean-gitinfo" /> <echo message= "[build] ========== Done Building (Compile) ==========" /> </target> <target name= "clean-svninfo" > <echo message= "Resetting svninfo..." /> <echo message= " " file= "runtime/svninfo.ftl" /> <echo message= "Done!" /> </target>
          Hide
          mbrohl Michael Brohl added a comment -

          Yes, the cleanup part is needed anyway. Of course, we can also change the includes in every themes' footer.
          I find it more efficient to have the files in place per default.

          Show
          mbrohl Michael Brohl added a comment - Yes, the cleanup part is needed anyway. Of course, we can also change the includes in every themes' footer. I find it more efficient to have the files in place per default.
          Hide
          jacopoc Jacopo Cappellato added a comment -

          I don't see a problem with the header, since it is a comment that doesn't cause any confusion or issue while reviewing or applying the patch.

          Show
          jacopoc Jacopo Cappellato added a comment - I don't see a problem with the header, since it is a comment that doesn't cause any confusion or issue while reviewing or applying the patch.
          Hide
          jacques.le.roux Jacques Le Roux added a comment -

          This is only a issue in Eclipse. When you apply it in Eclipse, it then searches for a project that you might not have. Mine is not named ofbiz-svn for instance

          Show
          jacques.le.roux Jacques Le Roux added a comment - This is only a issue in Eclipse. When you apply it in Eclipse, it then searches for a project that you might not have. Mine is not named ofbiz-svn for instance
          Hide
          mbrohl Michael Brohl added a comment -

          How does Eclipse behave on your site if the project stated in the header does not exist in the workspace?

          Show
          mbrohl Michael Brohl added a comment - How does Eclipse behave on your site if the project stated in the header does not exist in the workspace?
          Hide
          jacques.le.roux Jacques Le Roux added a comment -


          I'm surprised that you don't know that. BTW it's already in https://cwiki.apache.org/confluence/display/OFBADMIN/OFBiz+Contributors+Best+Practices see

          Eclipse internal command (don't use finish but rather select project to avoid the 2 1st lines in the patch)

          Show
          jacques.le.roux Jacques Le Roux added a comment - I'm surprised that you don't know that. BTW it's already in https://cwiki.apache.org/confluence/display/OFBADMIN/OFBiz+Contributors+Best+Practices see Eclipse internal command (don't use finish but rather select project to avoid the 2 1st lines in the patch)
          Hide
          jacques.le.roux Jacques Le Roux added a comment - - edited

          See my comment below. It's a very minor thing, because you can copy/paste the patch somewhere and remove the lines. But it's easier to not have those lines in 1st place, just a reflex to get.

          Show
          jacques.le.roux Jacques Le Roux added a comment - - edited See my comment below. It's a very minor thing, because you can copy/paste the patch somewhere and remove the lines. But it's easier to not have those lines in 1st place, just a reflex to get.
          Hide
          jacques.le.roux Jacques Le Roux added a comment -

          I'll propose something tomorrow, I'm very surprised by a Freemarker behaviour...

          Show
          jacques.le.roux Jacques Le Roux added a comment - I'll propose something tomorrow, I'm very surprised by a Freemarker behaviour...
          Hide
          jacques.le.roux Jacques Le Roux added a comment -

          After reading at http://freemarker.org/docs/ref_directive_include.html the ignore_missing description of this <#include attribute

          ignore_missing: When true, suppresses the error when the template to include is missing, instead <#include ...> will print nothing. When false, the template processing will stop with error if the template is missing. If you omit this option, then it defaults to false.

          I thought that we could get rid of the empty files creation when building. But like Deepak here I found that it's not working.
          So I agree that the proposed patch is the best solution so far. I have reopened OFBIZ-7942 because I did not understood the problem there when I closed.

          We need to report to Apache Freemarker (incubating) because as mentionned Deepak the ignore_missing attribute of the <#include directive does not work. When it will work we will use it and remove the need of creating empty files when running the build task.

          I commit the proposed patch now, thanks for your patience

          Show
          jacques.le.roux Jacques Le Roux added a comment - After reading at http://freemarker.org/docs/ref_directive_include.html the ignore_missing description of this <#include attribute ignore_missing: When true, suppresses the error when the template to include is missing, instead <#include ...> will print nothing. When false, the template processing will stop with error if the template is missing. If you omit this option, then it defaults to false. I thought that we could get rid of the empty files creation when building. But like Deepak here I found that it's not working. So I agree that the proposed patch is the best solution so far. I have reopened OFBIZ-7942 because I did not understood the problem there when I closed. We need to report to Apache Freemarker (incubating) because as mentionned Deepak the ignore_missing attribute of the <#include directive does not work. When it will work we will use it and remove the need of creating empty files when running the build task. I commit the proposed patch now, thanks for your patience
          Hide
          jacques.le.roux Jacques Le Roux added a comment - - edited

          Thanks Michael, Taher

          Your patch is in trunk at revision: 1761392

          I'll also close OFBIZ-7942 as a duplicate

          Show
          jacques.le.roux Jacques Le Roux added a comment - - edited Thanks Michael, Taher Your patch is in trunk at revision: 1761392 I'll also close OFBIZ-7942 as a duplicate
          Hide
          taher Taher Alkhateeb added a comment -

          Hi Jacques, thank you for the commit. I think it would be more appropriate in the future to let committers commit their own work. Michael added the patch in this JIRA for feedback and help, not committing, as he is the assignee of this JIRA.

          Show
          taher Taher Alkhateeb added a comment - Hi Jacques, thank you for the commit. I think it would be more appropriate in the future to let committers commit their own work. Michael added the patch in this JIRA for feedback and help, not committing, as he is the assignee of this JIRA.
          Hide
          jacques.le.roux Jacques Le Roux added a comment -

          I thought after making both of you wait it was more correct to commit myself, anyway not a big deal.

          Show
          jacques.le.roux Jacques Le Roux added a comment - I thought after making both of you wait it was more correct to commit myself, anyway not a big deal.
          Hide
          jacopoc Jacopo Cappellato added a comment -

          Thank you Michael Brohl for providing the fix for this annoying error; it was bothering me so thanks for taking the time to implement the patch!

          Show
          jacopoc Jacopo Cappellato added a comment - Thank you Michael Brohl for providing the fix for this annoying error; it was bothering me so thanks for taking the time to implement the patch!

            People

            • Assignee:
              mbrohl Michael Brohl
              Reporter:
              mbrohl Michael Brohl
            • Votes:
              0 Vote for this issue
              Watchers:
              3 Start watching this issue

              Dates

              • Created:
                Updated:
                Resolved:

                Development

                  Agile