Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

#13552 Merge grails-async into grails-core #13955

Merged
merged 621 commits into from
Jan 9, 2025
Merged

Conversation

jdaugherty
Copy link
Contributor

@jdaugherty jdaugherty commented Jan 7, 2025

  • All of the tests pass locally.
  • Docs will be moved to grails-doc repo (separate PR)
  • Old async github configuration (workflows, dependabot, issue templates, etc) is removed after importing
  • I preserved the artifact names & project info for old async artifacts
  • async was split into grails-async , grails-events, & grails-test-examples folders
  • I found a bug where the test projects were in the bom, they are now removed
  • fixed async & events in the bom to use the grails version
  • any part that was questionable has this comment prefix TODO - async move

This PR contains the entire history of grails-async, which was imported under /grails-async. It then modifies the original async structure to be like this:

    grails-async
    ├── core
    │   ├── build
    │   └── src
    ├── gpars
    │   ├── build
    │   └── src
    ├── plugin
    │   ├── build
    │   ├── grails-app
    │   └── src
    ├── rxjava
    │   ├── build
    │   └── src
    └── rxjava2
        ├── build
        └── src
    grails-events
    ├── compat
    │   ├── build
    │   └── src
    ├── core
    │   ├── build
    │   └── src
    ├── gpars
    │   ├── build
    │   └── src
    ├── plugin
    │   ├── build
    │   ├── grails-app
    │   └── src
    ├── rxjava
    │   ├── build
    │   └── src
    ├── rxjava2
    │   ├── build
    │   └── src
    ├── spring
    │   ├── build
    │   └── src
    └── transform
        ├── build
        └── src
    grails-test-examples
    └── async-events-pubsub-demo
        ├── grails-app
        └── src

To make this easier to review, only review the commits after "Imported grails-async" ... any commit prior is just imported for-like under /grails-async. Commits after that are moving the project, splitting it, etc.

@jdaugherty jdaugherty marked this pull request as ready for review January 7, 2025 21:58
@jdaugherty
Copy link
Contributor Author

grails/grails-doc#947 is the doc PR. For merge order: we should merge this one first, and then that one (and kick off the doc build again).

@codeconsole
Copy link
Contributor

codeconsole commented Jan 8, 2025

After reviewing a few things, I think bringing this in may a bad idea:

  1. grails-async and events aren't even used by a starter grails app or in most grails applications?
  2. the preferred approach going forward should probably be using @Async?

if 1 and 2 are true, this code really doesn't belong in this repo and really should be on an independent release cycle.

@matrei
Copy link
Contributor

matrei commented Jan 8, 2025

  1. grails-async and events aren't even used by a starter grails app or in most grails applications.

grails-spring-security-core depends on them.

  1. the preferred approach going forward should probably be using @Async?

This may or may not be true. I think grails-async enables a very groovy programming style.

build.gradle Outdated Show resolved Hide resolved
build.gradle Outdated Show resolved Hide resolved
build.gradle Outdated Show resolved Hide resolved
build.gradle Outdated Show resolved Hide resolved
build.gradle Outdated Show resolved Hide resolved
grails-test-examples/async-events-pubsub-demo/build.gradle Outdated Show resolved Hide resolved
settings.gradle Outdated Show resolved Hide resolved
grails-test-examples/async-events-pubsub-demo/build.gradle Outdated Show resolved Hide resolved
grails-test-suite-uber/build.gradle Outdated Show resolved Hide resolved
grails-test-suite-web/build.gradle Outdated Show resolved Hide resolved
@davydotcom
Copy link
Contributor

👍

@davydotcom
Copy link
Contributor

This merge makes sense to me. It very rarely changes outside of grails core. In that context alone I think it's fine to merge into the single mono repo to reduce complexity in release

@jdaugherty
Copy link
Contributor Author

