890586 - Packages are inconsistent with directories in the Java source

Status: RESOLVED FIXED

(Firefox for Android Graveyard :: General, defect)

Description

[Chris Kitching [:ckitching]]

In Java it is convention (And actually required by the compiler) for packages of classes to match up with relative directories. (So relative to a class in . a class in ./A is in package .A, etc.)
This convention isn't followed in a couple of places in the code - It's not a serious problem, it just means class files aren't where you'd expect them to be, causing both IDEs and interns minor sadness.

In particular:

The UpdateService and UpdateServiceHelper classes are in the directory for package org.mozilla.gecko, but are declared as being in package org.mozilla.gecko.updater.

The packages in the awesomebar directory are all declared as being in package org.mozilla.gecko, despite being in a directory that would imply their package as org.mozilla.gecko.awesomebar.
Additionally, it seems that only some of the classes related to the AwesomeBar are in this almost-but-not-quite-a package.

The json-simple library is declared as being in package org.json.simple, but lives in the directory for org.mozilla.gecko.json-simple.
The httpclienandroidlib library is declared as being in package ch.boye.httpclientandroidlib, but has been placed in org.mozilla.gecko.httpclientandroidlib.
The braille library is declared as being in package com.googlecode.eyesfree.braille.selfbraille but has really been placed in org.mozilla.gecko.braille.com.googlecode.eyesfree.braille.selfbraille.

On the topic of the libraries it's not quite clear how best to proceed - should they all be refactored into some special libraries package, should they be moved to the packages they are declared as being in, or should their declarations be changed to match their real locations in the source trees?

Or is correcting these inconsistencies not deemed pointful?

Comment 1

[Chris Kitching [:ckitching]]

Attachment: Patch to move UpdateService classes such that they match their declared packages.

Comment 2

[Chris Kitching [:ckitching]]

Attachment: Patch shifting Awesome Bar classes into the awesomebar package proper

This one is somewhat more involved than that for the UpdateServices classes - if AwesomeBar is going away in the near future due to the about:home rewrite then this perhaps isn't pointful.
This patch moves AwesomeBar classes from org.mozilla.gecko into org.mozilla.gecko.awesomebar, and sets the package declarations of classes already there to match their actual location.

Comment 3

[:Margaret Leibovic]

Comment on attachment 771733 [details] [diff] [review]
Patch to move UpdateService classes such that they match their declared packages.

This looks pretty harmless, but I'm gonna let snorp review it, since he knows more about the updater service.

Comment 4

[:Margaret Leibovic]

Comment on attachment 771734 [details] [diff] [review]
Patch shifting Awesome Bar classes into the awesomebar package proper

I don't think we should bother landing this patch, since most of this code is going to go away when bug 876712 gets merged back to m-c, and this will just cause more merge churn. Sorry for the personal churn :(

Comment 5

[:Margaret Leibovic]

