Re: [Codenarc-user] Running rules which require compilerPhase > Phases.CONVERSION using Gradle's Co
Brought to you by:
chrismair
From: Marcin E. <mar...@pr...> - 2017-03-13 13:08:58
|
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 >>>>>> >>>>>> >>>>>> >>>> >>> >> > |