Per discussion with @jamesfredley & @matrei , I have changed the bom scope to implementation from api. We do not want any grails project exporting the bom as an API dependency, because all projects & grails apps should explicitly include the version. Otherwise, you'll fight version resolution like @codeconsole has commented on in the slack channel.

@jdaugherty jdaugherty force-pushed the 7.0.x branch 2 times, most recently from 669b7d2 to 9e8333b Compare January 8, 2025 16:42
@jdaugherty
Copy link
Contributor Author

jdaugherty commented Jan 8, 2025

After fixing the projectDir for the example grails application from the async/events, it's failing to build with this error:

    Caused by: java.lang.NoSuchMethodError: 'org.springframework.asm.Type org.springframework.asm.Type.getMethodType(org.springframework.asm.Type, org.springframework.asm.Type[])'

It's failing when the gradle plugin is applied in the test project. The gradle plugin depends on the grails-gradle-model, which is in core. Because of this circular reference, to correctly test, I don't think a full grails project can live inside of core (unless we had a mono repo).

I'm looking for solutions to this problem.

@matrei
Copy link
Contributor

matrei commented Jan 8, 2025

@jdaugherty Add this to grails-test-examples/async-events-pubsub-demo/build.gradle and I think it will work:

bootRun {
    mainClass = 'pubsub.demo.Application'
}

tasks.withType(Test).configureEach {
    useJUnitPlatform()
}

@jdaugherty
Copy link
Contributor Author

@jdaugherty Add this to grails-test-examples/async-events-pubsub-demo/build.gradle and I think it will work:

bootRun {
    mainClass = 'pubsub.demo.Application'
}

tasks.withType(Test).configureEach {
    useJUnitPlatform()
}

Adding these did not resolve the error:

            Caused by: java.lang.NoSuchMethodError: 'org.springframework.asm.Type org.springframework.asm.Type.getMethodType(org.springframework.asm.Type, org.springframework.asm.Type[])'
            at org.springframework.boot.loader.tools.MainClassFinder.<clinit>(MainClassFinder.java:61)
            at org.springframework.boot.gradle.plugin.ResolveMainClassName.findMainClass(ResolveMainClassName.java:145)
            at org.springframework.boot.gradle.plugin.ResolveMainClassName.resolveMainClassName(ResolveMainClassName.java:139)
            at org.springframework.boot.gradle.plugin.ResolveMainClassName.resolveAndStoreMainClassName(ResolveMainClassName.java:124)

@codeconsole
Copy link
Contributor

codeconsole commented Jan 8, 2025

grails-spring-security-core depends on them.

@matrei that's not a reason to merge async into core. It's probably the opposite. IMO, It is very likely grails-spring-security-core plugin has no future. Most of what it does works on deprecated functionality scheduled to be removed this year.

Its current state doesn't even remotely match the existing filter chain. I had to completely write a new plugin to properly lock down my application.

@jdaugherty
Copy link
Contributor Author

I may have found the cause for the test project: hierynomus/license-gradle-plugin#161

- docs are moved to grails-doc repo
- old async github workflows & associated config removed
- async & events split into separate directories
- shared example project now lives in grails-test-examples
- remove test and example projects from the bom
- fix async / events in the bom now that they're in grails-core
- change the build action to not fail fast so we can identify which version of java is failing
@matrei
Copy link
Contributor

matrei commented Jan 9, 2025

grails-spring-security-core depends on them.

@matrei that's not a reason to merge async into core. It's probably the opposite. IMO, It is very likely grails-spring-security-core plugin has no future. Most of what it does works on deprecated functionality scheduled to be removed this year.

Its current state doesn't even remotely match the existing filter chain. I had to completely write a new plugin to properly lock down my application.

This was only intended as feedback to your question/statement that "most grails applications don't use" async & events.

build.gradle Show resolved Hide resolved
@davydotcom davydotcom merged commit 4d9d2f6 into grails:7.0.x Jan 9, 2025
20 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.