I reached out to security inquiring if our work around would be
acceptable.  That will help guide us on the priority of the groovy
changes.

I haven't seen anyone object to merging the snapshot changes and we can
easily reverse them if needed.  I intend to merge these later today unless
there are objections.

-James

On Mon, May 5, 2025 at 9:08 PM James Daugherty <jdaughe...@jdresources.net>
wrote:

> To go back to security fully, I think we'll need to make a script that
> downloads the jars, builds locally, and then compares.  Currently the
> script locally just builds twice and compares.
>
> As a starter, I'll ask security if they're ok with diffing via the
> decompile approach we've been taking.
>
> On Mon, May 5, 2025 at 2:24 PM James Fredley <jamesfred...@apache.org>
> wrote:
>
>> I think we should proceed with updating the ASF security team on the
>> progress so we receive updated feedback.
>>
>> Since we are working on 7.0.0-SNAPSHOT, will most likely need Groovy
>> 4.0.27 for Grails 7.0.0-M4 and already put the Apache Snapshot Repository
>> in the right locations to pull Grails SNAPSHOTS and Groovy SNAPSHOTS, I'd
>> be OK with Groovy 4.0.27-SNAPSHOT for the entire build for now and also OK
>> just for documentation.   We ran into issues running an earlier Groovy
>> SNAPSHOT last year, but the root causes for those issues are addressed in
>> grails-core.
>>
>> I also think it would be fine to merge and then start a new PR once
>> addition updates can be made.
>>
>> On 2025/05/05 16:11:22 James Daugherty wrote:
>> > Paul was able to fix the groovydoc differences via groovy
>> 4.0.27-SNAPSHOT.
>> > I've updated the ticket; we're down to these jars differing:
>> >
>> >       grails-cache/build/libs/grails-cache-7.0.0-SNAPSHOT.jar
>> >
>>  grails-datamapping-tck/build/libs/grails-datamapping-tck-7.0.0-SNAPSHOT.jar
>> >       grails-fields/build/libs/grails-fields-7.0.0-SNAPSHOT.jar
>> >
>>  grails-gsp/grails-sitemesh3/build/libs/grails-sitemesh3-7.0.0-SNAPSHOT.jar
>> >
>>  
>> grails-gsp/grails-web-gsp-taglib/build/libs/grails-web-gsp-taglib-7.0.0-SNAPSHOT.jar
>> >       grails-gsp/plugin/build/libs/grails-gsp-7.0.0-SNAPSHOT.jar
>> >
>>  grails-rest-transforms/build/libs/grails-rest-transforms-7.0.0-SNAPSHOT.jar
>> >
>>  grails-scaffolding/build/libs/grails-scaffolding-7.0.0-SNAPSHOT.jar
>> >       grails-shell-cli/build/libs/grails-shell-cli-7.0.0-SNAPSHOT.jar
>> >       grails-views-gson/build/libs/grails-views-gson-7.0.0-SNAPSHOT.jar
>> >
>>  
>> grails-views-markup/build/libs/grails-views-markup-7.0.0-SNAPSHOT-javadoc.jar
>> >
>>  grails-views-markup/build/libs/grails-views-markup-7.0.0-SNAPSHOT.jar
>> >
>>  
>> grails-web-url-mappings/build/libs/grails-web-url-mappings-7.0.0-SNAPSHOT.jar
>> >
>>  grails-wrapper-impl/build/libs/grails-wrapper-impl-7.0.0-SNAPSHOT.jar
>> >
>> > My latest PR is here:
>> https://github.com/apache/grails-core/issues/14679 It
>> > uses the 4.0.27 groovy snasphot to fix the javadoc issue.  Instead of
>> just
>> > assigning 4.0.27 for documentation generation, I set it for the entire
>> > project default.  This PR also fixes the comparison of the jars in the
>> > grails-gradle project & addresses some documentation issues (one
>> example:
>> > grails data docs aren't fully being generated).
>> >
>> > Of these jars, the differences look due to fields reordering from traits
>> > (fields have annotations like @TraitBridge on them).  I'm guessing
>> there's
>> > some issue related to how groovy processes the traits, but this could be
>> > something we're doing (I haven't figured out the specifics, but
>> > ApplicationTagLib is the easiest case of this since none of our AST
>> > transforms appear to do anything to that file).  I am under the
>> impression
>> > that Paul is continuing to look at this issue to see if he can help or
>> > identify a groovy specific issue.
>> >
>> > My current dilemma is if I should merge this PR.  I could scale it back
>> to
>> > use groovy 4.0.26 for compiling & only use the snapshot for
>> documentation
>> > generation.  The problem is if this is a groovy issue, we'll need a
>> groovy
>> > 4.0.27 release anyhow.  What are people's thoughts here?  Should I just
>> > leave this on a side branch for now?  Should we depend entirely on the
>> > snapshot build?  Should only documentation depend on the snapshot build?
>> >
>> > Alternatively, I think we can respond to ASF security that we can verify
>> > the build artifacts by using the same process we use to test
>> reproducible
>> > builds.  Namely, we can diff any artifact that has a different checksum
>> via
>> > the decompiler in IntelliJ to confirm the code is the same.  This
>> solution
>> > isn't a permanent solution, but for the Milestone / RC builds, I think
>> it's
>> > acceptable.
>> >
>> > -James
>> >
>> > On Mon, Apr 28, 2025 at 10:15 PM James Daugherty <
>> jdaughe...@jdresources.net>
>> > wrote:
>> >
>> > > Hi Everyone,
>> > >
>> > > I summarized my progress thus far under this ticket:
>> > > https://github.com/apache/grails-core/issues/14679
>> > >
>> > > I checked in several scripts (see ticket) that assist in easily
>> testing
>> > > the jar files for differences.
>> > >
>> > > From what I can tell, there is an ordering issue when multiple,
>> inherited
>> > > traits are involved.  I've found a simplified scenario in the
>> grails-gsp
>> > > plugin (ApplicationTagLib) that doesn't appear to be transformed by
>> our
>> > > code in a material way, and yet the difference always deals with the
>> copied
>> > > trait methods being in a random order.  I'm at a loss for how to
>> further
>> > > debug this issue.  From what I can tell, Grails isn't copying those
>> > > methods.  I would assume Groovy is.
>> > >
>> > > The steps to reproduce this:
>> > > 1. etc/bin/test-reproducible-builds.sh
>> > > 2. etc/bin/extract-build-artifact.sh
>> > > grails-gsp/plugin/build/libs/grails-gsp-7.0.0-SNAPSHOT.jar
>> > > 3. select `firstArtifact` and `secondArtifact` in IntelliJ under
>> > > `etc/bin/results` and right click to select `Compare Directories`
>> > >
>> > > I intend to send an email to the groovy dev list to see if they can
>> help
>> > > me understand how I can debug this further.
>> > >
>> > > If we can't resolve these issues fully in Grails 7, the good news is
>> the
>> > > scripts I've checked in can be used to "verify" that the jars were
>> built
>> > > correctly - we can build locally and manually diff the artifacts to
>> confirm
>> > > they're correct.  This is the work around I plan to suggest to ASF
>> security.
>> > >
>> > > -James
>> > >
>> > >
>> > >
>> > >
>> > > On Mon, Apr 28, 2025 at 11:58 AM James Daugherty <
>> > > jdaughe...@jdresources.net> wrote:
>> > >
>> > >> Thank you Jochen.  I believe I've switched to order stable
>> collections
>> > >> throughout the AST code base.  Unfortunately, I'm still seeing
>> issues.
>> > >>
>> > >> I've narrowed the differences down to the following:
>> > >>
>> grails-async/core/build/libs/grails-async-core-7.0.0-SNAPSHOT-javadoc.jar
>> > >>
>> grails-bootstrap/build/libs/grails-bootstrap-7.0.0-SNAPSHOT-javadoc.jar
>> > >> grails-cache/build/libs/grails-cache-7.0.0-SNAPSHOT.jar
>> > >>
>> grails-converters/build/libs/grails-converters-7.0.0-SNAPSHOT-javadoc.jar
>> > >> grails-core/build/libs/grails-core-7.0.0-SNAPSHOT-javadoc.jar
>> > >>
>> > >>
>> grails-data-hibernate5/core/build/libs/grails-data-hibernate5-core-7.0.0-SNAPSHOT-javadoc.jar
>> > >>
>> > >>
>> grails-data-hibernate5/dbmigration/build/libs/grails-data-hibernate5-dbmigration-7.0.0-SNAPSHOT-javadoc.jar
>> > >>
>> > >>
>> grails-datamapping-core/build/libs/grails-datamapping-core-7.0.0-SNAPSHOT-javadoc.jar
>> > >>
>> > >>
>> grails-datamapping-tck-domains/build/libs/grails-datamapping-tck-domains-7.0.0-SNAPSHOT.jar
>> > >>
>> > >>
>> grails-datamapping-tck-tests/build/libs/grails-datamapping-tck-tests-7.0.0-SNAPSHOT.jar
>> > >>
>> > >>
>> grails-datamapping-validation/build/libs/grails-datamapping-validation-7.0.0-SNAPSHOT-javadoc.jar
>> > >>
>> > >>
>> grails-datastore-core/build/libs/grails-datastore-core-7.0.0-SNAPSHOT-javadoc.jar
>> > >> grails-fields/build/libs/grails-fields-7.0.0-SNAPSHOT.jar
>> > >> grails-gsp/core/build/libs/grails-gsp-core-7.0.0-SNAPSHOT-javadoc.jar
>> > >>
>> grails-gsp/grails-sitemesh3/build/libs/grails-sitemesh3-7.0.0-SNAPSHOT.jar
>> > >>
>> > >>
>> grails-gsp/grails-taglib/build/libs/grails-taglib-7.0.0-SNAPSHOT-javadoc.jar
>> > >>
>> > >>
>> grails-gsp/grails-web-gsp-taglib/build/libs/grails-web-gsp-taglib-7.0.0-SNAPSHOT.jar
>> > >>
>> > >>
>> grails-gsp/grails-web-gsp/build/libs/grails-web-gsp-7.0.0-SNAPSHOT-javadoc.jar
>> > >> grails-gsp/plugin/build/libs/grails-gsp-7.0.0-SNAPSHOT.jar
>> > >>
>> > >>
>> grails-rest-transforms/build/libs/grails-rest-transforms-7.0.0-SNAPSHOT.jar
>> > >> grails-scaffolding/build/libs/grails-scaffolding-7.0.0-SNAPSHOT.jar
>> > >>
>> grails-shell-cli/build/libs/grails-shell-cli-7.0.0-SNAPSHOT-javadoc.jar
>> > >> grails-shell-cli/build/libs/grails-shell-cli-7.0.0-SNAPSHOT.jar
>> > >>
>> grails-test-core/build/libs/grails-test-core-7.0.0-SNAPSHOT-javadoc.jar
>> > >>
>> grails-views-core/build/libs/grails-views-core-7.0.0-SNAPSHOT-javadoc.jar
>> > >> grails-views-gson/build/libs/grails-views-gson-7.0.0-SNAPSHOT.jar
>> > >>
>> > >>
>> grails-views-markup/build/libs/grails-views-markup-7.0.0-SNAPSHOT-javadoc.jar
>> > >> grails-views-markup/build/libs/grails-views-markup-7.0.0-SNAPSHOT.jar
>> > >>
>> grails-web-common/build/libs/grails-web-common-7.0.0-SNAPSHOT-javadoc.jar
>> > >>
>> > >>
>> grails-web-url-mappings/build/libs/grails-web-url-mappings-7.0.0-SNAPSHOT-javadoc.jar
>> > >>
>> > >>
>> grails-web-url-mappings/build/libs/grails-web-url-mappings-7.0.0-SNAPSHOT.jar
>> > >>
>> > >> Since the javadoc jars now make up 13 of the remaining 26 jar
>> > >> differences, I was hoping someone can help here:
>> > >>
>> > >> The javadoc jars above are interesting because the source files &
>> > >> compiled classes no longer differ for projects like `grails-core`.
>> These
>> > >> jars actually contain the groovydoc (for java + groovy classes).  In
>> the
>> > >> case of `grails-core`, the example difference is the
>> > >> groovy.lang.Script#getBinding() and groovy.lang.Script#setBinding()
>> methods
>> > >> are reordered.
>> > >> Since this is groovydoc generating this, do you know if there are
>> known
>> > >> defined orders in groovydoc?  This is from the gradle task, so I
>> assume
>> > >> this is being done with groovy 3's groovydoc.  Is this something
>> addressed
>> > >> in groovydoc under groovy 4?
>> > >>
>> > >> The source file in question is rather simple.  The methods being
>> > >> reordered are from the parent class (which is in Groovy instead of
>> > >> Grails).   Here's the source:
>> > >>
>> > >> @CompileStatic
>> > >> abstract class SettingsFile extends Script {
>> > >>
>> > >>
>> > >>     void include(String[] projectPaths) {
>> > >>         binding.setVariable("projectPaths", projectPaths)
>> > >>     }
>> > >>
>> > >>     void includeFlat(String[] projectPaths) {
>> > >>         binding.setVariable("projectPaths", projectPaths)
>> > >>     }
>> > >>
>> > >>     def methodMissing(String name, args) {
>> > >>         // ignore
>> > >>     }
>> > >>
>> > >>     def propertyMissing(String name) {
>> > >>         // ignore
>> > >>     }
>> > >> }
>> > >>
>> > >>
>> > >>
>> > >> On Sat, Apr 26, 2025 at 5:32 AM Jochen Theodorou
>> > >> <blackd...@gmx.org.invalid> wrote:
>> > >>
>> > >>> On 24.04.25 23:06, James Daugherty wrote:
>> > >>> [...]
>> > >>> > I'm planning to continue researching this, but if anyone has
>> experience
>> > >>> > with the AST transforms or the ordering problem, it would be
>> helpful to
>> > >>> > solve this.  This is probably the largest blocker to us proceeding
>> > >>> with an
>> > >>> > ASF release.
>> > >>>
>> > >>> as for the ordering. Groovy wants in general to use the order of
>> > >>> declaration in the source code. What we are talking about are most
>> > >>> likely methods added by transforms. And if it is one time stable
>> and one
>> > >>> time not, then I guess Maps are involved. ClassNode has several
>> > >>> getDeclaredMethodsXY methods, which are based on a methods maps,
>> while
>> > >>> getMethods or getMethodsList is based on the methods list.
>> > >>>
>> > >>> If you then have something like
>> > >>>
>> > >>> classNode.getAllDeclaredMethods().findAll(it annnotated with
>> XY).each
>> > >>> {doSomethingWith(it)}
>> > >>>
>> > >>> and there is more than one method annotated, then the order might be
>> > >>> broken between builds. Just to give one example.
>> > >>>
>> > >>>
>> > >>> I would suggest to go through all transforms and check on the usage
>> of
>> > >>> the getDeclaredMethods methods and then avoid them to use for
>> iteration.
>> > >>> They are ok for lookup of course.
>> > >>>
>> > >>> bye Jochen
>> > >>>
>> > >>
>> >
>>
>

Reply via email to