(In reply to Chris Kitching [:ckitching] from comment #0)

> The json-simple library is declared as being in package org.json.simple, but
> lives in the directory for org.mozilla.gecko.json-simple.
> The httpclienandroidlib library is declared as being in package
> ch.boye.httpclientandroidlib, but has been placed in
> org.mozilla.gecko.httpclientandroidlib.
> The braille library is declared as being in package
> com.googlecode.eyesfree.braille.selfbraille but has really been placed in
> org.mozilla.gecko.braille.com.googlecode.eyesfree.braille.selfbraille.
> 
> On the topic of the libraries it's not quite clear how best to proceed -
> should they all be refactored into some special libraries package, should
> they be moved to the packages they are declared as being in, or should their
> declarations be changed to match their real locations in the source trees?
> 
> Or is correcting these inconsistencies not deemed pointful?

Cc'ing rnewman and maxli because it looks like they're the ones who've added these libraries.

Comment 6

[James Willcox (:snorp) ([e-mail]) (he/him)]

Comment on attachment 771733 [details] [diff] [review]
Patch to move UpdateService classes such that they match their declared packages.

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

I remember trying this when I first wrote it but ran into some kind of trouble. Assuming this builds, looks good to me.

Comment 7

[Richard Newman [:rnewman]]

(In reply to Chris Kitching [:ckitching] from comment #0)

> The json-simple library is declared as being in package org.json.simple, but
> lives in the directory for org.mozilla.gecko.json-simple.
>
> The httpclienandroidlib library is declared as being in package
> ch.boye.httpclientandroidlib, but has been placed in
> org.mozilla.gecko.httpclientandroidlib.

That's really just because android/base/ is implicitly o.m.g for our code, but it's also the root for "build all this stuff as Java, just like we do for our own code".

It's not correct to assume that mobile/android/base 100% follows Java directory-package conventions; this is a Make-based system that calls javac and dex directly with file paths.


> On the topic of the libraries it's not quite clear how best to proceed -
> should they all be refactored into some special libraries package, should
> they be moved to the packages they are declared as being in, or should their
> declarations be changed to match their real locations in the source trees?

I don't think introducing new tiers of directories within m/a/b for o.m.g and others would be a good idea (for lots of pragmatic reasons).

If you're willing to do the work to move the external library dependencies into another directory elsewhere in the tree (e.g., mobile/android/lib), and get a build peer to agree (:gps is a good starting point), then I'm happy with that, and would help with the android-services tooling.

I think our two options are:

* Continue to *not* treat m/a/b as o.m.g, and provide hints to IDEs to make the situation less painful for people like you.
* Move libraries to somewhere outside of m/a/b.

> Or is correcting these inconsistencies not deemed pointful?

That really depends on how easy it is to get things working with an IDE with the current state of affairs. If the answer is "impossible", and we care about IDE support, then we should do something.

Comment 8

[Chris Kitching [:ckitching]]

(In reply to Richard Newman [:rnewman] from comment #7)
> (In reply to Chris Kitching [:ckitching] from comment #0)
> 
> > The json-simple library is declared as being in package org.json.simple, but
> > lives in the directory for org.mozilla.gecko.json-simple.
> >
> > The httpclienandroidlib library is declared as being in package
> > ch.boye.httpclientandroidlib, but has been placed in
> > org.mozilla.gecko.httpclientandroidlib.
> 
> That's really just because android/base/ is implicitly o.m.g for our code,
> but it's also the root for "build all this stuff as Java, just like we do
> for our own code".

The problem here isn't that android/base is implicitly o.m.g - in fact both IDEs and the Javac compiler can be told "This directory has this package prefix". The problem is the package declarations in the libraries don't reflect that they're really living in o.m.g.whatever.the.library.is and instead just say whatever.the.library.is.



> 
> It's not correct to assume that mobile/android/base 100% follows Java
> directory-package conventions; this is a Make-based system that calls javac
> and dex directly with file paths.
> 
> 
> > On the topic of the libraries it's not quite clear how best to proceed -
> > should they all be refactored into some special libraries package, should
> > they be moved to the packages they are declared as being in, or should their
> > declarations be changed to match their real locations in the source trees?
> 
> I don't think introducing new tiers of directories within m/a/b for o.m.g
> and others would be a good idea (for lots of pragmatic reasons).
> 

This seems sensible - keeping libraries seperate from our code is a good idea - makes it more obvious which stuff one shouldn't attempt to edit.


> If you're willing to do the work to move the external library dependencies
> into another directory elsewhere in the tree (e.g., mobile/android/lib), and
> get a build peer to agree (:gps is a good starting point), then I'm happy
> with that, and would help with the android-services tooling.
> 
> I think our two options are:
> 
> * Continue to *not* treat m/a/b as o.m.g, and provide hints to IDEs to make
> the situation less painful for people like you.
> * Move libraries to somewhere outside of m/a/b.
> 

No problem doing that - I'll get on with that today.


> > Or is correcting these inconsistencies not deemed pointful?
> 
> That really depends on how easy it is to get things working with an IDE with
> the current state of affairs. If the answer is "impossible", and we care
> about IDE support, then we should do something.

So far as I can tell, IDEs can't be coaxed into understanding the current setup, but they can be taught how to deal with a slightly tweaked one using the whole "Package prefix" setting - moving the libraries as you describe would solve this problem (with my accepted patch above) (With the exception of the AwesomeBar classes, but they're going away soon so it's not important).

Comment 9

[Richard Newman [:rnewman]]

(In reply to Chris Kitching [:ckitching] from comment #8)

> > That's really just because android/base/ is implicitly o.m.g for our code,
> > but it's also the root for "build all this stuff as Java, just like we do
> > for our own code".
> 
> The problem is the package declarations in the libraries don't
> reflect that they're really living in o.m.g.whatever.the.library.is and
> instead just say whatever.the.library.is.

Yeah, that's why I included my "but it's also" -- for *our* code m/a/b implies one package prefix, and for library code it implies other package prefixes (or possibly no prefix).

Those libraries don't live in o.m.g.library; that's the (incorrect) perspective of assuming the same package prefix applies to all code in that directory.

If mainstream IDEs aren't able to handle this kind of overlapping package prefixing, then let's continue thinking about moves.

Comment 10

[Chris Kitching [:ckitching]]

Ah yes, I'm sorry, I omitted the most important part of my sentence there - mainstream IDEs do not allow overlapping package prefixes. 
Hoping to have a patch for this shortly - looks like it'll be another one needing to be committed upstream to sync, too.

A question: Why are these libraries included as source, instead of jars, if we're not modifying them? It seems like that would be the simplest option of all - I guess this is some quirk of using a Makefile based system?
Or is it so we have the freedom to possibly modify them later if we choose, or something?

Comment 11

[Richard Newman [:rnewman]]

(In reply to Chris Kitching [:ckitching] from comment #10)

> A question: Why are these libraries included as source, instead of jars, if
> we're not modifying them? It seems like that would be the simplest option of
> all - I guess this is some quirk of using a Makefile based system?
> Or is it so we have the freedom to possibly modify them later if we choose,
> or something?

A bunch of reasons.

Firstly, they have compatible licenses, so if we needed to then yes, we could fork them.

Secondly, this also means we're distributing the source that we ship, which is friendly.

Thirdly, the source we ship is preprocessed (see the horror that is android-sync/external/httpclientandroidlib/script/convert_stock_httpclient, for example), so any included jar would have to be built from our own source repos anyway. Ain't no way I want to port that shell script to work reliably on multiple platforms!

Finally, it was much easier to just add source to the build than to incorporate jars (at least, at the time this work was going on), particularly given that these were Sync's dependencies -- it's not possible to combine two separate built artifacts that both depend on the Android R.java system, which was a strike against jar delivery as a whole.

We might now have a jar-based inclusion mechanism, in which case we can revisit this decision, but it doesn't seem like a good effort tradeoff at this point.

Comment 12

[Chris Kitching [:ckitching]]

(In reply to Richard Newman [:rnewman] from comment #11)
> (In reply to Chris Kitching [:ckitching] from comment #10)
> 
> > A question: Why are these libraries included as source, instead of jars, if
> > we're not modifying them? It seems like that would be the simplest option of
> > all - I guess this is some quirk of using a Makefile based system?
> > Or is it so we have the freedom to possibly modify them later if we choose,
> > or something?
> 
> A bunch of reasons.
> 
> Firstly, they have compatible licenses, so if we needed to then yes, we
> could fork them.
> 
> Secondly, this also means we're distributing the source that we ship, which
> is friendly.
> 
> Thirdly, the source we ship is preprocessed (see the horror that is
> android-sync/external/httpclientandroidlib/script/convert_stock_httpclient,
> for example), so any included jar would have to be built from our own source
> repos anyway. Ain't no way I want to port that shell script to work reliably
> on multiple platforms!
> 
> Finally, it was much easier to just add source to the build than to
> incorporate jars (at least, at the time this work was going on),
> particularly given that these were Sync's dependencies -- it's not possible
> to combine two separate built artifacts that both depend on the Android
> R.java system, which was a strike against jar delivery as a whole.
> 
> We might now have a jar-based inclusion mechanism, in which case we can
> revisit this decision, but it doesn't seem like a good effort tradeoff at
> this point.

Indeed. That would be somewhat complicated.
Righto, then - I'll shift the files to a lib directory.

Comment 13

[Chris Kitching [:ckitching]]

Attachment: Shifting external libraries into their own, package preserving lib directory

Here's a patch shifting the libraries to a new directory, as suggested (And dealing with the resulting fallout)

Comment 14

[Chris Kitching [:ckitching]]

(In reply to Chris Kitching [:ckitching] from comment #13)
> Created attachment 772378 [details] [diff] [review]
> Shifting external libraries into their own, package preserving lib directory
> 
> Here's a patch shifting the libraries to a new directory, as suggested (And
> dealing with the resulting fallout)

It occurs to me that this is a stupendously large patch, mostly created via liberal application of sed. To aid in the preservation of reviewer sanity,

What is being done here can be summarized as follows:

Makefile.in: 

Add new 'fenneclibdir' which points to a new source directory dedicated to library sources (To keep them seperate from our code)
Add a new list of files for third party libraries (To seperate them from our own code)
Update the paths of the websockets files to represent the changes described below.

android-services-files.mk

Diff Lines 136-985: Update paths of httpclientandroidlib, simple-json to their new locations inside this new fenneclibdir.

General:

Diff lines 2558-4007: Moving library source files to this new library source path

Diff Lines 995-2557: Updating imports and javadoc comments to reflect the new package structure this movement of the library files induces.


Hope this helps! Sorry for filing a 4007 line patch for review.. (It DOES compile, so presumably this counts as "Not broken")

Comment 15

[Stefan Mirea [:smirea]]

https://tbpl.mozilla.org/?tree=Try&rev=318f44f666fc

Comment 16

[Gregory Szorc [:gps]]

Comment on attachment 772378 [details] [diff] [review]
Shifting external libraries into their own, package preserving lib directory

A lot of these touch "services" code. IIRC, much of this code is imported into the tree using an automated mechanism. I'm not familiar with that process and would like someone to sign off that said automated process won't be broken as a result of this.

Comment 17

[Richard Newman [:rnewman]]

(In reply to Gregory Szorc [:gps] from comment #16)
> Comment on attachment 772378 [details] [diff] [review]
> Shifting external libraries into their own, package preserving lib directory
> 
> A lot of these touch "services" code. IIRC, much of this code is imported
> into the tree using an automated mechanism. I'm not familiar with that
> process and would like someone to sign off that said automated process won't
> be broken as a result of this.

Just means modifying

https://github.com/mozilla-services/android-sync/blob/develop/fennec-copy-code.sh

which is straightforward. If you have a services checkout from GitHub, you can run that script yourself:

  ./fennec-copy-code.sh ~/moz/hg/mozilla-inbound

(or wherever). `hg purge; hg up -C` will restore your hg working tree if you mess up.

Happy to review a pull request for this, or myself or Nick can write it if you don't want to.

Comment 18

[Richard Newman [:rnewman]]

Comment on attachment 772378 [details] [diff] [review]
Shifting external libraries into their own, package preserving lib directory

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

General request: split the file changes out from the make changes. gps reviews the latter, and a background services peer (probably me or Nick) do the rest.

::: mobile/android/base/background/healthreport/Environment.java
@@ +11,5 @@
>  import java.util.SortedSet;
>  
>  import org.json.JSONException;
>  import org.json.JSONObject;
> +import apache.commons.codec.binary.Base64;

We prefixed many of our libs because they don't match upstream. If they *do* (please verify), then this should be org.apache.commons... if not, save the churn and keep it as o.m.

::: mobile/android/lib/apache/commons/codec/language/DoubleMetaphone.java
@@ +15,5 @@
>   * See the License for the specific language governing permissions and
>   * limitations under the License.
>   */
>  
> +package apache.commons.codec.language;

These definitely require upstream changes to preprocessing, which we probably don't want to make, so I'm going to f- this. Keep the o.m.a.

Comment 19

[Gregory Szorc [:gps]]

Comment on attachment 772378 [details] [diff] [review]
Shifting external libraries into their own, package preserving lib directory

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

I insist the java changes be split from the build system ones and that they be reviewed first, since any changes to them will likely have an impact on the build changes.

Comment 20

[Nick Alexander :nalexander [he/him]]

rnewman has expressed my opinion, more or less, but I want to add:

1) delivering Sync's dependencies (background services' dependencies) as jars is feasible, with or without renaming the internal packages.  The dependencies don't use Android resources (and in fact, recent Android build tools can be bent to handle multiple R.java files).  My prototype Eclipse project generator used the built jars.

2) there is value in sorting out o.m.g's internal packages (updater service and awesomebar).  This is never going to fly in a sane IDE.

3) I only see value in moving the third party dependencies into m/a/libs if we move more of the Java build phase into m/a (rather than m/a/b), so that these jars get built less frequently and pre-dexed.

Comment 21

[Chris Kitching [:ckitching]]

Not been able to get to this for a while - too many concurrent tasks. Unassigning to facilitate theft (Theft is encouraged).
There are shinier things to do!

Comment 22

[Chris Kitching [:ckitching]]

We could of course land the UpdateService patch, though... That one's fine.

Comment 23

[Richard Newman [:rnewman]]

Morph this bug (move the other stuff to a follow-up) and land it.

Comment 24

[Nick Alexander :nalexander [he/him]]

Now that Bug 1107811 is landed, our packages are consistent.  Huzzah!