Hi devs,
any objections to get this into the codebase (and backporting to 17.12./18.12)? See https://issues.apache.org/jira/browse/OFBIZ-10168 Thanks for your feedback, Michael Brohl ecomify GmbH - www.ecomify.de smime.p7s (5K) Download Attachment |
Oops, just realized I already asked in
https://lists.apache.org/thread.html/rf88ce5eb1a3842471ddb8239bb5f08b71c107c4b0d3863b3c13e54fe%40%3Cdev.ofbiz.apache.org%3E Better respond there, thanks! Michael Am 25.02.20 um 11:29 schrieb Michael Brohl: > Hi devs, > > any objections to get this into the codebase (and backporting to > 17.12./18.12)? > > See https://issues.apache.org/jira/browse/OFBIZ-10168 > > Thanks for your feedback, > > Michael Brohl > > ecomify GmbH - www.ecomify.de > > > smime.p7s (5K) Download Attachment |
Hi Michael,
The idea is fine but I think we can improve the implementation. All that you need is to remove the dependsOn declaration if it is a shutdown command which would probably result in less code than in the proposed patch. Cheers On Tue, Feb 25, 2020, 1:32 PM Michael Brohl <[hidden email]> wrote: > Oops, just realized I already asked in > > https://lists.apache.org/thread.html/rf88ce5eb1a3842471ddb8239bb5f08b71c107c4b0d3863b3c13e54fe%40%3Cdev.ofbiz.apache.org%3E > > Better respond there, thanks! > > Michael > > > Am 25.02.20 um 11:29 schrieb Michael Brohl: > > Hi devs, > > > > any objections to get this into the codebase (and backporting to > > 17.12./18.12)? > > > > See https://issues.apache.org/jira/browse/OFBIZ-10168 > > > > Thanks for your feedback, > > > > Michael Brohl > > > > ecomify GmbH - www.ecomify.de > > > > > > > > |
Hi Taher,
thanks for your feedback! It seems that removing a dependency from a task is deprecated and should not be used: "Do not remove a task dependency from a Task instance. This behaviour has been deprecated and is scheduled to be removed in Gradle 6.0." Given this depracation, do you think there is another way to reduce the code added? Thanks! Am 25.02.20 um 12:14 schrieb Taher Alkhateeb: > Hi Michael, > > The idea is fine but I think we can improve the implementation. All that > you need is to remove the dependsOn declaration if it is a shutdown command > which would probably result in less code than in the proposed patch. > > Cheers > > On Tue, Feb 25, 2020, 1:32 PM Michael Brohl <[hidden email]> > wrote: > >> Oops, just realized I already asked in >> >> https://lists.apache.org/thread.html/rf88ce5eb1a3842471ddb8239bb5f08b71c107c4b0d3863b3c13e54fe%40%3Cdev.ofbiz.apache.org%3E >> >> Better respond there, thanks! >> >> Michael >> >> >> Am 25.02.20 um 11:29 schrieb Michael Brohl: >>> Hi devs, >>> >>> any objections to get this into the codebase (and backporting to >>> 17.12./18.12)? >>> >>> See https://issues.apache.org/jira/browse/OFBIZ-10168 >>> >>> Thanks for your feedback, >>> >>> Michael Brohl >>> >>> ecomify GmbH - www.ecomify.de >>> >>> >>> >> smime.p7s (5K) Download Attachment |
Hi Michael,
I could be mistaken, but I think perhaps you're trying to remove the dependency during the execution phase instead of the configuration phase? Would a patch like the below not work? diff --git a/build.gradle b/build.gradle index e5bfb3e9c9..96dcb53a01 100644 --- a/build.gradle +++ b/build.gradle @@ -976,11 +976,14 @@ tasks.addRule('Pattern: ofbizBackground <Commands>: Execute OFBiz startup comman * ======================================================== */ def createOfbizCommandTask(taskName, arguments) { - task(type: JavaExec, dependsOn: classes, taskName) { + task(taskName, type: JavaExec) { jvmArgs(application.applicationDefaultJvmArgs) classpath = sourceSets.main.runtimeClasspath main = application.mainClassName args arguments + if (!(taskName ==~ /^ofbiz.*(--shutdown|-d).*/)) { + dependsOn classes + } if (taskName ==~ /^ofbiz.*(--test|-t).*/) { finalizedBy(createTestReports) } On Tue, Feb 25, 2020 at 8:48 PM Michael Brohl <[hidden email]> wrote: > > Hi Taher, > > thanks for your feedback! > > It seems that removing a dependency from a task is deprecated and should > not be used: > > "Do not remove a task dependency from a Task instance. This behaviour > has been deprecated and is scheduled to be removed in Gradle 6.0." > > Given this depracation, do you think there is another way to reduce the > code added? > > Thanks! > > > Am 25.02.20 um 12:14 schrieb Taher Alkhateeb: > > Hi Michael, > > > > The idea is fine but I think we can improve the implementation. All that > > you need is to remove the dependsOn declaration if it is a shutdown command > > which would probably result in less code than in the proposed patch. > > > > Cheers > > > > On Tue, Feb 25, 2020, 1:32 PM Michael Brohl <[hidden email]> > > wrote: > > > >> Oops, just realized I already asked in > >> > >> https://lists.apache.org/thread.html/rf88ce5eb1a3842471ddb8239bb5f08b71c107c4b0d3863b3c13e54fe%40%3Cdev.ofbiz.apache.org%3E > >> > >> Better respond there, thanks! > >> > >> Michael > >> > >> > >> Am 25.02.20 um 11:29 schrieb Michael Brohl: > >>> Hi devs, > >>> > >>> any objections to get this into the codebase (and backporting to > >>> 17.12./18.12)? > >>> > >>> See https://issues.apache.org/jira/browse/OFBIZ-10168 > >>> > >>> Thanks for your feedback, > >>> > >>> Michael Brohl > >>> > >>> ecomify GmbH - www.ecomify.de > >>> > >>> > >>> > >> > |
Hi Taher,
the configuration in your patch is executed correctly, I logged it with this code enhancement: if (!(taskName ==~ /^ofbiz.*(--shutdown|-d).*/)) { println taskName + ' dependsOn classes' dependsOn classes } else { println taskName + ' NO dependsOn classes' } ./gradlew "ofbiz --shutdown" --dry-run > Configure project : executeLoadAdminUser dependsOn classes executeLoadTenant dependsOn classes loadTenantOnMasterTenantDb dependsOn classes loadTenantData dependsOn classes loadTenantAdminUserLogin dependsOn classes ofbiz --shutdown NO dependsOn classes :compileJava SKIPPED :compileGroovy SKIPPED :processResources SKIPPED :classes SKIPPED :ofbiz --shutdown SKIPPED but it still seems to execute the whole task dependencies: > Task :compileJava UP-TO-DATE > Task :compileGroovy UP-TO-DATE > Task :processResources UP-TO-DATE > Task :classes UP-TO-DATE > Task :ofbiz --shutdown The plugin https://github.com/dorongold/gradle-task-tree shows the following task dependencies: :ofbiz --shutdown \--- :classes +--- :compileGroovy | \--- :compileJava +--- :compileJava \--- :processResources Not sure why... Thanks, Michael Am 26.02.20 um 13:53 schrieb Taher Alkhateeb: > Hi Michael, > > I could be mistaken, but I think perhaps you're trying to remove the > dependency during the execution phase instead of the configuration > phase? > > Would a patch like the below not work? > > diff --git a/build.gradle b/build.gradle > index e5bfb3e9c9..96dcb53a01 100644 > --- a/build.gradle > +++ b/build.gradle > @@ -976,11 +976,14 @@ tasks.addRule('Pattern: ofbizBackground > <Commands>: Execute OFBiz startup comman > * ======================================================== */ > > def createOfbizCommandTask(taskName, arguments) { > - task(type: JavaExec, dependsOn: classes, taskName) { > + task(taskName, type: JavaExec) { > jvmArgs(application.applicationDefaultJvmArgs) > classpath = sourceSets.main.runtimeClasspath > main = application.mainClassName > args arguments > + if (!(taskName ==~ /^ofbiz.*(--shutdown|-d).*/)) { > + dependsOn classes > + } > if (taskName ==~ /^ofbiz.*(--test|-t).*/) { > finalizedBy(createTestReports) > } > > On Tue, Feb 25, 2020 at 8:48 PM Michael Brohl <[hidden email]> wrote: >> Hi Taher, >> >> thanks for your feedback! >> >> It seems that removing a dependency from a task is deprecated and should >> not be used: >> >> "Do not remove a task dependency from a Task instance. This behaviour >> has been deprecated and is scheduled to be removed in Gradle 6.0." >> >> Given this depracation, do you think there is another way to reduce the >> code added? >> >> Thanks! >> >> >> Am 25.02.20 um 12:14 schrieb Taher Alkhateeb: >>> Hi Michael, >>> >>> The idea is fine but I think we can improve the implementation. All that >>> you need is to remove the dependsOn declaration if it is a shutdown command >>> which would probably result in less code than in the proposed patch. >>> >>> Cheers >>> >>> On Tue, Feb 25, 2020, 1:32 PM Michael Brohl <[hidden email]> >>> wrote: >>> >>>> Oops, just realized I already asked in >>>> >>>> https://lists.apache.org/thread.html/rf88ce5eb1a3842471ddb8239bb5f08b71c107c4b0d3863b3c13e54fe%40%3Cdev.ofbiz.apache.org%3E >>>> >>>> Better respond there, thanks! >>>> >>>> Michael >>>> >>>> >>>> Am 25.02.20 um 11:29 schrieb Michael Brohl: >>>>> Hi devs, >>>>> >>>>> any objections to get this into the codebase (and backporting to >>>>> 17.12./18.12)? >>>>> >>>>> See https://issues.apache.org/jira/browse/OFBIZ-10168 >>>>> >>>>> Thanks for your feedback, >>>>> >>>>> Michael Brohl >>>>> >>>>> ecomify GmbH - www.ecomify.de >>>>> >>>>> >>>>> smime.p7s (5K) Download Attachment |
I think I understand the reason why this is happening. Someone changed
the implementation of createOfbizCommandTask to depend on the classes directly instead of depending on the generated Jar. I'm not sure why but I personally do not prefer this approach. The solution is to revert to the original design where the classpath = files(thejarfilehere) and change dependsOn from classes to build On Wed, Feb 26, 2020 at 6:18 PM Michael Brohl <[hidden email]> wrote: > > Hi Taher, > > the configuration in your patch is executed correctly, I logged it with > this code enhancement: > > if (!(taskName ==~ /^ofbiz.*(--shutdown|-d).*/)) { > println taskName + ' dependsOn classes' > dependsOn classes > } else { > println taskName + ' NO dependsOn classes' > } > > ./gradlew "ofbiz --shutdown" --dry-run > > > Configure project : > executeLoadAdminUser dependsOn classes > executeLoadTenant dependsOn classes > loadTenantOnMasterTenantDb dependsOn classes > loadTenantData dependsOn classes > loadTenantAdminUserLogin dependsOn classes > ofbiz --shutdown NO dependsOn classes > :compileJava SKIPPED > :compileGroovy SKIPPED > :processResources SKIPPED > :classes SKIPPED > :ofbiz --shutdown SKIPPED > > but it still seems to execute the whole task dependencies: > > > Task :compileJava UP-TO-DATE > > Task :compileGroovy UP-TO-DATE > > Task :processResources UP-TO-DATE > > Task :classes UP-TO-DATE > > > Task :ofbiz --shutdown > > > The plugin https://github.com/dorongold/gradle-task-tree shows the > following task dependencies: > > :ofbiz --shutdown > \--- :classes > +--- :compileGroovy > | \--- :compileJava > +--- :compileJava > \--- :processResources > > > Not sure why... > > Thanks, > > Michael > > > Am 26.02.20 um 13:53 schrieb Taher Alkhateeb: > > Hi Michael, > > > > I could be mistaken, but I think perhaps you're trying to remove the > > dependency during the execution phase instead of the configuration > > phase? > > > > Would a patch like the below not work? > > > > diff --git a/build.gradle b/build.gradle > > index e5bfb3e9c9..96dcb53a01 100644 > > --- a/build.gradle > > +++ b/build.gradle > > @@ -976,11 +976,14 @@ tasks.addRule('Pattern: ofbizBackground > > <Commands>: Execute OFBiz startup comman > > * ======================================================== */ > > > > def createOfbizCommandTask(taskName, arguments) { > > - task(type: JavaExec, dependsOn: classes, taskName) { > > + task(taskName, type: JavaExec) { > > jvmArgs(application.applicationDefaultJvmArgs) > > classpath = sourceSets.main.runtimeClasspath > > main = application.mainClassName > > args arguments > > + if (!(taskName ==~ /^ofbiz.*(--shutdown|-d).*/)) { > > + dependsOn classes > > + } > > if (taskName ==~ /^ofbiz.*(--test|-t).*/) { > > finalizedBy(createTestReports) > > } > > > > On Tue, Feb 25, 2020 at 8:48 PM Michael Brohl <[hidden email]> wrote: > >> Hi Taher, > >> > >> thanks for your feedback! > >> > >> It seems that removing a dependency from a task is deprecated and should > >> not be used: > >> > >> "Do not remove a task dependency from a Task instance. This behaviour > >> has been deprecated and is scheduled to be removed in Gradle 6.0." > >> > >> Given this depracation, do you think there is another way to reduce the > >> code added? > >> > >> Thanks! > >> > >> > >> Am 25.02.20 um 12:14 schrieb Taher Alkhateeb: > >>> Hi Michael, > >>> > >>> The idea is fine but I think we can improve the implementation. All that > >>> you need is to remove the dependsOn declaration if it is a shutdown command > >>> which would probably result in less code than in the proposed patch. > >>> > >>> Cheers > >>> > >>> On Tue, Feb 25, 2020, 1:32 PM Michael Brohl <[hidden email]> > >>> wrote: > >>> > >>>> Oops, just realized I already asked in > >>>> > >>>> https://lists.apache.org/thread.html/rf88ce5eb1a3842471ddb8239bb5f08b71c107c4b0d3863b3c13e54fe%40%3Cdev.ofbiz.apache.org%3E > >>>> > >>>> Better respond there, thanks! > >>>> > >>>> Michael > >>>> > >>>> > >>>> Am 25.02.20 um 11:29 schrieb Michael Brohl: > >>>>> Hi devs, > >>>>> > >>>>> any objections to get this into the codebase (and backporting to > >>>>> 17.12./18.12)? > >>>>> > >>>>> See https://issues.apache.org/jira/browse/OFBIZ-10168 > >>>>> > >>>>> Thanks for your feedback, > >>>>> > >>>>> Michael Brohl > >>>>> > >>>>> ecomify GmbH - www.ecomify.de > >>>>> > >>>>> > >>>>> > |
or another approach is again to have another if condition to say if
the task is shutdown, then depend on the jar, else depend on the classes although that would be messy IMO and might increase the technical debt due to inconsistencies On Wed, Feb 26, 2020 at 6:36 PM Taher Alkhateeb <[hidden email]> wrote: > > I think I understand the reason why this is happening. Someone changed > the implementation of createOfbizCommandTask to depend on the classes > directly instead of depending on the generated Jar. I'm not sure why > but I personally do not prefer this approach. > > The solution is to revert to the original design where the classpath = > files(thejarfilehere) and change dependsOn from classes to build > > On Wed, Feb 26, 2020 at 6:18 PM Michael Brohl <[hidden email]> wrote: > > > > Hi Taher, > > > > the configuration in your patch is executed correctly, I logged it with > > this code enhancement: > > > > if (!(taskName ==~ /^ofbiz.*(--shutdown|-d).*/)) { > > println taskName + ' dependsOn classes' > > dependsOn classes > > } else { > > println taskName + ' NO dependsOn classes' > > } > > > > ./gradlew "ofbiz --shutdown" --dry-run > > > > > Configure project : > > executeLoadAdminUser dependsOn classes > > executeLoadTenant dependsOn classes > > loadTenantOnMasterTenantDb dependsOn classes > > loadTenantData dependsOn classes > > loadTenantAdminUserLogin dependsOn classes > > ofbiz --shutdown NO dependsOn classes > > :compileJava SKIPPED > > :compileGroovy SKIPPED > > :processResources SKIPPED > > :classes SKIPPED > > :ofbiz --shutdown SKIPPED > > > > but it still seems to execute the whole task dependencies: > > > > > Task :compileJava UP-TO-DATE > > > Task :compileGroovy UP-TO-DATE > > > Task :processResources UP-TO-DATE > > > Task :classes UP-TO-DATE > > > > > Task :ofbiz --shutdown > > > > > > The plugin https://github.com/dorongold/gradle-task-tree shows the > > following task dependencies: > > > > :ofbiz --shutdown > > \--- :classes > > +--- :compileGroovy > > | \--- :compileJava > > +--- :compileJava > > \--- :processResources > > > > > > Not sure why... > > > > Thanks, > > > > Michael > > > > > > Am 26.02.20 um 13:53 schrieb Taher Alkhateeb: > > > Hi Michael, > > > > > > I could be mistaken, but I think perhaps you're trying to remove the > > > dependency during the execution phase instead of the configuration > > > phase? > > > > > > Would a patch like the below not work? > > > > > > diff --git a/build.gradle b/build.gradle > > > index e5bfb3e9c9..96dcb53a01 100644 > > > --- a/build.gradle > > > +++ b/build.gradle > > > @@ -976,11 +976,14 @@ tasks.addRule('Pattern: ofbizBackground > > > <Commands>: Execute OFBiz startup comman > > > * ======================================================== */ > > > > > > def createOfbizCommandTask(taskName, arguments) { > > > - task(type: JavaExec, dependsOn: classes, taskName) { > > > + task(taskName, type: JavaExec) { > > > jvmArgs(application.applicationDefaultJvmArgs) > > > classpath = sourceSets.main.runtimeClasspath > > > main = application.mainClassName > > > args arguments > > > + if (!(taskName ==~ /^ofbiz.*(--shutdown|-d).*/)) { > > > + dependsOn classes > > > + } > > > if (taskName ==~ /^ofbiz.*(--test|-t).*/) { > > > finalizedBy(createTestReports) > > > } > > > > > > On Tue, Feb 25, 2020 at 8:48 PM Michael Brohl <[hidden email]> wrote: > > >> Hi Taher, > > >> > > >> thanks for your feedback! > > >> > > >> It seems that removing a dependency from a task is deprecated and should > > >> not be used: > > >> > > >> "Do not remove a task dependency from a Task instance. This behaviour > > >> has been deprecated and is scheduled to be removed in Gradle 6.0." > > >> > > >> Given this depracation, do you think there is another way to reduce the > > >> code added? > > >> > > >> Thanks! > > >> > > >> > > >> Am 25.02.20 um 12:14 schrieb Taher Alkhateeb: > > >>> Hi Michael, > > >>> > > >>> The idea is fine but I think we can improve the implementation. All that > > >>> you need is to remove the dependsOn declaration if it is a shutdown command > > >>> which would probably result in less code than in the proposed patch. > > >>> > > >>> Cheers > > >>> > > >>> On Tue, Feb 25, 2020, 1:32 PM Michael Brohl <[hidden email]> > > >>> wrote: > > >>> > > >>>> Oops, just realized I already asked in > > >>>> > > >>>> https://lists.apache.org/thread.html/rf88ce5eb1a3842471ddb8239bb5f08b71c107c4b0d3863b3c13e54fe%40%3Cdev.ofbiz.apache.org%3E > > >>>> > > >>>> Better respond there, thanks! > > >>>> > > >>>> Michael > > >>>> > > >>>> > > >>>> Am 25.02.20 um 11:29 schrieb Michael Brohl: > > >>>>> Hi devs, > > >>>>> > > >>>>> any objections to get this into the codebase (and backporting to > > >>>>> 17.12./18.12)? > > >>>>> > > >>>>> See https://issues.apache.org/jira/browse/OFBIZ-10168 > > >>>>> > > >>>>> Thanks for your feedback, > > >>>>> > > >>>>> Michael Brohl > > >>>>> > > >>>>> ecomify GmbH - www.ecomify.de > > >>>>> > > >>>>> > > >>>>> > > |
In reply to this post by taher
Hello,
Taher Alkhateeb <[hidden email]> writes: > I think I understand the reason why this is happening. Someone changed > the implementation of createOfbizCommandTask to depend on the classes > directly instead of depending on the generated Jar. I'm not sure why > but I personally do not prefer this approach. > > The solution is to revert to the original design where the classpath = > files(thejarfilehere) and change dependsOn from classes to build Running OFBiz via Gradle only depends on building the classes because it does not need more. Depending on stuff like :jar or :build will only slow down the recompilation process for no sound benefits. Additionally this approach matches what is done for the standard :run task. The whole idea of running a program from Gradle which is a *build* tool is to make sure that the executable is in sync with the current state of the sources which is important in a development context. As a consequence the idea of disabling this fundamental feature in presence of the --shutdown option simply does not make sense. If people want to be in charge of the compilation and execution lifecycles, they are free to use Gradle to build an artifact and run OFBiz outside of their build tool like in the following example: ``` $ ./gradlew jar $ java -jar build/libs/ofbiz & $ java -jar build/libs/ofbiz --shutDown ``` I will not try to argue with people that this is the right approach so feel free to ignore my input if it does not make sense. In any case you have my blessing for reverting what “someone” sneaked in the code base. ;-) Regards, -- Mathieu Lirzin GPG: F2A3 8D7E EB2B 6640 5761 070D 0ADE E100 9460 4D37 |
Mathieu Lirzin <[hidden email]> writes:
> If people want to be in charge of the compilation and execution > lifecycles, they are free to use Gradle to build an artifact and run > OFBiz outside of their build tool like in the following example: > > ``` > $ ./gradlew jar > $ java -jar build/libs/ofbiz & > $ java -jar build/libs/ofbiz --shutDown > ``` Oups, I forgot the “.jar” extension and misspelled --shutdown. Here are the correct commands. ``` $ ./gradlew jar $ java -jar build/libs/ofbiz.jar & $ java -jar build/libs/ofbiz.jar --shutdown ``` -- Mathieu Lirzin GPG: F2A3 8D7E EB2B 6640 5761 070D 0ADE E100 9460 4D37 |
Hello Mathieu,
I did not know that it was you who changed it nor am I implying "sneaking" anything. I'm just analyzing the reason for the dependency on classes sticking in the code base and did not check who / why out of laziness. I appreciate your understanding and hope you don't personalize issues. On another note, I don't have a strong opinion on whatever direction people prefer, I'm just happy we figured out the root cause and any solution is fine by me. Cheers, On Wed, Feb 26, 2020 at 9:14 PM Mathieu Lirzin <[hidden email]> wrote: > > Mathieu Lirzin <[hidden email]> writes: > > > If people want to be in charge of the compilation and execution > > lifecycles, they are free to use Gradle to build an artifact and run > > OFBiz outside of their build tool like in the following example: > > > > ``` > > $ ./gradlew jar > > $ java -jar build/libs/ofbiz & > > $ java -jar build/libs/ofbiz --shutDown > > ``` > > Oups, I forgot the “.jar” extension and misspelled --shutdown. Here are > the correct commands. > > ``` > $ ./gradlew jar > $ java -jar build/libs/ofbiz.jar & > $ java -jar build/libs/ofbiz.jar --shutdown > ``` > > -- > Mathieu Lirzin > GPG: F2A3 8D7E EB2B 6640 5761 070D 0ADE E100 9460 4D37 |
Free forum by Nabble | Edit this page |