1119365 - Implement |mach ide| for mobile/android and IntelliJ/Android Studio

Status: RESOLVED FIXED

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

Description

[Nick Alexander :nalexander [he/him]]

In [1], there's an all-in-one |mach ide| command that prepares and launches an appropriate IDE.  This ticket tracks adding an "intellij" (and maybe "android studio") command to the list of supported IDEs.

Conceptually, this needs to:

1) |mach build|
2) |mach gradle-install|
3) open IntelliJ/Android Studio pointing at the $OBJDIR/mobile/android/gradle directory.

To achieve this, add a few choices to the "ide" argument at [1], add a new function implementing the logic above (crib heavily from what exists for Eclipse/Visual Studio, but please try to handle both Linux and Mac OS X when launching the IDE), and call your new function when the "ide" argument is one of the relevant ones.

[1] https://dxr.mozilla.org/mozilla-central/source/python/mozbuild/mozbuild/backend/mach_commands.py#30

Comment 1

[Ahmed Khalil]

After I added few lines of code to handle the androidstudio case, The script fails in https://dxr.mozilla.org/mozilla-central/source/python/mozbuild/mozbuild/backend/mach_commands.py#59 with error http://pastebin.com/JX1N07Kj . So my question should I add a new backend ? `f yes, Where is the file that I should add the backend in ?

Comment 2

[Nick Alexander :nalexander [he/him]]

