Re: svn commit: r1845933 - /ofbiz/ofbiz-framework/trunk/framework/base/src/main/java/org/apache/ofbiz/base/util/GroovyUtil.java

[Pierre Smits-3]

Hi Jacques,

You're referencing a groovy script that is not located in the agreed
folder, which is the 'groovyScript' folder. Is there a reason why the
referenced file is in the config folder?

Best regards,

Pierre Smits

*Apache Trafodion <https://trafodion.apache.org>, Vice President*
*Apache Directory <https://directory.apache.org>, PMC Member*
Apache Incubator <https://incubator.apache.org>, committer
*Apache OFBiz <https://ofbiz.apache.org>, contributor (without privileges)
since 2008*
Apache Steve <https://steve.apache.org>, committer


On Tue, Nov 6, 2018 at 5:33 PM <[hidden email]> wrote:

> Author: jleroux
> Date: Tue Nov  6 16:33:11 2018
> New Revision: 1845933
>
> URL: http://svn.apache.org/viewvc?rev=1845933&view=rev
> Log:
> Fixed: Error in GetLocaleListTests.java
> (OFBIZ-10641)
>
> A test error related to GetLocaleListTests.java seems to happen only on a
> fresh
> trunk install when running unit tests. It's no stopping tests.
>
> I'm not sure yet if R17 is concerned. I did not try, but will 1st confirm
> with
> this commit with trunk demo error.loç
>
> Mathieu suggested to  replace the location
> component://base/config/GroovyInit.groovy
> with ofbizhome://framework/base/config/GroovyInit.groovy
> because GroovyUtil [should not] depend on the component container in the
> 1st place, and it seems to work.
>
> Thanks: Mathieu for the idea.
>
> Modified:
>
> ofbiz/ofbiz-framework/trunk/framework/base/src/main/java/org/apache/ofbiz/base/util/GroovyUtil.java
>
> Modified:
> ofbiz/ofbiz-framework/trunk/framework/base/src/main/java/org/apache/ofbiz/base/util/GroovyUtil.java
> URL:
> http://svn.apache.org/viewvc/ofbiz/ofbiz-framework/trunk/framework/base/src/main/java/org/apache/ofbiz/base/util/GroovyUtil.java?rev=1845933&r1=1845932&r2=1845933&view=diff
>
> ==============================================================================
> ---
> ofbiz/ofbiz-framework/trunk/framework/base/src/main/java/org/apache/ofbiz/base/util/GroovyUtil.java
> (original)
> +++
> ofbiz/ofbiz-framework/trunk/framework/base/src/main/java/org/apache/ofbiz/base/util/GroovyUtil.java
> Tue Nov  6 16:33:11 2018
> @@ -63,7 +63,7 @@ public class GroovyUtil {
>           *  the workaround is to execute at startup a script containing
> the @BaseScript annotation.
>           */
>          try {
> -
> GroovyUtil.runScriptAtLocation("component://base/config/GroovyInit.groovy",
> null, null);
> +
> GroovyUtil.runScriptAtLocation("ofbizhome://framework/base/config/GroovyInit.groovy",
> null, null);
>          } catch (Exception e) {
>              Debug.logWarning("The following error occurred during the
> initialization of Groovy: " + e.getMessage(), module);
>          }
>
>
>

[Mathieu Lirzin]

Hello Pierre,

Pierre Smits <[hidden email]> writes:

> You're referencing a groovy script that is not located in the agreed
> folder, which is the 'groovyScript' folder. Is there a reason why the
> referenced file is in the config folder?

According to the developper documentation, ‘groovyScripts’ is the
“agreed” folder.  It appears that the ‘base’ component is not following
this convention by using the singular form:

--8<---------------cut here---------------start------------->8---
~/src/ofbiz$ find . -name 'groovyScript*'
./applications/accounting/groovyScripts
./applications/product/groovyScripts
./applications/content/groovyScripts
./applications/humanres/groovyScripts
./applications/commonext/groovyScripts
./applications/party/groovyScripts
./applications/marketing/groovyScripts
./applications/manufacturing/groovyScripts
./applications/order/groovyScripts
./applications/workeffort/groovyScripts
./framework/webtools/groovyScripts
./framework/common/groovyScripts
./framework/base/groovyScript
--8<---------------cut here---------------end--------------->8---

--
Mathieu Lirzin
GPG: F2A3 8D7E EB2B 6640 5761  070D 0ADE E100 9460 4D37

[Jacques Le Roux]

Hi Pierre, Mathieu,

I did not notice and I have no ideas why it's like that. We need to look at the history to see if there is a reason.

Jacques


Le 08/11/2018 à 18:10, Mathieu Lirzin a écrit :

> Hello Pierre,
>
> Pierre Smits <[hidden email]> writes:
>
>> You're referencing a groovy script that is not located in the agreed
>> folder, which is the 'groovyScript' folder. Is there a reason why the
>> referenced file is in the config folder?
> According to the developper documentation, ‘groovyScripts’ is the
> “agreed” folder.  It appears that the ‘base’ component is not following
> this convention by using the singular form:
>
> --8<---------------cut here---------------start------------->8---
> ~/src/ofbiz$ find . -name 'groovyScript*'
> ./applications/accounting/groovyScripts
> ./applications/product/groovyScripts
> ./applications/content/groovyScripts
> ./applications/humanres/groovyScripts
> ./applications/commonext/groovyScripts
> ./applications/party/groovyScripts
> ./applications/marketing/groovyScripts
> ./applications/manufacturing/groovyScripts
> ./applications/order/groovyScripts
> ./applications/workeffort/groovyScripts
> ./framework/webtools/groovyScripts
> ./framework/common/groovyScripts
> ./framework/base/groovyScript
> --8<---------------cut here---------------end--------------->8---
>

[Jacques Le Roux]

Here are the Groovy files in base not in groovyScript folder

if-script.groovy (unused OOTB, it seems we should get rid of)
GroovyInit.groovy (I see indeed no reasons to have in config, that's history)

There are 2 other exceptions in framework (not in applications, nor plugins):

GroovyServiceTest.groovy (in minilang should be in the groovyScript folder)
GroovyBaseScript.groovy (unused OOTB, it seems we should get rid of)

Jacques


Le 08/11/2018 à 20:33, Jacques Le Roux a écrit :

> Hi Pierre, Mathieu,
>
> I did not notice and I have no ideas why it's like that. We need to look at the history to see if there is a reason.
>
> Jacques
>
>
> Le 08/11/2018 à 18:10, Mathieu Lirzin a écrit :
>> Hello Pierre,
>>
>> Pierre Smits <[hidden email]> writes:
>>
>>> You're referencing a groovy script that is not located in the agreed
>>> folder, which is the 'groovyScript' folder. Is there a reason why the
>>> referenced file is in the config folder?
>> According to the developper documentation, ‘groovyScripts’ is the
>> “agreed” folder.  It appears that the ‘base’ component is not following
>> this convention by using the singular form:
>>
>> --8<---------------cut here---------------start------------->8---
>> ~/src/ofbiz$ find . -name 'groovyScript*'
>> ./applications/accounting/groovyScripts
>> ./applications/product/groovyScripts
>> ./applications/content/groovyScripts
>> ./applications/humanres/groovyScripts
>> ./applications/commonext/groovyScripts
>> ./applications/party/groovyScripts
>> ./applications/marketing/groovyScripts
>> ./applications/manufacturing/groovyScripts
>> ./applications/order/groovyScripts
>> ./applications/workeffort/groovyScripts
>> ./framework/webtools/groovyScripts
>> ./framework/common/groovyScripts
>> ./framework/base/groovyScript
>> --8<---------------cut here---------------end--------------->8---
>>
>
>

[Jacques Le Roux]

Le 09/11/2018 à 11:45, Jacques Le Roux a écrit :
> GroovyBaseScript.groovy (unused OOTB, it seems we should get rid of)
I only made a search on the complete name, but this is injected with groovy.properties using the @BaseScript annotation
It was introduced by Jacopo and relates to Groovy DSL: http://svn.apache.org/viewvc?view=revision&revision=1298461

So quite used, and need to be in this place

Jacques

[Jacques Le Roux]

Hi,

So we would

remove ant-scripts folder and its content;

rename framework/base/groovyScript to framework/base/groovyScripts;

move GroovyInit.groovy and GroovyServiceTest.groovy to framework/base/groovyScripts and change their paths where used.

And let GroovyBaseScript.groovy where it is

Right?

Jacques


Le 09/11/2018 à 12:19, Jacques Le Roux a écrit :

> Le 09/11/2018 à 11:45, Jacques Le Roux a écrit :
>> GroovyBaseScript.groovy (unused OOTB, it seems we should get rid of)
> I only made a search on the complete name, but this is injected with groovy.properties using the @BaseScript annotation
> It was introduced by Jacopo and relates to Groovy DSL: http://svn.apache.org/viewvc?view=revision&revision=1298461
>
> So quite used, and need to be in this place
>
> Jacques
>
>

[Mathieu Lirzin]

Hello Jacques,

Jacques Le Roux <[hidden email]> writes:

> So we would
>
> remove ant-scripts folder and its content;
>
> rename framework/base/groovyScript to framework/base/groovyScripts;
>
> move GroovyInit.groovy and GroovyServiceTest.groovy to framework/base/groovyScripts and change their paths where used.
>
> And let GroovyBaseScript.groovy where it is
>
> Right?

IMO since ‘GroovyBaseScript.groovy’ is not a java file it should not be
placed in:

 ./framework/service/src/main/java/org/apache/ofbiz/service/engine

but rather in:

 ./framework/service/src/main/groovy/org/apache/ofbiz/service/engine

Since Groovy is not configured in Gradle this might not work OOTB
because the ‘GroovyBaseScript’ will probably not be copied in
‘ofbiz.jar’ and not defined in the classpath of the MANIFEST.

However if we were to apply OFBIZ-10611 [1] which adds the Gradle Groovy
plugin for supporting unit tests written in Groovy beforehand, it should
work gracefully.

Otherwise looks good to me.

[1] https://issues.apache.org/jira/projects/OFBIZ/issues/OFBIZ-10611
--
Mathieu Lirzin
GPG: F2A3 8D7E EB2B 6640 5761  070D 0ADE E100 9460 4D37

[Jacques Le Roux]

Done :)

Jacques


Le 11/11/2018 à 13:31, Mathieu Lirzin a écrit :

> Hello Jacques,
>
> Jacques Le Roux <[hidden email]> writes:
>
>> So we would
>>
>> remove ant-scripts folder and its content;
>>
>> rename framework/base/groovyScript to framework/base/groovyScripts;
>>
>> move GroovyInit.groovy and GroovyServiceTest.groovy to framework/base/groovyScripts and change their paths where used.
>>
>> And let GroovyBaseScript.groovy where it is
>>
>> Right?
> IMO since ‘GroovyBaseScript.groovy’ is not a java file it should not be
> placed in:
>
>   ./framework/service/src/main/java/org/apache/ofbiz/service/engine
>
> but rather in:
>
>   ./framework/service/src/main/groovy/org/apache/ofbiz/service/engine
>
> Since Groovy is not configured in Gradle this might not work OOTB
> because the ‘GroovyBaseScript’ will probably not be copied in
> ‘ofbiz.jar’ and not defined in the classpath of the MANIFEST.
>
> However if we were to apply OFBIZ-10611 [1] which adds the Gradle Groovy
> plugin for supporting unit tests written in Groovy beforehand, it should
> work gracefully.
>
> Otherwise looks good to me.
>
> [1] https://issues.apache.org/jira/projects/OFBIZ/issues/OFBIZ-10611