Crimix / ChangedProjectsTaskPlugin

A Gradle plugin to run a user defined task on changed projects (modules) and their dependent projects (modules)
https://plugins.gradle.org/plugin/io.github.crimix.changed-projects-task
MIT License
8 stars 2 forks source link

Target task is passed through all dependency modules and executed separately #9

Closed mklueh closed 1 year ago

mklueh commented 1 year ago

Hi,

after a while I'm again trying to work with this plugin :) something is going wrong and I can't understand, why it is happening, so I thought you may know the reason:

If I build my application with 3 module dependencies, where some of those also have module dependencies, it seems that the task I specify is called for each one individually.

I have ensured that this part

 affectedTask.dependsOn(targetTask);
 targetTask.onlyIf(t -> {})

is really only true for the actual application, not any other package.

In my case it's a Quarkus application I'd like to deploy as Docker images. If I run it with affected, it will create one Docker image for each application and also for each dependency module.

Do you have any clue why that might happen?

Crimix commented 1 year ago

Hi

Are you able to create a minimal dummy example Gradle project to showcase the error? (it can use a hello world task)

The plugin should only ever execute the task on a project if that project depends on another affected project.

You can test if it is indeed the plugin directly by using the neverRunProject option and adding the module dependencies to that option which should stop the plugin from calling the task you defined on those

mklueh commented 1 year ago

I've made a monorepo that includes your plugin, as well as my for - I initially wasn't sure if the changes of my fork were the problem, but I've got the same behaviour with yours as well.

After some searching I found this entry which kind of describes what I'm facing - despite the task being skipped, there are still builds for everything running https://discuss.gradle.org/t/onlyif-affect-only-task-but-not-tasks-that-depends-on/14887

The changes of my fork are in essence just refactorings and renamings to adjust the API a little bit, and I've places some different logs here and there to track the problem down. I've also added another extension / configuration that - so my idea - should allow on a sub-project level to enable or disable changetracking by using the affectedProject{} configuration which is nowhere used in the project.

This is the first condition in the .onlyIf and should prevent any task execution

image

which is also implied by what I can see in the logs

image

However, this will still trigger a full build and even worse, a full build of every module.

Not sure if I just do something wrong with the configuration.

Generally my main test case was modifying the grouped-module, committing and then running either the affected plugin or the ChangedProjectsTaskPlugin and both have the "ONLY_DIRECTLY" mode on, so I assume it should detect dependent changes but only build the application modules.

I've also experimented a bit with the other options, but of onlyIf does not what it should (my impression) all the other stuff is not relevant for the problem.

https://github.com/mklueh/quarkus-affected-build-reproducer

Thanks in advance and I hope you can help me :)

mklueh commented 1 year ago

I've also integrated and tried https://github.com/ismaeldivita/change-tracker-plugin with the reproducer but it has the exact same behavior

Crimix commented 1 year ago

I will take a look at it, but it sounds more like a Gradle issue

mklueh commented 1 year ago

Indeed.

image

Sounds like there is no way but to find all related tasks of those who should not run and stop them too

https://stackoverflow.com/a/18877504/2324388

@Crimix I think the problem might be, the evaluation of the task determining if it should run or not happens during the execution phase (with .onlyIf). However, the dependent tasks might have been executed already at that point.

Crimix commented 1 year ago

I have tested a bit on your repo, so the issue is that the onlyIf only determines if the specific task to run should run, not if tasks that it depends should run. As the build task is just an umbrella for othere tasks, they get ecxecuted.

What this means is that the other dependent tasks gets excecuted and create output but the task to run does nothing, this can be seen in the Gradle console, where the other tasks infront of build does not say SKIPPED but build does.

I have a locally tweaked version where the onlyIf gets applied to all tasks that the task to run depends on and that they depend on. The issue then becomes that if there is no changes to a component that an application depends on then it does not get build and thus the application fails to build a jar as the component's jar is missing.

I think I might have a possible workaround, I noticed that executing the build using commandline against the specific module does what you want, thus I have made a modification on a dev branch that can switch it to be commandline instead of 100% Gradle based. You can check it out here branch and maybe do some test on your own, I would still say that it might not be the best idea to it this way. Also disclaimer it is not setup to handle all scenarios yet and does not support always and never run options yet

