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

Simplify generated build #424

Merged
merged 16 commits into from
Nov 14, 2024
Merged

Simplify generated build #424

merged 16 commits into from
Nov 14, 2024

Conversation

jamesfredley
Copy link
Contributor

@jamesfredley jamesfredley commented Nov 11, 2024

This was significantly more involved than anticipated.

This PR:

  • Addresses the 5 linked issues
  • Consolidates Gradle Plugin configuration to buildscript{}, plugins{} and apply plugin: in build.gradle
  • Changes Gradle buildSrc/build.gradle and Settings File to non-default, optional features
  • When the Gradle Settings File feature is selected, it only contains one line defining the root project
  • When the Gradle buildSrc feature is selected buildscript{} is not generated, they are equivalent
  • Works around org.grails.grails-web & org.grails.grails-gsp Gradle Plugin ids with versions not working for snapshots grails-gradle-plugin#351 by using "apply plugin:" for Gradle plugin ids that are not published to the Gradle Plugin repo and are not release versions. Release versions work fine with id and version.
  • significant updates to fix the tests, including one that was not cross platform compatible
  • adds classpath scope
  • updates asset-pipeline to 4.5.1
  • standardizes on double quotes with no parentheses in build.gradle, did not update web-driver/selenium since it has been removed in 7.0.x
  • dependencies set to pom will still have parentheses, since they are required for platform()
  • move hamcrest, micronaut-http-client and neo4j-harness dependencies to features instead of manually being set in dependencies.rocker.raw
  • remove dependencies.rocker.raw since it duplicates toSnippet()
  • rename RUNTIME to RUNTIME_ONLY, TEST to TEST_IMPLEMENTATION and
    TEST_RUNTIME to TEST_RUNTIME_ONLY to reflect the actual Gradle scopes
  • rename compile to implementation

Merging up to 7.0.x is going to be fun.

@jamesfredley
Copy link
Contributor Author

This required significant changes to get the desired outcome. We need to debate whether this is completed in 6.2.2 or 7.0.0.

grails/grails-doc#923 - ticket to update documentation.

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.

This was some heavy lifting, nice work James!

I think there is a risk that doing this big of a change will introduce bugs in 6.2.2. As you say, we should discuss if it is worth the trouble to merge this to 6.2.x.

I'm not sure it's even going to help anyone out, but rather cause more confusion. If a user is on Grails 5 or lower, is there any need to upgrade to Grails 6 before upgrading to Grails 7? If a user is on Grails 6 it should be an easy operation to remove buildSrc when upgrading to Grails 7 (if that is what the user wants).

@codeconsole
Copy link
Contributor

codeconsole commented Nov 11, 2024

I'm not sure it's even going to help anyone out, but rather cause more confusion. If a user is on Grails 5 or lower, is there any need to upgrade to Grails 6 before upgrading to Grails 7? If a user is on Grails 6 it should be an easy operation to remove buildSrc when upgrading to Grails 7 (if that is what the user wants).

@matrei we need to think from the perspective of what is create-app for? Creating new apps.

  1. Why create an overcomplicated build for a new app only for someone to have to dramatically change it in an upcoming release?
  2. Grails <= 6.2.0 was broken. We were just cleaning up an inherited mess and are putting our names behind the release. Let's finish with a more polished product we can be proud of.
  3. This has to be done either way for Grails 7.
  4. This can continue to evolve independently as it is only forge. It does not introduce any bugs or affect 6.2.2 working in any way.
  5. The current behavior is inconsistent with Grails shell. Does Grails shell create-app create a buildSrc?

…o features

instead of manually being set in dependencies.rocker.raw
remove dependencies.rocker.raw since it duplicates toSnippet()

rename RUNTIME to RUNTIME_ONLY, TEST to TEST_IMPLEMENTATION and
TEST_RUNTIME to TEST_RUNTIME_ONLY to reflect the actual Gradle scope
public static final Scope TEST_ANNOTATION_PROCESSOR = new Scope(Source.TEST, Collections.singletonList(Phase.ANNOTATION_PROCESSING));
public static final Scope TEST_COMPILE_ONLY = new Scope(Source.TEST, Collections.singletonList(Phase.COMPILATION));
public static final Scope TEST_RUNTIME = new Scope(Source.TEST, Collections.singletonList(Phase.RUNTIME));
public static final Scope TEST_RUNTIME_ONLY = new Scope(Source.TEST, Collections.singletonList(Phase.RUNTIME));
Copy link
Contributor

Choose a reason for hiding this comment

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

Should we also add the integrationTest* scopes for completeness. Geb, Selenium and perhaps some others should preferably be moved to those.

And what about renaming compile to implementation?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good catch, I've renamed compile to implementation.

For integrationTest* scopes, I'd like to do that on the 7.0.x branch since it already has INTEGRATION_TEST_IMPLEMENTATION_TEST_FIXTURES

public static final Scope INTEGRATION_TEST_IMPLEMENTATION_TEST_FIXTURES = new Scope(Source.MAIN, Collections.singletonList(Phase.INTEGRATION_TEST_IMPLEMENTATION_TEST_FIXTURES));

@jamesfredley
Copy link
Contributor Author

I've seen this test fail several times. Deleting build manually gets it working again.

> Task :grails-forge-core:test FAILED

GradleSpec > test buildSrc/build.gradle FAILED
    Condition not satisfied:

    buildSrcGradle
    |
    ""
        at org.grails.forge.build.gradle.GradleSpec.test buildSrc/build.gradle(GradleSpec.groovy:50)

…e same as main build.gradle file

this is the only Gradle build dependency that is needed in buildSrc/build.gradle and not in buildscript{}
@jamesfredley jamesfredley merged commit a5994bd into 6.2.x Nov 14, 2024
10 checks passed
@jamesfredley jamesfredley deleted the simplify-generated-build branch November 14, 2024 22:59
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment