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-10 18:13:28
|
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/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/ >>> 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/subprojec >>> ts/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 >>> >>> >>> > |