I think we should keep this as a side branch for now. If I understand correctly, we will still have issues with reproducibility, so I don't see the value in merging it at the moment. Merging it will make Groovy a moving target that might create problems that are difficult to diagnose/understand when working on other tasks.
Den ons 7 maj 2025 kl 08:28 skrev Søren Berg Glasius <soe...@glasius.dk>: > No objections. > > Den tirs. 6. maj 2025 kl. 16.54 skrev James Daugherty > <jdaughe...@jdresources.net.invalid>: > > > 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 > > >> > >>> > > >> > >> > > >> > > > >> > > > > > > > > -- > > Med venlig hilsen, > Søren Berg Glasius > > Hedevej 1, Gl. Rye, 8680 Ry > Mobile: +45 40 44 91 88 > --- Press ESC once to quit - twice to save the changes. >
