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

Migrate Makefile targets to Gradle tasks #105

Merged
merged 8 commits into from
Feb 10, 2020

Conversation

martinbonnin
Copy link
Contributor

Replace the Makefile targets with Gradle tasks:

make install-hooks => ./gradlew installHooks
make run-hooks => ./gradlew runHooks
make test => ./gradlew preMerge

Copy link
Collaborator

@cortinico cortinico left a comment

Choose a reason for hiding this comment

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

Thanks for this! Also is @macisamuele happy with this setup?

build.gradle.kts Show resolved Hide resolved
build.gradle.kts Outdated
Comment on lines 50 to 53
dependsOn(gradle.includedBuild("gradle-plugin").task(":plugin:build"))
dependsOn(gradle.includedBuild("gradle-plugin").task(":plugin:check"))
dependsOn(subprojects.filter { it.name != "samples" }.map { it.tasks.named("assembleDebug") })
dependsOn(subprojects.filter { it.name != "samples" }.map { it.tasks.named("check") })
Copy link
Collaborator

Choose a reason for hiding this comment

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

This won't work at the moment, as you have to specify also the mustRunAfter rules. In the current situation you're just saying that preMerge depends on runHooks, :plugin:build, :plugin:check, assembleDebug, check but you're not specifying the order.

This is also the reason why Travis is failing (he's running a one of a sample's check before the generateSwagger).

Copy link
Contributor Author

@martinbonnin martinbonnin Feb 8, 2020

Choose a reason for hiding this comment

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

This is also the reason why Travis is failing (he's running a one of a sample's check before the generateSwagger).

That was the point of this block int the root build.gradle.kts:

        tasks.all {
            if (name == "assembleDebug") {
                dependsOn(tasks.named("generateSwagger"))
            }
        }

Except I forgot that the generated files are needed before the compile task 🤦‍♂. So at the moment I think there's a race between compileKotlin and generateSwagger, both being dependencies of assembleDebug. I hopefully fixed that here: b0e45e7#diff-392475fdf2bc320d17762ed97109a121R66

build.gradle.kts Outdated
Comment on lines 59 to 62
if (name == "assembleDebug") {
// assembleDebug needs the generated files
dependsOn(tasks.named("generateSwagger"))
}
Copy link
Collaborator

Choose a reason for hiding this comment

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

Maybe we can have this as part of the preMerge?

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 guess we could but isn't it nice to have gradle regenerate the swagger files on-demand ?

I.e. you could run assembleDebug, build, compileMainReleaseeKotlin, etc... and Gradle would make sure the generated files are up-to-date without having to run generateSwagger first.

…kts.

This makes KotlinCompile visible from the root projet. Use that to make all KotlinCompile tasks depend on GenerateSwagger
build.gradle.kts Outdated
dependsOn(runHooks)
dependsOn(gradle.includedBuild("gradle-plugin").task(":plugin:build"))
dependsOn(gradle.includedBuild("gradle-plugin").task(":plugin:check"))
dependsOn(subprojects.filter { it.name != "samples" }.map { it.tasks.named("assembleDebug") })
Copy link
Contributor Author

Choose a reason for hiding this comment

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

We could maybe remove the "assembleDebug" there. The "check" test below already executes testDebugUnitTest and testReleaseUnitTest so it will compile the sources for both debug and release mode.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Agree, let's remove it 👍

@macisamuele
Copy link
Collaborator

@martinbonnin ++
Thanks for putting the effort to gradle-ify the repo 😄

I'm not very familiar with gradle so I'll rely @cortinico for comments.
For what I can see the makefile targets have been converted to gradle tasks , so all good here

Copy link
Collaborator

@cortinico cortinico left a comment

Choose a reason for hiding this comment

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

LGTM, let's just remove line 52 and we should be good to go!

build.gradle.kts Outdated
dependsOn(runHooks)
dependsOn(gradle.includedBuild("gradle-plugin").task(":plugin:build"))
dependsOn(gradle.includedBuild("gradle-plugin").task(":plugin:check"))
dependsOn(subprojects.filter { it.name != "samples" }.map { it.tasks.named("assembleDebug") })
Copy link
Collaborator

Choose a reason for hiding this comment

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

Agree, let's remove it 👍

@cortinico cortinico merged commit 444b8c5 into Yelp:master Feb 10, 2020
@cortinico cortinico added the infra PR or Issue related to project infrastructure label Feb 10, 2020
@cortinico cortinico added this to the 1.4.0 milestone Feb 10, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
infra PR or Issue related to project infrastructure
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants