Pull request with changes needed by gradle discobot (groovy on android) plugin

7 views
Skip to first unread message

Marcin Erdmann

unread,
Aug 19, 2011, 5:06:20 PM8/19/11
to gradle-android-plugin-developers, erik....@jworks.nl
Dear all,

My name is Marcin Erdmann and together with Erik Pragt we're working
on Discobot project which aims at making it possible to develop
Android apps in Groovy.

I've just created a pull request (https://github.com/jvoegele/gradle-
android-plugin/pull/29) with changes needed for our gradle discobot
plugin. The changes are described in the pull request.

I would really appreciate if someone could please look at it and give
some feedback - mainly if you will accept it. I'm writing here because
shall any discussion arise it will be for sure easier to communicate
here than on github.

Best regards,
Marcin Erdmann

Ladislav Thon

unread,
Aug 19, 2011, 6:14:59 PM8/19/11
to gradle-android-p...@googlegroups.com
Only short notice that we are (at least I am :-) ) aware of this and surely will take a look at it. After few quick moments of looking at the code, I think it will get accepted without much troubles...

Btw -- are you using Gradle 0.9 or 1.0-milestone-X? Because if you are using 1.0 milestones, you should have a look at gradle-1.0m2-support branch (sorry for poor naming, it is meant to contain changes for Gradle 1.0 as a whole, not 1.0m2 only).

LT

2011/8/19 Marcin Erdmann <erd...@gmail.com>

Marcin Erdmann

unread,
Aug 19, 2011, 6:36:10 PM8/19/11
to gradle-android-p...@googlegroups.com
Hello,

Thanks for a really quick response.

Just to let you know - we're still on 0.9. The project is still very
young and we don't want to spend too much time on build tools. It works
for us on 0.9 so for now on we're using this version. And also when I
forked I don't think the gradle-1.0m2-support was there already. If it's
a blocker for you I can have a look at rebasing it onto the 1.0m2
branch, but I would rather postpone it up to the moment when we go
public with the Discobot project or up to the moment when gradle hits 1.0.

Thanks,
Marcin

On 08/20/2011 12:14 AM, Ladislav Thon wrote:
> Only short notice that we are (at least I am :-) ) aware of this and
> surely will take a look at it. After few quick moments of looking at the
> code, I think it will get accepted without much troubles...
>
> Btw -- are you using Gradle 0.9 or 1.0-milestone-X? Because if you are
> using 1.0 milestones, you should have a look at gradle-1.0m2-support
> branch (sorry for poor naming, it is meant to contain changes for Gradle
> 1.0 as a whole, not 1.0m2 only).
>
> LT
>

> 2011/8/19 Marcin Erdmann <erd...@gmail.com <mailto:erd...@gmail.com>>


>
> Dear all,
>
> My name is Marcin Erdmann and together with Erik Pragt we're working
> on Discobot project which aims at making it possible to develop
> Android apps in Groovy.
>
> I've just created a pull request (https://github.com/jvoegele/gradle-

> android-plugin/pull/29 <https://github.com/jvoegele/gradle-
> android-plugin/pull/29>) with changes needed for our gradle discobot

Ladislav Thon

unread,
Aug 19, 2011, 6:51:23 PM8/19/11
to gradle-android-p...@googlegroups.com
If it's a blocker for you I can have a look at rebasing it onto the 1.0m2 branch

Not needed, really. Just wanted to make you aware of that.

LT 

Ladislav Thon

unread,
Sep 3, 2011, 8:30:14 AM9/3/11
to gradle-android-p...@googlegroups.com
I've just created a pull request (https://github.com/jvoegele/gradle-
android-plugin/pull/29
) with changes needed for our gradle discobot
plugin. The changes are described in the pull request.

Okay, guys, I got some time and look into this. I'm sorry it took so long, but that's just the way it is.

I have no problem with those changes (heck, I even like some of them!), but there's this problem... and you've probably seen it coming... Signing configuration was moved to a new task. That itself is probably a good thing, but it is also a big backwards incompatible change in public API. I don't like that.

I guess it can be solved, either by changing the AndroidSignAndAlignTask to read its configuration from AndroidPackageTask, or by putting the respective getters and setters back to AndroidPackageTask but making them set the properties of AndroidSignAndAlignTask. Not sure which is the better way or if there is something even better to do, but I really really think we shouldn't break this API.

Apart from that, there is only one minor glitch in the way the tasks are defined in AndroidPlugin, there was a little change in the convention (nothing big, you can easily see it if you try to merge your changes into current jvoegele/master).

You might also want to know that we switched to Gradle 1.0 in master -- but it won't be a big issue for you I guess.

LT

Marcin Erdmann

unread,
Oct 3, 2011, 3:21:48 PM10/3/11
to gradle-android-p...@googlegroups.com
Sorry for this really late response, especially cause you put in some
effort into going through the pull request and there was no response
from our side. We had some crazy deadlines at work recently, I got
married and was on a honeymoon in a place where even an extremely slow
internet was a luxury. But now we're back to work on the project and we
have a talk scheduled on it for 8-9th December at Groovy & Grails
Exchange in London so we need to get it up and running by then.

On 09/03/2011 02:30 PM, Ladislav Thon wrote:
> I have no problem with those changes (heck, I even like some of them!),
> but there's this problem... and you've probably seen it coming...
> Signing configuration was moved to a new task. That itself is probably a
> good thing, but it is also a big backwards incompatible change in public
> API. I don't like that.

Nice to hear that you like some of the changes:). Yep, I was afraid that
you might not like this backward incompatible change, but no problem. I
will make it possible to set the signing configuration on both of the
tasks, but will print a deprecation message if you do it on the
AndroidPackageTask, what do you think?

> Apart from that, there is only one minor glitch in the way the tasks are
> defined in AndroidPlugin, there was a little change in the convention
> (nothing big, you can easily see it if you try to merge your changes
> into current jvoegele/master).

I will adjust the code so that it follows the task definition conventions.

Thanks again for having a look at it. I plan to deliver a new pull
request by the end of this week.

Marcin

Ladislav Thon

unread,
Oct 3, 2011, 3:31:46 PM10/3/11
to gradle-android-p...@googlegroups.com
Sorry for this really late response, especially cause you put in some effort into going through the pull request and there was no response from our side. We had some crazy deadlines at work recently, I got married and was on a honeymoon in a place where even an extremely slow internet was a luxury. But now we're back to work on the project and we have a talk scheduled on it for 8-9th December at Groovy & Grails Exchange in London so we need to get it up and running by then.

No problem on my side :-)
 
Nice to hear that you like some of the changes:). Yep, I was afraid that you might not like this backward incompatible change, but no problem. I will make it possible to set the signing configuration on both of the tasks, but will print a deprecation message if you do it on the AndroidPackageTask, what do you think?

I'm fine with that. If some of other devs have objections, hopefully they will express them here soon :-)

Thanks again for having a look at it. I plan to deliver a new pull request by the end of this week.

OK. Will have a look at it then and merge it if all is fine. Thanks for your effort on making Gradle Android plugin better, it's highly appreciated! 

LT

Marcin Erdmann

unread,
Oct 10, 2011, 2:45:42 PM10/10/11
to gradle-android-p...@googlegroups.com
One day later than promised but it's in:
https://github.com/jvoegele/gradle-android-plugin/pull/30

This new pull request includes all of Ladicek's suggestions - mainly
making it still possible to configure signing on androidPackage task
with integration tests. It also honours the latest convention of
defining tasks in AndroidPlugin class and adds gradle wrapper to the
project.

I hope that now it's acceptable;) and I'm looking forward to getting
this pull request accepted.

Thanks,
Marcin

Reply all
Reply to author
Forward
0 new messages