Extract resolved includes logic into a method for input tracking

[sorin-florea]

Extracting resolved includes into a method will allow this plugin to properly work with the Gradle Enterprise Maven extension.

[sorin-florea]

@slachiewicz do you think you can have a look over this PR?

[slachiewicz]

Looks safe. Why extraction to separate method would help this plugin?

[sorin-florea]

Resolved inputs can now be tracked as input and the goal execution can now be cached.

[kriegaex]

@sorin-florea, to me this does not make sense. You simply factored out a code snippet into a private method which is only called from one place. What exactly changes in the plugin behaviour like this? What gets cached that didn't before? Is some local change of yours missing in this PR? I wonder why it was merged.

[sorin-florea]

@kriegaex the goal of the PR is not to change the behavior of the plugin. With this change resolvedIncludes can be computed before execution, and as such the inputs of the goal can be determined and the goal can be cached by the Gradle Enterprise Maven extension.

[kriegaex]

Sorry, I still don't get it. If you inline the extracted method or not, the behaviour is 100% the same, the way I understand your pretty trivial PR. The code is executed exactly at the same time as before, is it not? What would Gradle Enterprise care about a new private method?

[sorin-florea]

@kriegaex, Gradle Enterprise Maven extension allows users to configure what is an input for a goal execution. So extracting this method will effectively allow users to declare resolvedIncludes as an input and then the get method will be used by the Gradle Enterprise extension to get the value before the execution.

[kriegaex]

@sorin-florea, please do not misunderstand me. It is not so much that I do not believe you, rather that I do not understand, because you do not explain anything, but merely make claims.

So I am asking again: How would a private method which does not cache anything help you - or Gradle, for that matter? Please explain comprehensively, and ideally also point to a documentation resource. What kind of programming is that supposed to be anyway, if Gradle would rely on the existence of a private getter method which is not part of the public API of this plugin? Programming by sacred knowledge seems like a design and maintenance nightmare to me. Your PR did not even include any javadoc or at least comment on that private method, explaining why it exists and is not to be touched.

If I was a maintainer here, I would never merge a PR like this. I am really trying to understand your reasoning, please do not make me beg for information one more time.

[sorin-florea]

@kriegaex The basic concepts are explained here, especially the short inputs section. The problem with Maven is that, unlike Gradle, it does not track inputs, so having a build cache solution is a rather complex topic requiring some compromises. As the Maven team puts it: The challenge of implementing build cache in Maven is that domain model is overly generic and doesn't have dedicated API for build inputs.

Now, if we think about resolvedIncludes, a change to the resolved files would probably lead to a new build result, hence we can consider this as input. However, to be able to cache anything, you need to know the inputs before executing, but in the current implementation, resolvedIncludes is computed at execution time.

So to answer your questions:

How would a private method which does not cache anything help you

The method can be called through reflection. Of course, having a dedicated API for build inputs would be preferable. I could have made the method protected like the field, however, that would introduce more meaning than was my intention.

Your PR did not even include any Javadoc or at least comment on that private method, explaining why it exists and is not to be touched.

The method is innocuous enough and encapsulates the logic of computing the resolvedIncludes. This does not mean the method must not be touched, and it is not my intention to force other plugins to do things in a certain way just so the plugin works nicely with the Gradle Enterprise Build Cache. There is already a way to make the plugin cacheable, but it does require users to keep multiple configurations in sync (this and this. Having the logic that computes the input files extracted in a method makes the work of users that want to make and keep their builds cacheable, easier.

if Gradle would rely on the existence of a private getter method that is not part of the public API of this plugin? Programming by sacred knowledge seems like a design and maintenance nightmare to me

As mentioned above, since Maven was not designed with caching in mind, some basic understanding of what a goal does and its internals is required to achieve fine-grained caching. I agree that this solution is not perfect, and resolvedIncludes should be computed before execution time as it relies only on inputs, I think I will update this PR with a better solution. And btw, the Gradle Enterprise maven extension local cache can now be used without requiring a Gradle Enterprise license, so if you wish, you can give it a try and see it in action.

[kriegaex]

@kriegaex The basic concepts are explained here, especially the short inputs section.

I am not challenging the usefulness of what you want to achieve, only the way in which you do. Now that the PR is still unmerged, you could still improve it.

How would a private method which does not cache anything help you

The method can be called through reflection.

😱 That was exactly what I thought you would be doing.

Of course, having a dedicated API for build inputs would be preferable. I could have made the method protected like the field, however, that would introduce more meaning than was my intention.

Are you extending AbstractAjcCompiler or any of its subclasses in your use case? Then a protected method would suffice. Otherwise, something public would be the way to go. In either case, the method should have corresponding Javadoc, explaining the use case, maybe even mentioning Gradle Enterprise Build Cache as an example. Transparency is king!

Your PR did not even include any Javadoc or at least comment on that private method, explaining why it exists and is not to be touched.

The method is innocuous enough and encapsulates the logic of computing the resolvedIncludes.

Well, that is just your opinion, and I totally disagree. If that is too much work for you, maybe you should not be surprised that it has not been merged yet. I am asking not just on behalf of the Mojohaus version but also, because if done correctly, I would pull the same change into the aspectj.dev fork I am maintaining, in order to offer the same API there, if it helps Gradle users. But I wish for it to be engineered just a little bit nicer.

This does not mean the method must not be touched, and it is not my intention to force other plugins to do things in a certain way just so the plugin works nicely with the Gradle Enterprise Build Cache.

Of course it means that it must not be touched, because otherwise your use case breaks again. That way, you would be shooting your own foot. Then it would not have been worth the trouble to create, comment on and merge a PR in the first place, and you would need the next one. After all you wrote, would it not have been quicker to just re-engineer your PR a little bit and add Javadoc? It would not be a big deal. I think, it is a fair use case and the maintainers both at Mojohaus and at aspectj.dev would be glad to support it.