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

GEODE-5688 Create task to generate all sources for better IDE behavior #2422

Merged
merged 3 commits into from Sep 17, 2018

Conversation

rhoughton-pivot
Copy link
Member

@rhoughton-pivot rhoughton-pivot commented Sep 4, 2018

Use ./gradle generate to create the sources from both protobuf and
ANTLR, then refresh the IDE sources and build as normal.

Thank you for submitting a contribution to Apache Geode.

In order to streamline the review of the contribution we ask you
to ensure the following steps have been taken:

For all changes:

  • Is there a JIRA ticket associated with this PR? Is it referenced in the commit message?

  • Has your PR been rebased against the latest commit within the target branch (typically develop)?

  • Is your initial contribution a single, squashed commit?

  • Does gradlew build run cleanly?

  • Have you written or updated unit tests to verify your changes?

  • If adding new dependencies to the code, are these dependencies licensed in a way that is compatible for inclusion under ASF 2.0?

Note:

Please ensure that once the PR is submitted, you check travis-ci for build issues and
submit an update to your PR as soon as possible. If you need help, please send an
email to dev@geode.apache.org.

Use `./gradle generate` to create the sources from both protobuf and
ANTLR, then refresh the IDE sources and build as normal.
@metatype
Copy link
Contributor

metatype commented Sep 5, 2018

Why not just run the existing gradle tasks to generate the files?

@rhoughton-pivot
Copy link
Member Author

rhoughton-pivot commented Sep 5, 2018 via email

@jake-at-work
Copy link
Contributor

@metatype there are multiple targets in different projects to execute. Having a meta-task that these, and any new ones, attach to makes it easy to document.

  1. Clone
  2. ./gradlew magic
  3. Import into IJ
    Where magic does everything minimally necessary to import into IJ or Eclipse.


afterEvaluate {
rootProject.generate.dependsOn(generateGrammarSource)
project.assemble.dependsOn(generateGrammarSource)
Copy link
Contributor

Choose a reason for hiding this comment

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

Why do we need this dependency and all the others added after this line?

Copy link
Member Author

Choose a reason for hiding this comment

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

assemble is the lifecycle task most closely aligned with this task as a dependency, then we use mustRunAfter to ensure that our generate tasks happen first within this project in all cases.

Copy link
Contributor

Choose a reason for hiding this comment

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

I don’t understand why we need to do that though. They already happen before assemble by design. We should only need to add a pseudo target to encourage them to run outside of that normal behavior for quick project generation.

Copy link
Member Author

Choose a reason for hiding this comment

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

@pivotal-jbarrett I'll remove the offending lines. Look for incoming commit...

@metatype
Copy link
Contributor

metatype commented Sep 6, 2018

@pivotal-jbarrett gradle build works right (or event dev-build)? I don't get why we need gradle generate. Seems like unnecessary overhead to maintain in our build scripts. Every custom task we write is one more thing to maintain across gradle versions :-)

@jake-at-work
Copy link
Contributor

@metatype I don't see it as anything more than devBuild, which was added to ease the pain of build when all you want to do is develop. Well if you want to develop in an IDE why do I need to run build or devBuild to get 1% of the tasks completed that are necessary to work from the IDE?

Authored-by: Robert Houghton <rhoughton@pivotal.io>
@@ -301,3 +301,7 @@ distributedTest {
// Some tests have inner tests that should be ignored
exclude "**/*\$*.class"
}

afterEvaluate {
Copy link
Contributor

Choose a reason for hiding this comment

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

Why does this need to be done in afterEvaluate?

Copy link
Member Author

Choose a reason for hiding this comment

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

Turns out it does not. I had both generation tasks using identical formatting for consistency, but it is not needed here. I have removed the block for this dependency change.

@PurelyApplied
Copy link
Member

I like the idea of this minimal task, but I somewhat agree with Anthony that having to remember to run ./gradlew generate is little better than having to remember to run ./gradlew dev.

Is there a way to hook this into the idea module (and for Eclipse if necessary)? It's been a huge pain point to get OQLLexerToken.java and protobuf issues when just trying to run a test, and while this makes that quick to fix, it would be ideal if there weren't the break in workflow in the first place.

@jake-at-work
Copy link
Contributor

There is not a way to teach IDEA to generate or run arbitrary tasks. You can tell it to use gradle for all tasks or for none.

…marSource

Authored-by: Robert Houghton <rhoughton@pivotal.io>
@@ -61,3 +61,7 @@ task zip(type: Zip) {
}

assemble.dependsOn 'zip'

afterEvaluate {
Copy link
Contributor

Choose a reason for hiding this comment

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

Is this afterEvaluate really necessary too?

@rhoughton-pivot
Copy link
Member Author

rhoughton-pivot commented Sep 15, 2018 via email

@jake-at-work jake-at-work merged commit 0a493c3 into apache:develop Sep 17, 2018
@rhoughton-pivot rhoughton-pivot deleted the feature/GEODE-5688 branch June 23, 2020 17:09
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
5 participants