Re: [Codenarc-user] Running rules which require compilerPhase > Phases.CONVERSION using Gradle's CodeNarc plugin

[Marcin E. <mar...@pr...>]

That's great, thanks a lot.

Marcin

On Sat, 18 Mar 2017 at 16:16, Chris Mair <cm...@gm...> wrote:

> I have released and published *CodeNarc* v0.27.0, which includes your
> changes,
>
> Thanks.
> Chris
> On Sun, Mar 12, 2017 at 5:12 PM, Marcin Erdmann <mar...@pr...
> > wrote:
>
> Chris,
>
> I decided to get the necessary changes into CodeNarc first (
> https://github.com/CodeNarc/CodeNarc/pull/192) and then simply submit a
> PR for the Gradle side of things. Hopefully it will be small and obvious to
> the Gradle guys and get accepted without a need for much explanation from
> my side.
>
> Cheers,
> Marcin
>
> On Fri, Mar 10, 2017 at 6:13 PM, Chris Mair <cm...@gm...> wrote:
>
> Marcin,
>
> Yes, your proposal seems reasonable, and I am okay putting out a CodeNarc
> release afterward.
>
> Thanks for your in-depth analysis.
>
> Chris
>
> On Fri, Mar 10, 2017 at 9:53 AM, Marcin Erdmann <mar...@pr...
> > wrote:
>
> So I had a look and this is my current thinking:
>
> 1. Add a new *classpath* nested element and *classpathref* property to
> CodeNarc's ant task. They would be analogous to the ones with the same
> names defined for groovyc ant task (
> http://groovy-lang.org/groovyc.html#_ant_task) and thus use a way of
> defining classpaths ant users are familiar with (by being backed with
> org.apache.tools.ant.types.Path).
> 2. Use the newly added classpath definition to create a new
> *UrlClassLoader* with current context classloader as it's parent in
> *CodeNarcTask.execute()*. Set the newly created classloader as the
> context classloader around the call to *CodeNarcRunner.execute()*.
> 3. Get a new version of CodeNarc released so that the new functionality
> can be integrated with Gradle's CodeNarc plugin.
> 4. Add a new *FileCollection compilationClasspath* property to Gradle's
> CodeNarc task that will be used to configure *classpath* nested element
> of *CodeNarcTask* in *CodeNarcInvoker*. If the *compilationClasspath* property
> is not set then *classpath* nested element is not configured allowing for
> backwards compatibility with older CodeNarc versions.
>
> Another option would be to change which classloader is used when CodeNarc
> compiles analysed code to the one that was used to load its classes, but
> I'd rather not do that because:
> - it's a potentially breaking change
> - you should not be forced to use the same classpath for both the tool
> (Codenarc) and the classes you compile
>
> Chris, I would highly appreciate if you could confirm that you would be ok
> with such change and would be happy to do a release after it's merged in to
> unlock integrating the change with Gradle. Upon receiving a go ahead from
> you I will send a message to Gradle dev list asking if they are ok with my
> proposed change in point 4.
>
> Cheers,
> Marcin
>
> On Wed, Mar 8, 2017 at 7:56 AM, Marcin Erdmann <mar...@pr...>
> wrote:
>
> Thanks for your response, Chris.
>
> It's good to understand that using the context classloader when compiling
> analysed code is not a conscious design decision but just the way it works
> at the moment.
>
> I will do some more digging around on Friday which is my OSS day at work.
> I will check what the context classloader is set to when the extended rules
> are tested within CodeNarc's test suite and investigate if specifying a
> classpath to be used for the context classloader as part of CodeNarc's ant
> task is an option and how we would deal with that backwards incompatible
> change in Gradle's CodeNarc plugin.
>
> Cheers,
> Marcin
>
> On Wed, 8 Mar 2017 at 02:32, Chris Mair <cm...@gm...> wrote:
>
> Ugh. Sorry for the delayed response. I wasn't sure how to respond, and I
> was hoping that maybe someone else might have some useful input.
>
> I am not very familiar with the internals of the Gradle CodeNarc plugin,
> so I'm not sure what to suggest from that side.
>
> I am not philosophically opposed to changing the CodeNarc classloader, but
> I am nervous about unintended consequences. Support for the "enhanced"
> rules was a later addition, and presumably less used, though I'm still
> surprised no one else has complained about this.
>
> On Fri, Mar 3, 2017 at 8:39 AM, Marcin Erdmann <mar...@pr...>
> wrote:
>
> Hi,
>
> I wrote a custom rule which requires SEMANTIC_ANALYSIS compiler phase and
> I'm trying to run it using Gradle. Unfortunately I'm getting compilation
> errors upon analysis telling me that compiler cannot load any of the
> classes imported by the analysed code, even though I checked and they are
> available in the codenarc configuration of my project which is added by
> Gradle's CodeNarc plugin and used to load CodeNarc classes, see:
>
> -
> https://github.com/gradle/gradle/blob/v3.3.0/subprojects/code-quality/src/main/groovy/org/gradle/api/plugins/quality/CodeNarcPlugin.java#L72
> -
> https://github.com/gradle/gradle/blob/v3.3.0/subprojects/code-quality/src/main/groovy/org/gradle/api/plugins/quality/CodeNarcPlugin.java#L89
> -
> https://github.com/gradle/gradle/blob/v3.3.0/subprojects/code-quality/src/main/groovy/org/gradle/api/plugins/quality/CodeNarc.java#L104
> -
> https://github.com/gradle/gradle/blob/v3.3.0/subprojects/code-quality/src/main/groovy/org/gradle/api/plugins/quality/internal/CodeNarcInvoker.groovy#L45
>
> I then dug around and realised that when CodeNarc compiles analysed code
> it uses the context class loader, see:
> -
> https://github.com/CodeNarc/CodeNarc/blob/v0.26.0/src/main/groovy/org/codenarc/source/AbstractSourceCode.groovy#L88
> -
> https://github.com/groovy/groovy-core/blob/GROOVY_2_1_8/src/main/org/codehaus/groovy/control/CompilationUnit.java#L94
>
> https://github.com/groovy/groovy-core/blob/GROOVY_2_1_8/src/main/org/codehaus/groovy/control/CompilationUnit.java#L136
> -
> https://github.com/groovy/groovy-core/blob/GROOVY_2_1_8/src/main/org/codehaus/groovy/control/ProcessingUnit.java#L64
> -
> https://github.com/groovy/groovy-core/blob/GROOVY_2_1_8/src/main/org/codehaus/groovy/control/ProcessingUnit.java#L103
>
> And I finally realised that Gradle's BasicAntBuilder replaces the context
> class loader to only contain Ant classes which means that not even Groovy
> classes are available on the compilation classpath when CodeNarc compiles
> the analysed code:
> -
> https://github.com/gradle/gradle/blob/v3.3.0/subprojects/code-quality/src/main/groovy/org/gradle/api/plugins/quality/internal/CodeNarcInvoker.groovy#L45
> -
> https://github.com/gradle/gradle/blob/v3.3.0/subprojects/core/src/main/java/org/gradle/api/internal/project/antbuilder/DefaultIsolatedAntBuilder.java#L151
> -
> https://github.com/gradle/gradle/blob/v3.3.0/subprojects/core/src/main/java/org/gradle/api/internal/project/ant/BasicAntBuilder.java#L76
>
> Given the above, here are my questions:
>
> 1. Has anybody ever managed to get their projects set up correctly and get
> violations for one of the built-in CodeNarc Enhanced Classpath Rules,
> e.g. CloneWithoutCloneable? If that's the case can they please explain how
> they achieved it? If one is to believe comments in Gradle's codebase (e.g.
> https://github.com/gradle/gradle/blob/v3.3.0/subprojects/core/src/main/java/org/gradle/api/internal/project/antbuilder/DefaultIsolatedAntBuilder.java#L140)
> then it looks like a similar problem to the one occurring when using Gradle
> would occur when using Ant.
>
> 2. Is there a reason for CodeNarc to use the context class loader instead
> of the class loader that loaded CodeNarc when compiling the analysed code?
> Given that it is a breaking change, would a PR changing that class loader
> to the one which loaded CodeNarc be a valid option? It is a breaking change
> but I don't see a reason why somebody would depend on current behaviour as
> it actually prevents usage of Enhanced Classpath Rules. Or should I look
> into changing Gradle's CodeNarc plugin to set the context class loader as
> per CodeNarc's expectations?
>
> Cheers,
> Marcin
>
>
>
>
> ------------------------------------------------------------------------------
> Check out the vibrant tech community on one of the world's most
> engaging tech sites, SlashDot.org! http://sdm.link/slashdot
> _______________________________________________
> Codenarc-user mailing list
> Cod...@li...
> https://lists.sourceforge.net/lists/listinfo/codenarc-user
>
>
>
>
>
>