Skip to content

#14729 - Corrupt Jar & War Files #14745

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

Merged
merged 35 commits into from
May 17, 2025
Merged

#14729 - Corrupt Jar & War Files #14745

merged 35 commits into from
May 17, 2025

Conversation

jdaugherty
Copy link
Contributor

@jdaugherty jdaugherty commented May 16, 2025

Fixes #14729

This is a rather large change. There are multiple changes in this PR, including:

  1. by guaranteeing order of the AST Transformations, a side effect was that grails-plugin.xml & grails.factories are now properly generating for all projects in grails core. This meant any existing copy of those files that was checked into resources had to be removed. These duplicate files were 1 cause of the duplicated jar file

  2. AbstractFileResolvingResource.exists incorrectly reports result for resources inside of spring-boot executable jar spring-projects/spring-framework#34796 affected nested jar files and versions of spring boot after 3.4.3 all include the affected version of spring. There doesn't appear to be a "spring-dependencies" equivalent of spring framework like there is for spring boot. This means to override the spring version, every spring library has to be added to the bom and overridden. Worse, Maven / Gradle do not yet support exclusions so this is actually a difficult problem that significantly complicates our bom. For this reason, I've rolled us back to 3.4.3 until 3.5.0 comes out with the 6.2.7 spring framework fix.

  3. In Gradle 8, tasks now run in process isolation via the worker api. This means that the version of groovy can be different than Gradle. The problem is the grails-gradle-model doesn't export the version of groovy since it assumes there isn't isolation. So now it exports the version of gradle that it was built with since this library is used by both tasks and plugins

  4. reworks the FindMainTask to properly set the main class and fail if a bootJar / bootRun are enabled, but no Application class exists. I discussed this behavior with @jamesfredley, and I think end grails app developers need to understand that those tasks will generate invalid jars so it's important to fail. grails-geb is a good example of this, adding bootJar.enabled = false / bootRun.enabled = false will prevent the failure. We could alternatively just log a warning, but the type of failures caused by invalid jars are incredibly hard to diagnosis. Forcing a failure is a time saver. This change will likely affect plugins the most. We could make a further enhancement to detect if it's a plugin, and not hard fail in that case. @jamesfredley should we ticket this optional enhancement?

  5. removes the 'doc' method on the grails plugin since it's been deprecated since Grails 1 from when Grails used to be both a Build System & an app. This method previously generated documentation.

  6. Cleans up the Upgrade7.MD for any entry that's been added to the grails-doc.

  7. Removes the duplicated codec plugin that is deprecated. Updates the docs to reflect this change similar to the changes in gsp where we had other files in this scenario.

  8. removes the remaining pathingJar code since gradle properly handles this with bootRun and our start scripts handle it otherwise. People can use a shadowJar if they need to have their end application perform this function.

  9. Fixes the grails-gsp plugin so that it properly nests files in the jar again.

  10. In DataBindingUtils, I found some code that was calling class.getClass() and believe this is a bug in the domain databinding. Tests pass after this change, and the original change that changed this from class to class.getClass() claimed it was done for tests.

  11. As part of reviewing the gradle output to debug the invalid docs, I've cleaned up several warnings that I thought were straight forward changes via intellij refactoring.

  12. This change revealed there is a lack of test coverage for jar & war file starts. We know that from the profiles a GroovyMXBean can be used to stop an application. I think we should add tests that actually start each test app via jar / war file. There aren't changes to address this in this PR, but we should likely ticket this.

  13. There's a rather major problem with the functional tests: they pull the bom from the upstream repo instead of locally. This is really only a problem when changing a dependency, which we don't often do. This is due to Referencing a BOM project in a multi-project build spring-gradle-plugins/dependency-management-plugin#164 since we can't use the bom in the same gradle project. As a "workaround" I've added a local repo that only contains the grails-bom to make testing the functional tests with version changes. Running the task './gradlew :grails-bom:publishMavenPublicationToLocalBomRepository` will publish to that local repo, which typically will be empty, and then the functional tests will pull the dependencies.

  14. removes SDKman workflow, which was used prior to Grails 6. The release to SDKman occurs in grails-forge: https://github.com/apache/grails-forge/blob/7.0.x/.github/workflows/release.yml#L91

