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

Make the root project a composite build #97

Merged
merged 18 commits into from
Feb 7, 2020

Conversation

martinbonnin
Copy link
Contributor

@martinbonnin martinbonnin commented Feb 5, 2020

One more step towards lazy task configuration (#69)

This pull requests moves the samples to a composite build makes the rootProject a composite build that will treat the plugin as an included build.

This allows to do dependency substitution on the plugin artifact and therefore have the sample code in sync with the plugin code from the repo (without having to publish on mavenCentral mavenLocal or jcenter first).

If you're working on the samples, you should open swagger-gradle-codegen/sample in your favorite IDE. Opening the root project in intelliJ is still the recommended way to work on the project as it will include the necessary projects.

@cortinico
Copy link
Collaborator

This allows to do dependency substitution on the plugin artifact and therefore have the sample code in sync with the plugin code from the repo (without having to publish on mavenCentral or jcenter first).

You don't need to publish on mavenCentral or jcenter at the moment. You have to publish the plugin to mavenLocal and use it in a subsequent build.

If you're working on the samples, you should open swagger-gradle-codegen/sample in your favorite IDE.

Ideally we want to avoid having to open two separate project to work with samples and/or plugins. The convenience of the current setup is that you open a project and you have everything there.

@martinbonnin
Copy link
Contributor Author

martinbonnin commented Feb 5, 2020

This allows to do dependency substitution on the plugin artifact and therefore have the sample code in sync with the plugin code from the repo (without having to publish on mavenCentral or jcenter first).

You don't need to publish on mavenCentral or jcenter at the moment. You have to publish the plugin to mavenLocal and use it in a subsequent build.

Sorry, I meant mavenLocal, not mavenCentral 🤦‍♂️ . As I said in the other PR, I'm not a fan of using mavenLocal as it introduces a global state that can have side effects and that needs to be maintained.

Ideally we want to avoid having to open two separate project to work with samples and/or plugins.

Technically, you can do all the work from the composite build and edit the plugin files from there.

The convenience of the current setup is that you open a project and you have everything there.

The drawback being that changing the plugin requires to call publishToMavenLocal for the changes to be seen by the samples. Also when the project is first opened, the samples use the plugin from jcenter and not the one from the repo, which might be confusing if they don't behave exactly the same.

Let me know what you think. I started to work on this while working on lazy configuration because I changed some API in GenerateTaskConfiguration and this broke my gradle sync. But now I know about publishToMavenLocal, I can do the lazy configuration without having composite builds too.

@cortinico
Copy link
Collaborator

Sounds like composite build could help us improve the project setup 👍 I'm in for it. I just ask you to unwrap the setup that you're proposing here.

Ideally the base project should be the one in the root folder. From there you should include all the sample modules (as gradle module) and plugin as a composite build.

In other words: in the top level settings.gradle.kts the goal is to have something like.

include(":samples:junit-tests",
        ":samples:kotlin-android",
        ":samples:kotlin-android-moshi-codegen",
        ":samples:kotlin-coroutines",
        ":samples:groovy-android")
includeBuild(":plugin")

This will make sure that by running gradle in the top folder, everything works as expected.

@cortinico cortinico self-requested a review February 5, 2020 22:26
@cortinico cortinico added the infra PR or Issue related to project infrastructure label Feb 5, 2020
@cortinico cortinico added this to the 1.4.0 milestone Feb 5, 2020
Makefile Show resolved Hide resolved
settings.gradle.kts Outdated Show resolved Hide resolved
@martinbonnin martinbonnin changed the title Move samples to a composite build Make the root project a composite build Feb 6, 2020
Makefile Show resolved Hide resolved
plugin-root/gradle Outdated Show resolved Hide resolved
plugin-root/gradlew Outdated Show resolved Hide resolved
plugin-root/gradlew.bat Outdated Show resolved Hide resolved
plugin-root/settings.gradle.kts Outdated Show resolved Hide resolved
Makefile Outdated Show resolved Hide resolved
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.

Awesome! I tested it and it works great! Really looking forward to merge this. I left just a minor comment

Makefile Show resolved Hide resolved
@macisamuele
Copy link
Collaborator

@martinbonnin I think that #101 caused merge conflicts with this branch. Sorry 😢

Let me know if you need support or you are able to address them by yourself (I don't expect it being very complex)

@martinbonnin
Copy link
Contributor Author

@macisamuele this branch renames a lot of files so I guess that was expected. I just rebased on master, it should be good now.

@macisamuele
Copy link
Collaborator

@martinbonnin ++
Looked like you had @cortinico approval for your changes. So I'll prefer having him taking care of merging this change.
From a quick pass through the changes I see why the conflicts were there and how rebasing fixed all the things, so it should be all good ™️

Thanks a lot for helping in improving our gradle configurations 💯

Makefile Show resolved Hide resolved
@cortinico cortinico merged commit cc71ca0 into Yelp:master Feb 7, 2020
@macisamuele
Copy link
Collaborator

keep the Make+Gradle discussion opened

@cortinico @martinbonnin : I'm the kind of lazy developer ;)
I do usually configure, in each git repo I'm working on, a custom git command (ie. git push-after-tests) that allows me to push to origin only if tests are green.
Said so I don't have a very strong opinion on having Make or Gradle or both, is more about having a single command to run that basically runs all the tests that travis would run.

Having a gradle task that would behave like the current make test would be more than enough on my side 😄

@cortinico
Copy link
Collaborator

I'm totally in for nuking the Makefile. @martinbonnin do you want to take this?

@martinbonnin
Copy link
Contributor Author

👍 I'll open a PR. What's a good name for the gradle task that'll do everything ? test is already taken. Something like ./gradlew buildProject ? ./gradlew makeAll ? Something else ?

@macisamuele
Copy link
Collaborator

What about travisTest? At the end the task should be connected to the travis execution as well 😊

@cortinico
Copy link
Collaborator

I'd rather go for preMerge or preMergeTest as is CI agnostic and contains all the verification tasks that needs to be executed before integrating a branch

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.

3 participants