Re: [Codenarc-user] Running rules which require compilerPhase > Phases.CONVERSION using Gradle's Co
Brought to you by:
chrismair
From: Chris M. <cm...@gm...> - 2017-03-12 23:29:43
|
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 >>>>> >>>>> >>>>> >>> >> > |