(In reply to Ahmed Khalil from comment #1)
> After I added few lines of code to handle the androidstudio case, The script
> fails in
> https://dxr.mozilla.org/mozilla-central/source/python/mozbuild/mozbuild/
> backend/mach_commands.py#59 with error http://pastebin.com/JX1N07Kj . So my
> question should I add a new backend ? `f yes, Where is the file that I
> should add the backend in ?

Hey, sorry for the delayed reply.  You should not need to add a new backend.  Can you post a patch with your changes, so that I can see how you're invoking |mach build| or |mach build-backend|?

Comment 3

[Ahmed Khalil]

Attachment: mach_ide.patch

Here is the changed lines that you requested

Comment 4

[Nick Alexander :nalexander [he/him]]

Attachment: MozReview Request: bz://1119365/nalexander

/r/3951 - Bug 1119365 - "Implement |mach ide| for mobile/android and IntelliJ/Android Studio" [f=ahmedibrahimkhali]

Pull down this commit:

hg pull review -r 518ef16f8193880075c6b5ac374cc43f74a6d89a

Comment 5

[Nick Alexander :nalexander [he/him]]

Hey Ahmed!  I started playing with your commit and ended up building some of the extra functionality.  Can you take a look at the commit I posted above, see how it works for you, and give me some feedback?

Comment 6

[Nick Alexander :nalexander [he/him]]

Comment on attachment 8564239 [details] [diff] [review]
mach_ide.patch

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

Hey!  This is a good start -- I posted an improved version for you to take a look at.

Comment 7

[Ahmed Khalil]

Attachment: mach_commands.patch

I've augmented your patch and added "intellij" ide entry.
As we discussed on the IRC, when "intellij" is used Intellij entries are preferred over Android Studio with the opposite when using 'androidstudio' ide.
Please note that this behavior is only followed when on Mac unlike on Linux where the corresponding ide is used directly without any preferences.

Comment 8

[Nick Alexander :nalexander [he/him]]

Comment on attachment 8566825 [details] [diff] [review]
mach_commands.patch

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

This is looking really good!  Can you post a new single patch (against fx-team or mozilla-central) with my small comments, and then I'll test and land.  Thanks for your work!

I wonder if you would be interested in helping with another Python thing -- adding a mach command to install the MOZ_HOST_BIN utilities.  If you're interested, ping me in Bug 1126424.  Thanks again!

::: python/mozbuild/mozbuild/backend/mach_commands.py
@@ +106,5 @@
>          return os.path.join(self.topobjdir, 'msvc', 'mozilla.sln')
>  
>      def get_gradle_path(self):
>          return os.path.join(self.topobjdir, 'mobile', 'android', 'gradle', 'build.gradle')
> +    

nit: delete trailing whitespace through-out.

@@ +109,5 @@
>          return os.path.join(self.topobjdir, 'mobile', 'android', 'gradle', 'build.gradle')
> +    
> +    def get_mac_ide_preferences(self, ide):
> +        if sys.platform == 'darwin':
> +            android_studio_prefs = ['/Applications/Android Studio.app'];

Why don't we make these lists AS and IJ specific -- so just the one location for AS, and 4 locations for IJ.  (But not mixing the lists.)

Comment 9

[Ahmed Khalil]

Attachment: mach_commands.patch

Here is a single patch rebased on the original version

Comment 10

[Ahmed Khalil]

Attachment: mach_commands.patch

Removed a trailing white space

Comment 11

[Ahmed Khalil]

Hi Nick, any updates on the review ?

Comment 12

[Nick Alexander :nalexander [he/him]]

(In reply to Ahmed Khalil from comment #11)
> Hi Nick, any updates on the review ?

Hey!  Sorry for the long delay -- I've been traveling, sick, and building Firefox for iOS.  Always feel free to ping me.

The code looks good, but my local testing showed some odd behaviour.  In the version I pushed to you, I opened $OBJDIR/mobile/android/gradle/build.gradle.  That does the right thing when the project hasn't been imported yet, but not when it has.  Can you investigate opening $OBJDIR/mobile/android/gradle/.idea/SOMETHING if it exists, and build.gradle otherwise?  What we want is to import if we haven't already.  (To test this, you can just delete .idea locally.)

Let me know what you discover.  It would be nice to get this working smoothly, but if not, what we have is a big improvement.

Comment 13

[Nick Alexander :nalexander [he/him]]

Comment on attachment 8567070 [details] [diff] [review]
mach_commands.patch

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

This code looks great!  I've asked for a functionality change (or at least investigation) -- let's see where that ends up.

Thanks, Ahmed!

Comment 14

[Ahmed Khalil]

Hi Nick,
I investigated what you said, and you are right. the ide commands tries to reimport project although it is already imported.

I think to fix this issue, I'll check if "$OBJDIR/mobile/android/gradle/.idea" already exists or not.

If yes then I'll issue the command with "$OBJDIR/mobile/android/gradle" directory and the IDE will open without the import dialog.

I'll post another patch in the next few minutes.

Comment 15

[Ahmed Khalil]

Attachment: mach_command_fix.patch

This patch fixes the reimporting issue.

Comment 16

[Nick Alexander :nalexander [he/him]]

Comment on attachment 8570462 [details] [diff] [review]
mach_command_fix.patch

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

Very nice!  I'll land this with just a minor tweak (I want to include build_gradle in one of the function names).  Thanks for working on this!

Comment 17

[Nick Alexander :nalexander [he/him]]

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

Comment 18

[Nick Alexander :nalexander [he/him]]

Ahmed: your Python seems very strong.  I have two rather unusual Python projects, if you're interested -- one is re-working the GeckoView library packaging to upload an AAR file, for use from Gradle.  I wrote a job to do this, and a blog post about it at http://www.ncalexander.net/blog/2014/07/10/build-your-own-browser-a-maven-repository-for-geckoview/, and now it makes sense to do it as part of the build.  This would be a follow-up to Bug 1093242.  I don't think I have a ticket tracking this change yet.

The other is much more open-ended: it has to do with downloading the Gecko libraries without compiling them.  It's also a follow-up to Bug 1093242, and it's very loosely tracked by Bug 1124378 and some work in progress code in my tree.  This ticket might involve writing: a Mercurial extension; a web service; a Mozilla Pulse aggregator; a mach command; and some build system code.  It's pretty open-ended :)

Comment 19

[Ahmed Khalil]

Hi Nick,
The two projects that you've mentioned looks very appealing to me. I guess I'm going to start with first one, and I would be happy if you mentored me across it.

I've seen Bug 1093242 and seems that you've done the script that uploads the aar files, so what still needs to be done is to execute that script during the build and packaging phases, am I right ?

About the my last uploaded patch, do you just need to include build_gradle in get_gradle_path for example ,to be get_build_gradle_path ?

Comment 20

[Nick Alexander :nalexander [he/him]]

(In reply to Ahmed Khalil from comment #19)
> Hi Nick,
> The two projects that you've mentioned looks very appealing to me. I guess
> I'm going to start with first one, and I would be happy if you mentored me
> across it.

Great!  I will file a ticket to set some direction.  Ping me if I lag on this :)
 
> I've seen Bug 1093242 and seems that you've done the script that uploads the
> aar files, so what still needs to be done is to execute that script during
> the build and packaging phases, am I right ?
> 
> About the my last uploaded patch, do you just need to include build_gradle
> in get_gradle_path for example ,to be get_build_gradle_path ?

Yep, pretty much.  I did this and landed the patch with the slight modifications.

Comment 21

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/352ebcbf802e

Comment 22

[Nick Alexander :nalexander [he/him]]

This needs documentation in the mobile wiki.  NI to me for that.

Comment 23

[Nick Alexander :nalexander [he/him]]

(In reply to Nick Alexander :nalexander from comment #22)
> This needs documentation in the mobile wiki.  NI to me for that.

Finally added a section: https://wiki.mozilla.org/Mobile/Fennec/Android/IDEs#Really_quick_start

I ran into some problems with IntelliJ resolving symlinked paths, so we may need to update this |mach ide| command or the documentation.

Comment 25

[Nick Alexander :nalexander [he/him]]

Attachment: MozReview Request: Bug 1119365 - "Implement |mach ide| for mobile/android and IntelliJ/Android Studio" [f=ahmedibrahimkhali]

Comment 26

[Eric Shepherd [:sheppy]]

Is the information in https://wiki.mozilla.org/Mobile/Fennec/Android/IDEs#Really_quick_start stuff that should be reflected into the MDN article here: https://developer.mozilla.org/en-US/docs/Mozilla/Developer_guide/mach? If so, then it would be great if nalexander could help with that; otherwise, I'll look into it when I can.

Are there any bugs about fixing the problems mentioned in c#23? If so, I need to look at those, so I can provide information about when they were corrected (if they have been).

Comment 27

[Nick Alexander :nalexander [he/him]]

Sorry for the delayed reply.

(In reply to Eric Shepherd [:sheppy] from comment #26)
> Is the information in
> https://wiki.mozilla.org/Mobile/Fennec/Android/IDEs#Really_quick_start stuff
> that should be reflected into the MDN article here:
> https://developer.mozilla.org/en-US/docs/Mozilla/Developer_guide/mach? If
> so, then it would be great if nalexander could help with that; otherwise,
> I'll look into it when I can.

Two things: Mobile team has debated moving documentation into MDN a lot.  Our Wiki based system is working for us, and unifying similar documentation is a time sync, so we're pretty happy not to do it.

As for the stuff in this ticket: it could be added to documentation, but |mach ide| hasn't had the testing and love of other methods, so we're happy to leave it out.

> Are there any bugs about fixing the problems mentioned in c#23? If so, I
> need to look at those, so I can provide information about when they were
> corrected (if they have been).

No, there are no bugs; no, I don't think it's relevant to anybody as yet.  It's very specific to IntelliJ: you need to manually refresh.  There are other things that will remove that, but they've nothing to do with |mach ide|.

Comment 28

[Nick Alexander :nalexander [he/him]]

I'm going to remove the dev-doc-needed flag: we're happy with this being off the beaten path.  I have some changes to the Android Gradle/IntelliJ integration that make |mach ide| more interesting.

Comment 29

[Eric Shepherd [:sheppy]]

If we get to a point where documenting this on MDN would be worthwhile, let us know!