Crimix commented 1 year ago

Update, I have worked some more on the branch, it should now support the same scenarios as normal and respect the configured never and always run projects. You activate it on my branch using -PchangedProjectsTask.isCommandLine

Please let me know if this could solve your issue

mklueh commented 1 year ago

@Crimix thank you, that seems to be a good workaround!

One thing however does not work yet: I thing cli params are not propagated to the wrapper call. For example I'm passing -Dquarkus.container-image.build=true to trigger a Docker build, but that won't happen anymore.

And maybe the flag could have another name to make it more telling, but that's just my personal taste :)

Maybe it also could be used by default even without any flags?

Crimix commented 1 year ago

Good to hear.

It makes sense that the extra parameters do not get propagated, as the task is running tasks without parameters as it can be considered a "hacky" workaround. I might have an idea how to overcome the propagation issue.

But the above reason is also why it will not be the default behavior. Also I will be renaming the parameter 😀

mklueh commented 1 year ago

@Crimix not entirely sure what you mean. If you'd pass everything except the ChangedProjectsTask properties to the gradlew call, it shouldn be different than calling gradle directly with the properties, or am I wrong?

Btw: would it be possible to do all the work during evaluation rather than execution and only then walk trough all the affected projects and explicitly make the dependsOn call only for those affected ones, rather than doing it for all and later disabling the unaffected projects?

Crimix commented 1 year ago

You can't declare depends on during the execution phase, it must be done in the configuration phase, that is why all these plugins use onlyIf to disable the task it should not run. And you can't use any configuration before execution phase

Crimix commented 1 year ago

I have pushed a change to be able to pass parameters, the issue is that there are many project and system properties and I think it is too much to try an set them all.

Also I have changed the name of the parameter, now if you want to test it you can use runTaskForChangedProjects -PchangedProjectsTask.runCommandLine -PchangedProjectsTask.taskToRun=build -PchangedProjectsTask.commandLineArgs="-Dquarkus.container-image.build=true".

Let me know if this is better and works for you now

mklueh commented 1 year ago

@Crimix thanks, looks good :)

However, I'm now running in the problem, that if only grouped-component is changed, this package will get built as a Docker container, and not both apps.

Btw, how do you personally use this plugin? When and why do you use alwaysRunProjects for example?

Crimix commented 1 year ago

Glad to here it,

If you are using the same configuration as in the reproducere, then I can explain it. This changedProjectsMode = "ONLY_DIRECTLY" means as I wrote in the readme "ONLY_DIRECTLY causes the taskToRun to only be executed for projects that are changed and only those." So you want the default instead.

So personally I use this plugin to run unit tests on multi module project, where one of them is to always run architectural tests if any other module is changed, that is where I use alwaysRunProject

mklueh commented 1 year ago

That might be the case, I'll try it later :) On the other hand, how would it then know that grouped-dependency is not a buildable project as well?

To me it's a bit confusing sometimes, as there is no clear separation between what is allowed to trigger a task and what is allowed to execute a task.

For me personally, I'd only be interested in projects allowed to execute / be extecuted. Then I'd specify my 2-3 apps and would not care about all the library modules - they all would be allowed to trigger the application builds when there are changes, and I'd in addition just define root build.gradle and gradle.properties to affect all modules and trigger a build of everything.

alwaysRunProject - is it intended to really always run whenever this plugin is triggered, or only whenever there are changes anywhere, even in unrelated projects?

How would you configure the plugin in my case?

mklueh commented 1 year ago

@Crimix I've tested it now with "INCLUDE_DEPENDENTS" and it now builds both apps.

I think the way to go is just iterating through all libraries and adding it to

neverRunProject = [":components:group:grouped-component"]

Thank you :)

Crimix commented 1 year ago

alwaysRunProjectwill only trigger if there are any not ignored changes

Sounds like the right approach to make sure that they are able to trigger a build but not being built themselves.

Looks like I just need to clean up the code in the branch and publish that version for all to use. I will close this issue when it has been merged