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

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

Thanks, Chris, that was swift.

There's no real rush with the release, I won't be able to work on the
Gradle PR before Friday anyway but I will indeed need you to make a release
to unblock me and enable me to work on it.

Cheers,
Marcin

On Sun, Mar 12, 2017 at 11:29 PM, Chris Mair <cm...@gm...> wrote:

> Marcin,
>
> I have accepted the pull request. Thanks for the contribution.
>
> Are you now wanting me to do a CodeNarc release with this ASAP?
>
> 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/groovy
>>>> c.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.Pat
>>>> h).
>>>> 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/cod
>>>>>> e-quality/src/main/groovy/org/gradle/api/plugins/quality/Cod
>>>>>> eNarcPlugin.java#L72
>>>>>> - https://github.com/gradle/gradle/blob/v3.3.0/subprojects/c
>>>>>> ode-quality/src/main/groovy/org/gradle/api/plugins/quality/C
>>>>>> odeNarcPlugin.java#L89
>>>>>> - https://github.com/gradle/gradle/blob/v3.3.0/subprojects/c
>>>>>> ode-quality/src/main/groovy/org/gradle/api/plugins/quality/C
>>>>>> odeNarc.java#L104
>>>>>> - https://github.com/gradle/gradle/blob/v3.3.0/subprojects/c
>>>>>> ode-quality/src/main/groovy/org/gradle/api/plugins/quality/i
>>>>>> nternal/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/g
>>>>>> roovy/org/codenarc/source/AbstractSourceCode.groovy#L88
>>>>>> - https://github.com/groovy/groovy-core/blob/GROOVY_2_1_8/sr
>>>>>> c/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/sr
>>>>>> c/main/org/codehaus/groovy/control/ProcessingUnit.java#L64
>>>>>> - https://github.com/groovy/groovy-core/blob/GROOVY_2_1_8/sr
>>>>>> c/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/c
>>>>>> ode-quality/src/main/groovy/org/gradle/api/plugins/quality/i
>>>>>> nternal/CodeNarcInvoker.groovy#L45
>>>>>> - https://github.com/gradle/gradle/blob/v3.3.0/subprojects/c
>>>>>> ore/src/main/java/org/gradle/api/internal/project/antbuilder
>>>>>> /DefaultIsolatedAntBuilder.java#L151
>>>>>> - https://github.com/gradle/gradle/blob/v3.3.0/subprojects/c
>>>>>> ore/src/main/java/org/gradle/api/internal/project/ant/BasicA
>>>>>> ntBuilder.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/subprojec
>>>>>> ts/core/src/main/java/org/gradle/api/internal/project/antbui
>>>>>> lder/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
>>>>>>
>>>>>>
>>>>>>
>>>>
>>>
>>
>