-
Notifications
You must be signed in to change notification settings - Fork 275
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
Refactor ProtobufPlugin to support multiple languages #76
Conversation
Change-Id: I8758b8d3b094c08a4ac7d15b97de8c59053198e5
I didn't run the testProjectAndroid as it fails even after installing android sdk :( |
import com.google.protobuf.gradle.ProtobufConvention | ||
import com.google.protobuf.gradle.TaskGenerator | ||
import com.google.protobuf.gradle.Utils | ||
import javafx.concurrent.Task |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Import by accident?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
likely the usage is removed after some refactoring
Please provide more details about what this change does, e.g., what languages are supported. Please note there is an earlier PR (#44) by @Shad0w1nk that will add C++ support. You will likely step on each other. @Shad0w1nk could you comment on the status of your PR? |
Were you trying to (or are you going to) port the android test to the new test framework? What error did you get? |
I got error for testProjectAndroid as following UNEXPECTED TOP-LEVEL EXCEPTION: 1 error; aborting :testProjectAndroid:transformClassesWithDexForArmFreeappDebug FAILED |
I am looking for implementing C# support onto the plugin, and here is the draft
I have notice that and test case refactoring are borrowing the idea from the PR |
Googling for "com.android.dx.cf.iface.ParseException: bad class file magic (cafebabe) or version (0034.0000)", all results suggest issues with JDK version or compatibility issues. Assuming 0034 is hex, it is 52 in decimal which indicates Java 8. In |
Thanks @zhangkun83, after using jdk 1.7 I could run the testProjectAndroid, moved also the testProjectAndroid as well |
@ngyukman It doesn't seem the android tests are actually being run. If you alter the assertions in the second half of |
Test project is something I had difficulty in my pull request. Porting them to the TestKit framework will greatly help. With TestKit, you can say which version of Gradle you want to use for the test which would be useful with Android test as the minimum version of Gradle is not the latest. This way you can have the plugin use the latest version but run the test using an older version. @ngyukman For sure we can sync up on the work. I was moving toward an implementation in the software model for C++ as this is the direction where Gradle is moving. With the model, this limitation (https://discuss.gradle.org/t/limitation-of-generatedby-in-native-software-model/17321) does slow down the pull request a lot. |
@zhangkun83 you are right the test didn't run correctly due to some stupid mistake |
indeed the same issue found in #78 |
Unit test can now be ran on travis :) |
Sorry for being slow because I have other work to do. I will review this as quickly as I can. In general I like the new way of doing tests. I will first review the changes in tests, then the changes in the plugin. The tests are not re-run when any files are changed in the test projects. This is because the test projects directory are not in the transitive
|
// Include test projects only when we intend to run the tests. | ||
// We have to exclude them when we intend to build the plugin, because the test | ||
// projects apply the plugin at evaluation time. At evaluation time the plugin | ||
// has not been compiled yet. | ||
if (gradle.startParameter.taskNames.intersect(['install', 'uploadArchives', 'publishPlugins'])) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This was needed because the plugin needed to be installed from a separate command prior to being tested. You can remove it now.
Conflicts: build.gradle src/main/groovy/com/google/protobuf/gradle/ProtobufPlugin.groovy testProjectAndroid/build.gradle testProjectDependent/build.gradle
- Add test projects to test inputs - Remove unnecessary settings
you are right the testProjects are not part of the test source, thanks |
'generateX86RetailappDebugProto', 'generateX86RetailappReleaseProto'] as Set == | ||
protobuf.generateProtoTasks.all().collect({ it.name }) as Set | ||
|
||
assert ['generateArmFreeappDebugAndroidTestProto', |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's not common to use assert
in tests. I would stay away from it, as it's not obvious whether it's enabled or not, although it's enabled in this case as I tested. Please revert to the JUnit assertions.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi, I am not sure what do you mean by whether it's enable or not
but groovy assert is always executed, and provide much better comparison report
see http://groovy-lang.org/semantics.html#_power_assertion
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Didn't know the groovy assert was different from Java. That looks good :)
@ngyukman The changes in the tests look good to me. I am now looking at the plugin changes. A code-style note for Groovy files: please apply 2-space general indentation and 4-space line-break indentation, with a line length limit of 100. |
void applyWithPrerequisitePlugin(List<String> possiblePluginNames, String pluginToBeApplied) { | ||
possiblePluginNames.each { pluginName -> | ||
project.pluginManager.withPlugin(pluginName, { prerequisitePlugin -> | ||
applyWithPrerequisitePlugin (prerequisitePlugin, pluginToBeApplied) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: extra space before left parentheses.
Minor changes from comments as well
I feel this PR will take a while to evolve. It may make sense to split out the changes in the tests as a separate PR and get it in sooner. |
- Set classpath to test projects to remove the need for installing plugin before running test cases
committed another approach #102 |
Added following plugins while keeping the original plugin to determine which to apply