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

Updated gradle to 6.9.3 and optimized build process #1636

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

ajordanow
Copy link

Build Optimisations

This PR provides set of optimisations that lead to faster build times and more robust local builds:

  1. Incremental builds are now supported (hermes-consumers/build.gradle, hermes-management/build.gradle)
  2. Parallel execution is enabled (org.gradle.parallel=true in gradle.properties)
  3. Tests are now executed in parallel (HermesSenderTest, ReactiveHermesSenderTest, IntegrationTest and ZookeeperBaseTest had to be modified to use random ports - as reusing the same port numbers prevented the tests to be successful when running in parallel mode)
  4. Gradle has been updated to 6.9.3 (latest 6.x version)

Benchmarks

The bencharks were performed on Apple M1 Max machine.

Original build

  • ./gradlew clean assemble check took about 3m 11s (build scan)
  • subsequent ./gradlew clean assemble check took about 20s (build scan)

Optimized build

  • ./gradlew clean assemble check took about 1m 14s (build scan)
  • subsequent ./gradlew clean assemble check took about 1s (build scan)

Build cache

The build cache was not enabled in the build scripts, but the builds can already benefit from it (e.g. when building locally):

# clear the cache
rm -rf ~/.gradle/caches/build-cache-1

# first build
./gradlew clean build --build-cache
...
BUILD SUCCESSFUL in 1m 16s
157 actionable tasks: 157 executed

# subsequent builds
./gradlew clean build --build-cache
...
BUILD SUCCESSFUL in 5s
157 actionable tasks: 78 executed, 79 from cache

Copy link

@runningcode runningcode left a comment

Choose a reason for hiding this comment

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

looks good, just one issue (which was already present in the build before)

hermes-management/build.gradle Outdated Show resolved Hide resolved
@ajordanow ajordanow force-pushed the improve-build branch 9 times, most recently from ebed400 to f107e95 Compare December 12, 2022 11:15
runningcode
runningcode previously approved these changes Dec 12, 2022
Copy link

@runningcode runningcode left a comment

Choose a reason for hiding this comment

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

just some nits, but feel free to open the PR

build.gradle Show resolved Hide resolved
jar.dependsOn(attachHermesConsole, 'prepareIndexTemplate')
sourceSets {
main {
resources.srcDir consoleDir

Choose a reason for hiding this comment

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

you can add the task provider here to automatically wire the task dependencies to ensure they are executed in the correct order

Suggested change
resources.srcDir consoleDir
resources.srcDir tasks.named('attachHermesConsole')

Copy link
Author

Choose a reason for hiding this comment

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

I think this will break the current behaviour of the build as the static files are not attached to the distZip package. This change would add a dependency that will cause static files to be included in the output.

Choose a reason for hiding this comment

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

ah, i see. i suggested this with the assumption that all the outputs would be added.

Do we still would need to make sure that the ProcessResources task dependsOn attachHermesConsole?

Copy link
Author

Choose a reason for hiding this comment

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

I think not - attachHermesConsole is an auxilliary task that copies static files from other subproject. It's intended to be run manually when needed.

tasks.register('prepareIndexTemplate') {
def srcDir = '../hermes-console/dist/static'
def targetDir = consoleDir + '/static'
from srcDir
Copy link

Choose a reason for hiding this comment

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

Small improvement, you should be able to replace this by from tasks.named('buildHermesConsole') and you can remove the dependsOn: 'buildHermesConsole'

Copy link
Author

Choose a reason for hiding this comment

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

done. I also had to rework this task a little bit to have dist as a source (and static part included when copying)

@ajordanow ajordanow force-pushed the improve-build branch 6 times, most recently from d50f559 to 1bb413b Compare December 12, 2022 14:48
@ajordanow ajordanow marked this pull request as ready for review December 12, 2022 15:07
@szczygiel-m szczygiel-m self-assigned this Mar 9, 2023
@szczygiel-m
Copy link
Contributor

Hi @ajordanow, is this PR still relevant?

@ajordanow
Copy link
Author

ajordanow commented Apr 5, 2024

Hi @ajordanow, is this PR still relevant?

@szczygiel-m - Should be (once the merge conflicts are resolved). But the checks were never green for this PR because at the time of the PR submission they constantly failed on main.

@szczygiel-m
Copy link
Contributor

Since then we have improved the stability of the tests, would you be willing to update this PR?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants