Closed Bug 1355870 Opened 3 years ago Closed 3 years ago

Provide a way for distributions to set a custom path to customizations

Categories

(Firefox for Android :: Android partner distribution, defect)

54 Branch
defect
Not set

Tracking

()

RESOLVED FIXED
Firefox 55
Tracking Status
firefox53 + fixed
firefox54 --- fixed
firefox55 --- fixed

People

(Reporter: mkaply, Assigned: mkaply)

References

Details

Attachments

(1 file, 3 obsolete files)

We are getting requests from partners to use a location other than /system/ for customizations.

Plan is to use a system property to allow the directory to be specified and use that instead of system.

We will not use system as a backup if the property is set.
Attached patch Simple patch (obsolete) — Splinter Review
So this is how to do it. It's ugly, but it's pretty much the only way.

Looking for overall feedback. Should I move this to a separate function?
Attachment #8857523 - Flags: feedback?(nalexander)
Comment on attachment 8857523 [details] [diff] [review]
Simple patch

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

This is all just style, but yes -- extract this to a function, extract the special "ro.org.mozilla.distdir" string to a named constant, and include in the docstring comment some details about the fact that the property is readonly and that the method for getting it is not exposed in the SDK.

Then keep `baseDirectory` final but branch on the existence of the property and being able to read it (that is, log and return null in your catch if there's an error).  That is:
```
final String systemPropertyBaseDirectory = getDistributionDirectoryFromSystemProperty();
final String baseDirectory;
if (systemPropertyBaseDirectory != null && ... (exists?)) {
    baseDirectory = ...
} else {
    baseDirectory = "/system/" + context.getPackageName() + "/distribution";
}
return getDistributionDirectoriesFromBaseDirectory(context, baseDirectory);
```

But the approach is fine.
Attachment #8857523 - Flags: feedback?(nalexander) → feedback+
What is your opinion on ro.org.mozilla.distdir?

I hate coming up with things that will be used forever. What if I name it wrong? :)
(In reply to Mike Kaply [:mkaply] from comment #3)
> What is your opinion on ro.org.mozilla.distdir?
> 
> I hate coming up with things that will be used forever. What if I name it
> wrong? :)

I have no strong opinion; rnewman might.  `ro.org.mozilla` is a good prefix; I'd prefer `distribution.dir` rather than `distdir` since `dist` means something to Gecko hackers.  Will you ever care about different distributions for different Firefox channels?  (Probably not, since we only ship Release with partners.)  You might go `ro.org.mozilla.firefox.distribution.dir` to leave open the possibility of having package-specific directories.

rnewman, do you wish to weigh in?
Flags: needinfo?(rnewman)
Attached patch 1355870.diff (obsolete) — Splinter Review
I'm not using mozreview because the patch is against release right now.

There's some time sensitivity to this, so as you review, please consider "Would I be 100% OK with this going in a 53 point release if it at all possible." :)

I guess in theory it adds a startup hit, but I don't know how else we would do it.
Attachment #8857523 - Attachment is obsolete: true
Attachment #8857641 - Flags: review?(nalexander)
> Will you ever care about different distributions for different Firefox channels?  (Probably not, since we only ship Release with partners.)  You might go `ro.org.mozilla.firefox.distribution.dir` to leave open the possibility of having package-specific directories.

I thought about that, but we already append the package onto the end of the custom dir, so I figured it wasn't really necessary.

I will change to distributiondir.
Comment on attachment 8857641 [details] [diff] [review]
1355870.diff

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

Nits and a few substantive things.

::: mobile/android/base/java/org/mozilla/gecko/distribution/Distribution.java
@@ +82,5 @@
>      private static final String HISTOGRAM_REFERRER_INVALID = "FENNEC_DISTRIBUTION_REFERRER_INVALID";
>      private static final String HISTOGRAM_DOWNLOAD_TIME_MS = "FENNEC_DISTRIBUTION_DOWNLOAD_TIME_MS";
>      private static final String HISTOGRAM_CODE_CATEGORY = "FENNEC_DISTRIBUTION_CODE_CATEGORY";
>  
> +    // Name of system property for stored distribution directory

nit: full sentence comments, here and below.

@@ +927,5 @@
>      }
>  
>      /**
> +     * Checks to see if a custom distribution directory has been set as a
> +     * system property and returns it if it exists. Works with or without

nit: rather than "Works with...", say something like, "The path returned will have a single trailing slash." -- and then remove extra slashes if they are present.  We might have this functionality in one of the utility classes already.

@@ +943,5 @@
> +            Class clazz = Class.forName("android.os.SystemProperties");
> +            @SuppressWarnings("unchecked")
> +            Method method = clazz.getDeclaredMethod("get", String.class);
> +            String customDistDirName = (String)method.invoke(null, DISTDIR_SYSTEM_PROPERTY);
> +            Log.d(LOGTAG, "Custom distribution directory is " + customDistDirName);

Imagine you're debugging from logs.  Don't you want to know how this was found (from the system property), and what system property you checked?  Both in the found case and in the not found case.

@@ +945,5 @@
> +            Method method = clazz.getDeclaredMethod("get", String.class);
> +            String customDistDirName = (String)method.invoke(null, DISTDIR_SYSTEM_PROPERTY);
> +            Log.d(LOGTAG, "Custom distribution directory is " + customDistDirName);
> +            // Add a trailing slash if it isn't there
> +            if(customDistDirName.charAt(customDistDirName.length() - 1) != '/') {

nit: this doesn't follow our style guide, and isn't even internally consistent.  `if (` with a space after.

Also, you dropped the `isEmpty()` check, so this can now have an invalid index (0 - 1).

@@ +953,5 @@
> +            if (customDistDir.exists() && customDistDir.isDirectory()) {
> +                Log.d(LOGTAG, "Custom distribution directory exists");
> +                return customDistDirName;
> +            }
> +        }catch (Exception e){

nit: style is `} catch` with a space between.  Don't you want to log an error, if you really get one?  (I'm assuming "not present" or "not a directory" isn't an error."
Attachment #8857641 - Flags: review?(nalexander) → review-
(In reply to Nick Alexander :nalexander from comment #4)

> rnewman, do you wish to weigh in?

I agree 100% with your comments.
Flags: needinfo?(rnewman)
Thank you for your comments.

I've addressed all your comments. I couldn't find a mechanism to reduce trailing slashes, but there is a way to normalize, but you still have to add the trailing slash if it is not there.

I decided in the failing case (whether the property was there or not) to just say "Custom distribution directory not found." That should be clear that either the property didn't exist, or the path doesn't.

We'll know that the path doesn't exist because we'll see the message that the property was found.

If there is ever a property, we'll show it in debug, so we should be good on the logs.
Attachment #8857641 - Attachment is obsolete: true
Attachment #8857979 - Flags: review?(nalexander)
Comment on attachment 8857979 [details] [diff] [review]
Address review comments

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

Very good!

::: mobile/android/base/java/org/mozilla/gecko/distribution/Distribution.java
@@ +947,5 @@
> +            String distDirName = (String)method.invoke(null, SYSPROP_DISTRIBUTIONDIR);
> +            if (!distDirName.isEmpty()) {
> +                Log.d(LOGTAG, "System property " + SYSPROP_DISTRIBUTIONDIR + " found with value " + distDirName);
> +                // Remove multiple trailing slashes if present
> +                distDirName = new URI(distDirName).normalize().getPath();

I expect this will work, but it adds difficult to reason about functionality.  (I.e., are you really certain that `.` and `..` are handled as you expect?  URI normalization has interesting wrinkles.)  Just work backwards and trim the string.
Attachment #8857979 - Flags: review?(nalexander) → review+
Comment on attachment 8858095 [details]
Bug #1355870 - Allow a system preference to determine distribution dir.

https://reviewboard.mozilla.org/r/130062/#review132728

nalexander already r= in the bug.
Attachment #8858095 - Flags: review+
Attachment #8858095 - Attachment is obsolete: true
Attachment #8858095 - Flags: review?(nalexander)
Pushed by mozilla@kaply.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/74c853a70973
Allow a system preference to determine distribution dir. r=nalexander
https://hg.mozilla.org/mozilla-central/rev/74c853a70973
Status: NEW → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 55
[Tracking Requested - why for this release]:

I am NOT requesting this for the Firefox 53 that is about to go out the door this week.

Putting on release management radar simply because we have two partners that have this requirement to be on their device, so we will need this either in a partner specific dot release or in whatever dot release comes up.

Going forward, we need to figure out a better process for stuff like this, and I honestly don't know what that is.

Unfortunately requirements like this seem to somehow always come in at the end of a cycle.
Duplicate of this bug: 1354156
OK, let's start planning this for a 53 mobile dot release, we can talk about it tomorrow and pull QE into things too. 

Ioana, Florin, FYI, a partner build that will need testing at some point over the next couple of weeks.
Flags: needinfo?(ioana.chiorean)
Flags: needinfo?(florin.mezei)
Mike, can you request uplift to 53 (and 54?)  I'd like this to land today. Thanks!
Flags: needinfo?(mozilla)
Comment on attachment 8857979 [details] [diff] [review]
Address review comments

Approval Request Comment
[Feature/Bug causing the regression]: Partner requirement to specify location of distributions.
[User impact if declined]: No user impact, needed for partner requirements.
[Is this code covered by automated tests?]: No.
[Has the fix been verified in Nightly?]: Yes.
[Needs manual test from QE? If yes, steps to reproduce]: Partner is running test. I will document how to test here as well. It requires a rooted device.
[List of other uplifts needed for the feature/fix]: None
[Is the change risky?]: No
[Why is the change risky/not risky?]: Not a regular codepath
[String changes made/needed]: None.
Flags: needinfo?(mozilla)
Attachment #8857979 - Flags: approval-mozilla-release?
Attachment #8857979 - Flags: approval-mozilla-beta?
Here is my test process on a rooted Nexus 5X.

Modify the build.prop in the /vendor directory to have

ro.org.mozilla.distributiondir=/system/testdir/

Create a directory called testdir/org.mozilla.firefox/ in the system directory.

Please distribution files in these directories, making sure they have proper permissions.

Verify that when you start Firefox clean (no user data) that you get these customizations.

Instructions on how system distributions work in general are here:

https://wiki.mozilla.org/Mobile/Distribution_Files#System_distribution
Comment on attachment 8857979 [details] [diff] [review]
Address review comments

Let's uplift this now for the upcoming 53 dot release.
Attachment #8857979 - Flags: approval-mozilla-release?
Attachment #8857979 - Flags: approval-mozilla-release+
Attachment #8857979 - Flags: approval-mozilla-beta?
Attachment #8857979 - Flags: approval-mozilla-beta+
Flags: needinfo?(ioana.chiorean)
Flags: needinfo?(florin.mezei)
You need to log in before you can comment on or make changes to this bug.