What shall we do about GRADLE-3116?

classic Classic list List threaded Threaded
7 messages Options
Reply | Threaded
Open this post in threaded view
|

What shall we do about GRADLE-3116?

Perryn Fowler

I did some investigation and I *think* I know whats going on with this regression. If I'm right, the way it used to work is probably not desireable anyway, so it would be good to discuss what to do about it...

What I think is going on:

GROOVY CLASSPATH INFERENCE
  • The GroovyPlugin sets the classpath of the GroovyDoc task to the compile classpath of the main sourceset
  • The GroovyBasePlugin sets the groovyClasspath by using GroovyRuntime to infer it from the (compile) classpath 
  • The inference mechanism looks for a groovy jar on the classpath
  • If it finds groovy-all.jar it just uses that jar.
  • If it finds a different groovy jar it creates a new configuration and adds the jar as a dependency (I am not sure what this achieves over just using the jar)
  • It would make more sense to me if it added a 'tool' version of the jar instead (in fact the comments say it *does* do this)
  • This behaviour seems to have been changed by Adam in commit 818e0b9
  • I suspect this commit introduced the regression

HOWEVER...

  • The exception reported in the forums does not seem to occur when executing the groovydoc tool itself, but when instantiating a gradle BasicAntBuilder (in order to define and execute an ant groovydoc task)
  • This is because BasicAntBuilder inherits from groovy.util.AntBuilder
  • This is loaded by a classloader that by default has a classpath containing classPathRegistry.getClassPath("GROOVY"), but this appears to be replaced by AntGroovyDoc with the groovyClasspath from the GroovyDoc task
  • This all leads me to suspect that this used to work (prior to Adam's commit) by picking up groovy.util.AntBuilder from the users compile configuration rather than from the gradle installation 
  • That doesn't seem desireable...
Reply | Threaded
Open this post in threaded view
|

Re: What shall we do about GRADLE-3116?

Luke Daley-2


On 9 July 2014 at 4:49:27 pm, Perryn Fowler ([hidden email]) wrote:


I did some investigation and I *think* I know whats going on with this regression. If I'm right, the way it used to work is probably not desireable anyway, so it would be good to discuss what to do about it...

What I think is going on:

GROOVY CLASSPATH INFERENCE
  • The GroovyPlugin sets the classpath of the GroovyDoc task to the compile classpath of the main sourceset
  • The GroovyBasePlugin sets the groovyClasspath by using GroovyRuntime to infer it from the (compile) classpath 
  • The inference mechanism looks for a groovy jar on the classpath
  • If it finds groovy-all.jar it just uses that jar.
  • If it finds a different groovy jar it creates a new configuration and adds the jar as a dependency (I am not sure what this achieves over just using the jar)
  • It would make more sense to me if it added a 'tool' version of the jar instead (in fact the comments say it *does* do this)
  • This behaviour seems to have been changed by Adam in commit 818e0b9
  • I suspect this commit introduced the regression

Right, here’s the culprit: https://github.com/gradle/gradle/commit/818e0b9#diff-95e522b5be7746a11b2f9d31a2f29d9dL101

HOWEVER...

  • The exception reported in the forums does not seem to occur when executing the groovydoc tool itself, but when instantiating a gradle BasicAntBuilder (in order to define and execute an ant groovydoc task)
  • This is because BasicAntBuilder inherits from groovy.util.AntBuilder
  • This is loaded by a classloader that by default has a classpath containing classPathRegistry.getClassPath("GROOVY"), but this appears to be replaced by AntGroovyDoc with the groovyClasspath from the GroovyDoc task
  • This all leads me to suspect that this used to work (prior to Adam's commit) by picking up groovy.util.AntBuilder from the users compile configuration rather than from the gradle installation 
  • That doesn't seem desireable...

We need to change GroovyRuntime to have another method that returns a classpath with the ant classes and wire that to the groovydoc task.

— 

Luke Daley
http://www.gradleware.com
Reply | Threaded
Open this post in threaded view
|

Re: What shall we do about GRADLE-3116?

Luke Daley-2


On 10 July 2014 at 11:13:22 am, Luke Daley ([hidden email]) wrote:



On 9 July 2014 at 4:49:27 pm, Perryn Fowler ([hidden email]) wrote:


I did some investigation and I *think* I know whats going on with this regression. If I'm right, the way it used to work is probably not desireable anyway, so it would be good to discuss what to do about it...

What I think is going on:

GROOVY CLASSPATH INFERENCE
  • The GroovyPlugin sets the classpath of the GroovyDoc task to the compile classpath of the main sourceset
  • The GroovyBasePlugin sets the groovyClasspath by using GroovyRuntime to infer it from the (compile) classpath 
  • The inference mechanism looks for a groovy jar on the classpath
  • If it finds groovy-all.jar it just uses that jar.
  • If it finds a different groovy jar it creates a new configuration and adds the jar as a dependency (I am not sure what this achieves over just using the jar)
  • It would make more sense to me if it added a 'tool' version of the jar instead (in fact the comments say it *does* do this)
  • This behaviour seems to have been changed by Adam in commit 818e0b9
  • I suspect this commit introduced the regression

Right, here’s the culprit: https://github.com/gradle/gradle/commit/818e0b9#diff-95e522b5be7746a11b2f9d31a2f29d9dL101

HOWEVER...

  • The exception reported in the forums does not seem to occur when executing the groovydoc tool itself, but when instantiating a gradle BasicAntBuilder (in order to define and execute an ant groovydoc task)
  • This is because BasicAntBuilder inherits from groovy.util.AntBuilder
  • This is loaded by a classloader that by default has a classpath containing classPathRegistry.getClassPath("GROOVY"), but this appears to be replaced by AntGroovyDoc with the groovyClasspath from the GroovyDoc task
  • This all leads me to suspect that this used to work (prior to Adam's commit) by picking up groovy.util.AntBuilder from the users compile configuration rather than from the gradle installation 
  • That doesn't seem desireable...

We need to change GroovyRuntime to have another method that returns a classpath with the ant classes and wire that to the groovydoc task.

Just noticed that GroovyRuntime is public API (why?). This means we should actually just revert the change to this method.


— 

Luke Daley
http://www.gradleware.com
Reply | Threaded
Open this post in threaded view
|

Re: What shall we do about GRADLE-3116?

Adam Murdoch

On 10 Jul 2014, at 11:17 am, Luke Daley <[hidden email]> wrote:



On 10 July 2014 at 11:13:22 am, Luke Daley ([hidden email]) wrote:



On 9 July 2014 at 4:49:27 pm, Perryn Fowler ([hidden email]) wrote:


I did some investigation and I *think* I know whats going on with this regression. If I'm right, the way it used to work is probably not desireable anyway, so it would be good to discuss what to do about it...

What I think is going on:

GROOVY CLASSPATH INFERENCE
  • The GroovyPlugin sets the classpath of the GroovyDoc task to the compile classpath of the main sourceset
  • The GroovyBasePlugin sets the groovyClasspath by using GroovyRuntime to infer it from the (compile) classpath 
  • The inference mechanism looks for a groovy jar on the classpath
  • If it finds groovy-all.jar it just uses that jar.
  • If it finds a different groovy jar it creates a new configuration and adds the jar as a dependency (I am not sure what this achieves over just using the jar)
  • It would make more sense to me if it added a 'tool' version of the jar instead (in fact the comments say it *does* do this)
  • This behaviour seems to have been changed by Adam in commit 818e0b9
  • I suspect this commit introduced the regression

Right, here’s the culprit: https://github.com/gradle/gradle/commit/818e0b9#diff-95e522b5be7746a11b2f9d31a2f29d9dL101

HOWEVER...

  • The exception reported in the forums does not seem to occur when executing the groovydoc tool itself, but when instantiating a gradle BasicAntBuilder (in order to define and execute an ant groovydoc task)
  • This is because BasicAntBuilder inherits from groovy.util.AntBuilder
  • This is loaded by a classloader that by default has a classpath containing classPathRegistry.getClassPath("GROOVY"), but this appears to be replaced by AntGroovyDoc with the groovyClasspath from the GroovyDoc task
  • This all leads me to suspect that this used to work (prior to Adam's commit) by picking up groovy.util.AntBuilder from the users compile configuration rather than from the gradle installation 
  • That doesn't seem desireable...

We need to change GroovyRuntime to have another method that returns a classpath with the ant classes and wire that to the groovydoc task.

Just noticed that GroovyRuntime is public API (why?). This means we should actually just revert the change to this method.


Only when generating Javadoc. We don’t want to make groovy-ant visible at compilation time if it’s not declared as a dependency.


--
Adam Murdoch
Gradle Co-founder
http://www.gradle.org
CTO Gradleware Inc. - Gradle Training, Support, Consulting
http://www.gradleware.com



Reply | Threaded
Open this post in threaded view
|

Re: What shall we do about GRADLE-3116?

Luke Daley-2
That means a breaking change as GroovyRuntime is public. Granted we did already break it for 2.0. We didn't mention it AFAICT.

Given that we got grief other than this issue it might be ok to break compatibility.


On Sat, Jul 19, 2014 at 8:11 AM, Adam Murdoch <[hidden email]> wrote:


On 10 Jul 2014, at 11:17 am, Luke Daley <[hidden email]> wrote:



On 10 July 2014 at 11:13:22 am, Luke Daley ([hidden email]) wrote:



On 9 July 2014 at 4:49:27 pm, Perryn Fowler ([hidden email]) wrote:


I did some investigation and I *think* I know whats going on with this regression. If I'm right, the way it used to work is probably not desireable anyway, so it would be good to discuss what to do about it...

What I think is going on:

GROOVY CLASSPATH INFERENCE
  • The GroovyPlugin sets the classpath of the GroovyDoc task to the compile classpath of the main sourceset
  • The GroovyBasePlugin sets the groovyClasspath by using GroovyRuntime to infer it from the (compile) classpath 
  • The inference mechanism looks for a groovy jar on the classpath
  • If it finds groovy-all.jar it just uses that jar.
  • If it finds a different groovy jar it creates a new configuration and adds the jar as a dependency (I am not sure what this achieves over just using the jar)
  • It would make more sense to me if it added a 'tool' version of the jar instead (in fact the comments say it *does* do this)
  • This behaviour seems to have been changed by Adam in commit 818e0b9
  • I suspect this commit introduced the regression

Right, here’s the culprit: https://github.com/gradle/gradle/commit/818e0b9#diff-95e522b5be7746a11b2f9d31a2f29d9dL101

HOWEVER...

  • The exception reported in the forums does not seem to occur when executing the groovydoc tool itself, but when instantiating a gradle BasicAntBuilder (in order to define and execute an ant groovydoc task)
  • This is because BasicAntBuilder inherits from groovy.util.AntBuilder
  • This is loaded by a classloader that by default has a classpath containing classPathRegistry.getClassPath("GROOVY"), but this appears to be replaced by AntGroovyDoc with the groovyClasspath from the GroovyDoc task
  • This all leads me to suspect that this used to work (prior to Adam's commit) by picking up groovy.util.AntBuilder from the users compile configuration rather than from the gradle installation 
  • That doesn't seem desireable...

We need to change GroovyRuntime to have another method that returns a classpath with the ant classes and wire that to the groovydoc task.

Just noticed that GroovyRuntime is public API (why?). This means we should actually just revert the change to this method.


Only when generating Javadoc. We don’t want to make groovy-ant visible at compilation time if it’s not declared as a dependency.


--
Adam Murdoch
Gradle Co-founder
http://www.gradle.org
CTO Gradleware Inc. - Gradle Training, Support, Consulting
http://www.gradleware.com




Reply | Threaded
Open this post in threaded view
|

Re: What shall we do about GRADLE-3116?

Adam Murdoch

On 19 Jul 2014, at 8:57 am, Luke Daley <[hidden email]> wrote:

That means a breaking change as GroovyRuntime is public. Granted we did already break it for 2.0. We didn't mention it AFAICT.

GroovyRuntime is incubating, and due for an overhaul when we add the new groovy plugin, so I’m not too concerned about changing it. If we wanted, we would replace (via deprecation) the existing method with two other methods - one to calculate the groovy compiler classpath and one to calculate the ant based groovydoc tool classpath.



Given that we got grief other than this issue it might be ok to break compatibility.


On Sat, Jul 19, 2014 at 8:11 AM, Adam Murdoch <[hidden email]> wrote:


On 10 Jul 2014, at 11:17 am, Luke Daley <[hidden email]> wrote:



On 10 July 2014 at 11:13:22 am, Luke Daley ([hidden email]) wrote:



On 9 July 2014 at 4:49:27 pm, Perryn Fowler ([hidden email]) wrote:


I did some investigation and I *think* I know whats going on with this regression. If I'm right, the way it used to work is probably not desireable anyway, so it would be good to discuss what to do about it...
What I think is going on:

GROOVY CLASSPATH INFERENCE
  • The GroovyPlugin sets the classpath of the GroovyDoc task to the compile classpath of the main sourceset
  • The GroovyBasePlugin sets the groovyClasspath by using GroovyRuntime to infer it from the (compile) classpath 
  • The inference mechanism looks for a groovy jar on the classpath
  • If it finds groovy-all.jar it just uses that jar.
  • If it finds a different groovy jar it creates a new configuration and adds the jar as a dependency (I am not sure what this achieves over just using the jar)
  • It would make more sense to me if it added a 'tool' version of the jar instead (in fact the comments say it *does* do this)
  • This behaviour seems to have been changed by Adam in commit 818e0b9
  • I suspect this commit introduced the regression

Right, here’s the culprit: https://github.com/gradle/gradle/commit/818e0b9#diff-95e522b5be7746a11b2f9d31a2f29d9dL101

HOWEVER...

  • The exception reported in the forums does not seem to occur when executing the groovydoc tool itself, but when instantiating a gradle BasicAntBuilder (in order to define and execute an ant groovydoc task)
  • This is because BasicAntBuilder inherits from groovy.util.AntBuilder
  • This is loaded by a classloader that by default has a classpath containing classPathRegistry.getClassPath("GROOVY"), but this appears to be replaced by AntGroovyDoc with the groovyClasspath from the GroovyDoc task
  • This all leads me to suspect that this used to work (prior to Adam's commit) by picking up groovy.util.AntBuilder from the users compile configuration rather than from the gradle installation 
  • That doesn't seem desireable...

We need to change GroovyRuntime to have another method that returns a classpath with the ant classes and wire that to the groovydoc task.

Just noticed that GroovyRuntime is public API (why?). This means we should actually just revert the change to this method.


Only when generating Javadoc. We don’t want to make groovy-ant visible at compilation time if it’s not declared as a dependency.


--
Adam Murdoch
Gradle Co-founder
http://www.gradle.org
CTO Gradleware Inc. - Gradle Training, Support, Consulting
http://www.gradleware.com






--
Adam Murdoch
Gradle Co-founder
http://www.gradle.org
CTO Gradleware Inc. - Gradle Training, Support, Consulting
http://www.gradleware.com



Reply | Threaded
Open this post in threaded view
|

Re: What shall we do about GRADLE-3116?

Luke Daley-2


On 23 July 2014 at 8:07:00 am, Adam Murdoch ([hidden email]) wrote:


On 19 Jul 2014, at 8:57 am, Luke Daley <[hidden email]> wrote:

That means a breaking change as GroovyRuntime is public. Granted we did already break it for 2.0. We didn't mention it AFAICT.

GroovyRuntime is incubating, and due for an overhaul when we add the new groovy plugin, so I’m not too concerned about changing it. If we wanted, we would replace (via deprecation) the existing method with two other methods - one to calculate the groovy compiler classpath and one to calculate the ant based groovydoc tool classpath.

I missed that it was incubating.






Given that we got grief other than this issue it might be ok to break compatibility.


On Sat, Jul 19, 2014 at 8:11 AM, Adam Murdoch <[hidden email]> wrote:


On 10 Jul 2014, at 11:17 am, Luke Daley <[hidden email]> wrote:



On 10 July 2014 at 11:13:22 am, Luke Daley ([hidden email]) wrote:



On 9 July 2014 at 4:49:27 pm, Perryn Fowler ([hidden email]) wrote:


I did some investigation and I *think* I know whats going on with this regression. If I'm right, the way it used to work is probably not desireable anyway, so it would be good to discuss what to do about it...
What I think is going on:

GROOVY CLASSPATH INFERENCE
  • The GroovyPlugin sets the classpath of the GroovyDoc task to the compile classpath of the main sourceset
  • The GroovyBasePlugin sets the groovyClasspath by using GroovyRuntime to infer it from the (compile) classpath 
  • The inference mechanism looks for a groovy jar on the classpath
  • If it finds groovy-all.jar it just uses that jar.
  • If it finds a different groovy jar it creates a new configuration and adds the jar as a dependency (I am not sure what this achieves over just using the jar)
  • It would make more sense to me if it added a 'tool' version of the jar instead (in fact the comments say it *does* do this)
  • This behaviour seems to have been changed by Adam in commit 818e0b9
  • I suspect this commit introduced the regression

Right, here’s the culprit: https://github.com/gradle/gradle/commit/818e0b9#diff-95e522b5be7746a11b2f9d31a2f29d9dL101

HOWEVER...

  • The exception reported in the forums does not seem to occur when executing the groovydoc tool itself, but when instantiating a gradle BasicAntBuilder (in order to define and execute an ant groovydoc task)
  • This is because BasicAntBuilder inherits from groovy.util.AntBuilder
  • This is loaded by a classloader that by default has a classpath containing classPathRegistry.getClassPath("GROOVY"), but this appears to be replaced by AntGroovyDoc with the groovyClasspath from the GroovyDoc task
  • This all leads me to suspect that this used to work (prior to Adam's commit) by picking up groovy.util.AntBuilder from the users compile configuration rather than from the gradle installation 
  • That doesn't seem desireable...

We need to change GroovyRuntime to have another method that returns a classpath with the ant classes and wire that to the groovydoc task.

Just noticed that GroovyRuntime is public API (why?). This means we should actually just revert the change to this method.


Only when generating Javadoc. We don’t want to make groovy-ant visible at compilation time if it’s not declared as a dependency.


--
Adam Murdoch
Gradle Co-founder
http://www.gradle.org
CTO Gradleware Inc. - Gradle Training, Support, Consulting
http://www.gradleware.com






--
Adam Murdoch
Gradle Co-founder
http://www.gradle.org
CTO Gradleware Inc. - Gradle Training, Support, Consulting
http://www.gradleware.com