Since we do publish a valid gradle module for the bom, we could switch to using platforms exclusively in the functional tests, but I think that will defeat the purpose of these tests - end users are still using the spring dependency management plugin since gradle platforms do not support properties. So until gradle upstream implements a property override for the platform versions, we need to continue to test with the dependency management plugin. I think the workaround proposed here needs documented in a ticket and we need to track this longer term on how to fix this.

@jdaugherty jdaugherty requested review from matrei and jamesfredley May 16, 2025 13:42
@jdaugherty jdaugherty force-pushed the corrupt-jar-fixes branch from d51d0a4 to ce686fe Compare May 16, 2025 13:43
@jdaugherty jdaugherty marked this pull request as ready for review May 16, 2025 13:43
@jdaugherty
Copy link
Contributor Author

Hold off on this review, groovydoc isn't generating docs for this change correctly.

@jdaugherty
Copy link
Contributor Author

I reverted the java 17 cleanup - groovydoc still has an older language level set to it. I've reached out to both the Gradle Community & Paul to see if we can get this fixed longer term.

@jdaugherty
Copy link
Contributor Author

I also do not error on find main task if it's a plugin (so we do not need the enhancement from bullet 4)

@jdaugherty
Copy link
Contributor Author

(this is ready to review, we merged the spring boot change separately so the tests can run with the correct version once it's published)

@jamesfredley
Copy link
Contributor

jamesfredley commented May 16, 2025

Issue for point 4: #14748

Copy link
Contributor

@matrei matrei left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Great job James!
What's the story about the updated license headers?

@@ -46,6 +46,12 @@ ext {

allprojects {
repositories {
// workaround for https://github.com/spring-gradle-plugins/dependency-management-plugin/issues/164
maven {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm starting to think we should ditch the Dependency Management Plugin, and only use Gradle Platforms.
We are actually adding the grails-bom as a platform in generated apps.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I 100% agree but we need the ability for end users to easily set versions. I am debating whats better: ditch the dependency plugin inside of grails core or keep it to align with what end user apps use. If gradle platforms supported property overrides, I would have suggested we already switch. I came close to writing a Gradle plugin to just add this support too.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think property version overrides are a really good feature but when needed it can be accomplished with Gradle Platforms as well, only not as easily, as you have to declare the dependency you want to be able to override beforehand.
We should rip that band-aid, it causes confusion to do both.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I still maintain we shouldn't use both in end user apps - we should one or the other. The property overrides make it very easy to set versions. We should discuss this more outside of this PR.

@jdaugherty
Copy link
Contributor Author

Great job James!

What's the story about the updated license headers?

IntelliJ was updating these. Looks like I need to adjust its header. I will revert these changes.

@jdaugherty jdaugherty force-pushed the corrupt-jar-fixes branch from 6098d29 to 745f5d5 Compare May 17, 2025 12:17
@jdaugherty
Copy link
Contributor Author

jdaugherty commented May 17, 2025

  1. Per separate discussion, I've added [skip tests] option to skip the tests to make publishing the bom easier.
  2. After publishing the bom, I realized that the ErrorsControllerSpec is broken. Retesting, it seems to be broken in all of 3. 4.x in spring boot. The page being rendered in ErrorsControllerSpec appears to be the tomcat default 500 error page. I've switched this back to 3.5.0-RC1 & setup constraints for spring framework 6.2.7. Now both these tests + the nested jar issue is fixed.
  3. Per separate discussion, the .configure {} & .configureEach changes are being tested here: https://github.com/matrei/grails-core/tree/simplify-build I'll try to follow-up this week and see if I can make this change everywhere instead of piecemeal.

@jdaugherty jdaugherty merged commit 0c37d0d into 7.0.x May 17, 2025
17 of 28 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.

Invalid or corrupt jarfile - bootjar & bootwar
3 participants