1041395 - (intellij) Add Gradle configuration for building mobile/android

Status: RESOLVED FIXED

(Firefox Build System :: Android Studio and Gradle Integration, defect)

Description

[Nick Alexander :nalexander [he/him]]

A long sequence of bugs starting with Bug 853045 landed support for creating Eclipse project files for mobile/android, and for building and debugging entirely from Eclipse.  (At least, building and debugging the Java and JavaScript layer of Fennec.  Building the C/C++ layer is a non-goal of this work.)

Let's apply some of the lessons learned to do the same for Android Studio/IDEA IntelliJ.

Comment 1

[Nick Alexander :nalexander [he/him]]

Attachment: Pre: Fix a small encoding issue in a comment. r=me

These square brackets confuse the IDEA/IntelliJ XML parser.  Perhaps
square and brackets are expected to be escaped in comments, although I
can't find documentation saying that square brackets are illegal or need
to be escaped in XML comments, and a quick search didn't find an
existing JetBrains issue.

Comment 2

[Nick Alexander :nalexander [he/him]]

Attachment: Part 1: First steps.

Comment 3

[Nick Alexander :nalexander [he/him]]

Attachment: Part 2: Prepare the build directory before continuing with the Gradle build.

Comment 4

[Nick Alexander :nalexander [he/him]]

Here is some work-in-progress that creates a working Gradle build, and a working Android Studio project structure, at least locally.

This uses the same |mach build-backend -b=AndroidStudio| structure, and is a terrible copy-paste of the existing backend.  Unifying this will take some effort but will simplify things greatly.

Unfortunately, this work isn't complete -- you'll need to extract [1] into $OBJDIR/android_studio (after running |mach build-backend -b=AndroidStudio|) to get the top-level structure.  After that, |gradle clean build -x lint| in $OBJDIR/android_studio should build Fennec.  You should be able to import $OBJDIR/android_studio/Fennec into Android Studio and have it do the right thing with the other projects, but this is still not an area I understand well.

[1] http://people.mozilla.org/~nalexander/android_studio.tar.gz

Comment 5

[Chris Kitching [:ckitching]]

I'd love to try this out, but your patches are all hideously bitrotted. Since several of the lines you're changing in these patches don't look anything like they did when you generated your patches it's not trivial to unbitrot myself, alas :/

Still, I'm extremely happy you're doing this: making it build with IDEA is the main thing I've never managed to get it doing. Apologies for utter silence on this front for several weeks. Sans building the rest of the config isn't too awful. Hopefully you can plagerise mine somewhat.

I'm not particularly happy about keeping the IDEA configuration stuff in the objdir. I very much don't want `mach clobber` to destroy my project-specific settings (some of which can be user-specific, such as the inspection profile or code formatting profile in use).

Naturally, we also don't want Mercurial tracking it, either, so the solution to that problem I use is to use a secondary hgignore file.
Mercurial doesn't track files in .hg, so you create a file at .hg/hgignore containing extra ignores, in my case:

.idea
^.+\.iml$

and then add

[ui]
ignore = full-path-to-repo/.hg/hgignore

to .hg/hgrc

That said, now this is "official" we could probably stick the above in the real .hgignore and be done with it.



Alas, I can't really say a lot more until I can try it out. I'm particularly keen to try this out, because some of the more fun inspections only work once IDEA understands the whole build process. It would be excellent, for example, to have it highlight unused resources for us (something I've yet to be able to make it to without throwing an NPE).

Here are the files that make up my current config. Note that it's excessively hacky, extended as I needed to work in different regions of the code, and lacks all the C/C++ code (though I got that working last year, I managed to accidentally delete it a few months back and never needed to rebuild it since. I should get on that soonish.):

https://www.dropbox.com/s/5qtebaskxs4vs1u/intellij.tar.gz

Hopefully this helps!

Comment 6

[Nick Alexander :nalexander [he/him]]

Hi Chris!

Good comments, all.  Some responses inline.

(In reply to Chris Kitching [:ckitching] from comment #5)
> I'd love to try this out, but your patches are all hideously bitrotted.
> Since several of the lines you're changing in these patches don't look
> anything like they did when you generated your patches it's not trivial to
> unbitrot myself, alas :/

I'll rebase and push my git tree, then future rebases will be easier :)

> Still, I'm extremely happy you're doing this: making it build with IDEA is
> the main thing I've never managed to get it doing. Apologies for utter
> silence on this front for several weeks. Sans building the rest of the
> config isn't too awful. Hopefully you can plagerise mine somewhat.
> 
> I'm not particularly happy about keeping the IDEA configuration stuff in the
> objdir. I very much don't want `mach clobber` to destroy my project-specific
> settings (some of which can be user-specific, such as the inspection profile
> or code formatting profile in use).

Neither am I, but we're rather constrained.  The object directory changes, and we have *lots* of multi-objdir users (including me!), so we need a location that depends on the objdir.  So the srcdir is out, and in any case, writing to srcdir is strictly forbidden.  (It's too easy to make errors; we've busted things badly before, so badly that some tools now refuse to write into the srcdir.)

|mach| has some limited code to create a ~/.build_state directory or similar.  I would have written there except there's mankiness around the build-backend commands not actually being real mach commands, and I couldn't fix all the things at once.  I'd love to see this get some love.

In practice, the Eclipse project files are write-only.  That is: re-creating them is trivial and doesn't impact the development flow.  So it's irritating but fully functional.  I see no reason to do otherwise for Android Studio.

> Naturally, we also don't want Mercurial tracking it, either, so the solution
> to that problem I use is to use a secondary hgignore file.
> Mercurial doesn't track files in .hg, so you create a file at .hg/hgignore
> containing extra ignores, in my case:
> 
> .idea
> ^.+\.iml$

These (or older versions of these) already exist.  It's a pain, 'cuz I have to commit my templates separately :)

> and then add
> 
> [ui]
> ignore = full-path-to-repo/.hg/hgignore
> 
> to .hg/hgrc
> 
> That said, now this is "official" we could probably stick the above in the
> real .hgignore and be done with it.
> 
> 
> 
> Alas, I can't really say a lot more until I can try it out. I'm particularly
> keen to try this out, because some of the more fun inspections only work
> once IDEA understands the whole build process. It would be excellent, for
> example, to have it highlight unused resources for us (something I've yet to
> be able to make it to without throwing an NPE).
> 
> Here are the files that make up my current config. Note that it's
> excessively hacky, extended as I needed to work in different regions of the
> code, and lacks all the C/C++ code (though I got that working last year, I
> managed to accidentally delete it a few months back and never needed to
> rebuild it since. I should get on that soonish.):
> 
> https://www.dropbox.com/s/5qtebaskxs4vs1u/intellij.tar.gz

Thanks for this, I'll try to look at it.

Comment 7

[Chris Kitching [:ckitching]]

(In reply to Nick Alexander :nalexander [away July 22 - Aug 12] from comment #6)
> > I'm not particularly happy about keeping the IDEA configuration stuff in the
> > objdir. I very much don't want `mach clobber` to destroy my project-specific
> > settings (some of which can be user-specific, such as the inspection profile
> > or code formatting profile in use).
> 
> Neither am I, but we're rather constrained.  The object directory changes,
> and we have *lots* of multi-objdir users (including me!), so we need a
> location that depends on the objdir. 

We do... for the build config files.
Deleting such things as project-specific editor settings, inspection profiles configurations (which will differ between users greatly, as the default is very toned-down and "everything on" is very computationally expensive) each time someone rebuilds is undesirable. I think this is a problem that IDEA suffers from moreso than Eclipse, which (I think?) has less project-associated IDE config stuff.

> So the srcdir is out, and in any case,
> writing to srcdir is strictly forbidden.  (It's too easy to make errors;
> we've busted things badly before, so badly that some tools now refuse to
> write into the srcdir.)

Hmm. That is a sensible policy, so I why not stick build-specific IDEA configuration in the objdir (stuff that the user will never want to change: the generated "magic sauce"), and project-specific stuff (which users plausibly want to tinker with) somewhere else, ~/.mozilla or something, and symlink it into $OBJDIR/.idea.
That way, you can both have your cake and eat it. Since IDEA scatters its config around in a bunch of different xml files, I think you can achieve this exclusively with symlinks (no config files would need merging), so it shouldn't make your life too much more difficult...

Sorry to go on about this, but I think your current proposal would seriously harm the usefulness of the tool. I'm quite attached to my per-project inspection profiles :P.

> In practice, the Eclipse project files are write-only.  That is: re-creating
> them is trivial and doesn't impact the development flow.  So it's irritating
> but fully functional.  I see no reason to do otherwise for Android Studio.

I claim that IDEA's greater propensity for having project-specific settings that a user plausibly wants to set differently to other workers on the same project are such a reason.

Comment 8

[Nick Alexander :nalexander [he/him]]

(In reply to Chris Kitching [:ckitching] from comment #7)
> (In reply to Nick Alexander :nalexander [away July 22 - Aug 12] from comment
> #6)
> > > I'm not particularly happy about keeping the IDEA configuration stuff in the
> > > objdir. I very much don't want `mach clobber` to destroy my project-specific
> > > settings (some of which can be user-specific, such as the inspection profile
> > > or code formatting profile in use).
> > 
> > Neither am I, but we're rather constrained.  The object directory changes,
> > and we have *lots* of multi-objdir users (including me!), so we need a
> > location that depends on the objdir. 
> 
> We do... for the build config files.
> Deleting such things as project-specific editor settings, inspection
> profiles configurations (which will differ between users greatly, as the
> default is very toned-down and "everything on" is very computationally
> expensive) each time someone rebuilds is undesirable. I think this is a
> problem that IDEA suffers from moreso than Eclipse, which (I think?) has
> less project-associated IDE config stuff.
> 
> > So the srcdir is out, and in any case,
> > writing to srcdir is strictly forbidden.  (It's too easy to make errors;
> > we've busted things badly before, so badly that some tools now refuse to
> > write into the srcdir.)
> 
> Hmm. That is a sensible policy, so I why not stick build-specific IDEA
> configuration in the objdir (stuff that the user will never want to change:
> the generated "magic sauce"), and project-specific stuff (which users
> plausibly want to tinker with) somewhere else, ~/.mozilla or something, and
> symlink it into $OBJDIR/.idea.
> That way, you can both have your cake and eat it. Since IDEA scatters its
> config around in a bunch of different xml files, I think you can achieve
> this exclusively with symlinks (no config files would need merging), so it
> shouldn't make your life too much more difficult...

Some of this worries me: I really want all Android Studio devs to have the "best practices" installed and updated by default.  Things like formatting defaults shouldn't be per-developer.

> Sorry to go on about this, but I think your current proposal would seriously
> harm the usefulness of the tool. I'm quite attached to my per-project
> inspection profiles :P.
> 
> > In practice, the Eclipse project files are write-only.  That is: re-creating
> > them is trivial and doesn't impact the development flow.  So it's irritating
> > but fully functional.  I see no reason to do otherwise for Android Studio.
> 
> I claim that IDEA's greater propensity for having project-specific settings
> that a user plausibly wants to set differently to other workers on the same
> project are such a reason.

OK, thanks for this feedback.  It looks like the master .idea directory is all that would need to preserved (the .iml files are a lot closer to build configuration).  If we can plop the .idea down out of tree, I'm all for it.

Comment 9

[Nick Alexander :nalexander [he/him]]

Attachment: WIP on an in-tree Gradle configuration.

Modify mobile/android/gradle/gradle.properties to point to your
topsrcdir and topobjdir.

In mobile/android/gradle, execute |./gradlew build|.  You should find
artifacts in ${topobjdir}/gradle/fennec/*apk.

ckitching: you'll need a complete build and package in place to get a
good APK out of this.

Comment 10

[Chris Kitching [:ckitching]]

Comment on attachment 8501868 [details] [diff] [review]
WIP on an in-tree Gradle configuration.

I am a massive, massive fan.

As I mentioned on IRC, the only real problem I've found is that it doesn't correctly detect package prefixes inconsistent with directory structures. We can just fix that up manually (or you can do a clever thing).

In any case, the ensuing configuration provides even better support for IDEA's advanced code editing and analysis features than my manual configuration does, and you can build it and use the debugger to boot.

It's like Christmas.

Why couldn't you have done this while I still worked for Mozilla?! :P

Comment 11

[Martyn Haigh (:mhaigh)]

There's a bit of rot on this, I don't suppose you could update?

Also - what do we need to do to land this?

Comment 12

[Chris Kitching [:ckitching]]

I've been happily using the last version posted here for a little while now. Nick will know the details, but it seems that victory is near.
I think at least some of the rot is due to some chunks landing with different bugs already.

While I'm here: Nick, did you notice the problem with the Gradle import in which IDEA fails to configure the libraries correctly? It ends up with module dependencies for all the right things (such as support v4), but these dependencies point to nonexistent project libraries. Manually fixing this is insufficient, as idea clobbers settings given by cradle on each project reload (and for something as important as libraries that's kinda a big deal)
If this isn't clear enough I can provide some more details and screenshots etc. when I'm not on a train.

Comment 13

[Nick Alexander :nalexander [he/him]]

Attachment: Add gradle configuration. r=gps

I'll post a link to a blog post talking about how this works and
demonstrating the new functionality.

Comment 14

[Nick Alexander :nalexander [he/him]]

Attachment: Add gradle configuration. r=gps

Just a few minor fixes and re-organizations.

Comment 15

[Nick Alexander :nalexander [he/him]]

Blog post is up at

http://www.ncalexander.net/blog/2014/10/23/building-fennec-with-gradle-and-intellij-first-steps/

Unfortunately, Vimeo appears not to have recognized my screencast, so that'll have to wait until tomorrow.

Comment 16

[Nick Alexander :nalexander [he/him]]

(In reply to Nick Alexander :nalexander from comment #15)
> Blog post is up at
> 
> http://www.ncalexander.net/blog/2014/10/23/building-fennec-with-gradle-and-
> intellij-first-steps/
> 
> Unfortunately, Vimeo appears not to have recognized my screencast, so
> that'll have to wait until tomorrow.

Spoke too soon.  Demonstration is now included in the blog post.

Comment 17

[Gregory Szorc [:gps]]

Comment on attachment 8510838 [details] [diff] [review]
Add gradle configuration. r=gps

Review of attachment 8510838 [details] [diff] [review]:
-----------------------------------------------------------------

This is a 90% rubber stamp review. Please track me down IRL before landing this.

Also, please write up an in-tree README before you land this.

::: mobile/android/gradle/gradlew
@@ +149,5 @@
> +        (5) set -- "$args0" "$args1" "$args2" "$args3" "$args4" ;;
> +        (6) set -- "$args0" "$args1" "$args2" "$args3" "$args4" "$args5" ;;
> +        (7) set -- "$args0" "$args1" "$args2" "$args3" "$args4" "$args5" "$args6" ;;
> +        (8) set -- "$args0" "$args1" "$args2" "$args3" "$args4" "$args5" "$args6" "$args7" ;;
> +        (9) set -- "$args0" "$args1" "$args2" "$args3" "$args4" "$args5" "$args6" "$args7" "$args8" ;;

lol wut

::: mobile/android/moz.build
@@ +23,5 @@
>      'app',
>      'fonts',
>      'geckoview_library',
>      'extensions',
> +    'gradle',

Should this be optional?

Comment 18

[Mike Hommey [:glandium]]

Comment on attachment 8510838 [details] [diff] [review]
Add gradle configuration. r=gps

Review of attachment 8510838 [details] [diff] [review]:
-----------------------------------------------------------------

::: mobile/android/gradle/gradlew
@@ +1,1 @@
> +#!/usr/bin/env bash

We should avoid requiring bash.

@@ +137,5 @@
> +            eval `echo args$i`=`cygpath --path --ignore --mixed "$arg"`
> +        else
> +            eval `echo args$i`="\"$arg\""
> +        fi
> +        i=$((i+1))

You can do this in a much simpler way and without the limited-to-9-args thing:

for arg in "$@"; do
    (...)
    set -- "$@" "$munged_arg"
done

Comment 19

[Nick Alexander :nalexander [he/him]]

(In reply to Mike Hommey [:glandium] from comment #18)
> Comment on attachment 8510838 [details] [diff] [review]
> Add gradle configuration. r=gps
> 
> Review of attachment 8510838 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: mobile/android/gradle/gradlew
> @@ +1,1 @@
> > +#!/usr/bin/env bash
> 
> We should avoid requiring bash.

We only support building on Mac and Linux, and I think those both can expect bash.  More below.

> @@ +137,5 @@
> > +            eval `echo args$i`=`cygpath --path --ignore --mixed "$arg"`
> > +        else
> > +            eval `echo args$i`="\"$arg\""
> > +        fi
> > +        i=$((i+1))
> 
> You can do this in a much simpler way and without the limited-to-9-args
> thing:
> 
> for arg in "$@"; do
>     (...)
>     set -- "$@" "$munged_arg"
> done

This is not my code; this is the gradle wrapper as generated by the android tool (and also, I think, as distributed by gradle).  We can modify it, but we should expect to regenerate it (and check it in) periodically.

I don't care to make sure this wrapper is sh compatible.

Comment 20

[Nick Alexander :nalexander [he/him]]

https://hg.mozilla.org/integration/fx-team/rev/bb8831e29fe2

Comment 21

[Nick Alexander :nalexander [he/him]]

I pushed a somewhat evolved version of this patch, with in tree docs.  This is going to evolve a good bit and I already delayed a lot, so bombs away!

Comment 22

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/bb8831e29fe2