Closed Bug 1372486 Opened 7 years ago Closed 7 years ago

(photon) Support photon-only resources with mach/gradle build

Categories

(Firefox for Android Graveyard :: General, enhancement)

enhancement
Not set
normal

Tracking

(firefox56 fixed)

RESOLVED FIXED
Firefox 56
Tracking Status
firefox56 --- fixed

People

(Reporter: jwu, Assigned: jwu)

References

Details

Attachments

(5 files)

To keep resources for both Australis and Photon at the same time, we'll duplicate a copy of resources for Photon.

Any visual refresh for Photon should only modify the duplicated files. That would make us easily maintain two APKs.
The duplicate resource files/folders are just temporary on 56 because currently we have to keep both Australis and Photon available. We should remove all legacy resources of Australis on 57.
Assignee: nobody → topwu.tw
Comment on attachment 8877018 [details]
Bug 1372486 - Part 1: Move Fennec resources from base/resources to app/src/main/res.

https://reviewboard.mozilla.org/r/148352/#review152856
Attachment #8877018 - Flags: review?(walkingice0204) → review+
Comment on attachment 8877019 [details]
Bug 1372486 - Part 2: Fix Fennec resource path in build/lint scripts.

https://reviewboard.mozilla.org/r/148354/#review152888
Attachment #8877019 - Flags: review?(walkingice0204) → review+
Please loop me in to all of these build changes.  As it stands, this approach isn't needed.  It will be much better if we:

- overlay Photon resources on Australis resources, both in Gradle and in moz.build;
- overlay Australis resources onto a set of shared resources, both in Gradle and in moz.build;

rather than copying everything.  The best approach will be to make the moz.build setup use the Gradle directory structure, which is the end goal.

So: we should move:

- all of the _shared_ resources to mobile/android/app/src/main/res
- all of the _Photon-only_ (or replaced in Photon) resources to mobile/android/app/src/australis/res
- all of the _Australis-only_ (or replaced in Australis) resources to mobile/android/app/src/photon/res

That should "just work" for Gradle.  Now, for moz.build, we need to opt-in to the new directories, updating the directories around http://searchfox.org/mozilla-central/source/mobile/android/base/moz.build#1374 based on the flags.  Now, earlier directories should override later directories, so make sure to put the Photon-only and Australis-only directories _before_ the shared directory.  From |aapt|:

```
   -S  directory in which to find resources.  Multiple directories will be scanned
       and the first match found (left to right) will take precedence.
```

Again, please loop me in to _every_ build change.  This system is very fragile and I have a plan on how to evolve towards Gradle; doing a bunch of things that don't align with that plan will only keep us away from that goal.
Comment on attachment 8877019 [details]
Bug 1372486 - Part 2: Fix Fennec resource path in build/lint scripts.

https://reviewboard.mozilla.org/r/148354/#review153010

I want to see this before it lands.

::: mobile/android/app/build.gradle:208
(Diff revision 3)
>                  }
>                  srcDir "${topsrcdir}/mobile/android/app/assets"
>              }
>          }
>  
> +        australis {

Per my comment in the ticket, let's move to using the "Gradle default" locations, which I think I got right in the ticket, and make `moz.build` agree.

::: mobile/android/base/moz.build:1372
(Diff revision 3)
>          CONFIG['ANDROID_SUPPORT_V4_AAR_LIB'],
>          'sync-thirdparty.jar',
>      ]
>  
>  # Putting branding earlier allows branders to override default resources.
> +if CONFIG['MOZ_ANDROID_PHOTON_SKIN']:

Prefer:

```
ANDROID_RES_DIRS += [...]
if CONFIG[...]
    ANDROID_RES_DIRS += [...]
else:
    ANDROID_RES_DIRS += [...]
ANDROID_RES_DIRS += [...]
```

::: mobile/android/moz.configure:114
(Diff revision 3)
> +@depends('--enable-photon')
> +def photon_skin(value):
> +    if value:
> +        return True
> +
> +set_config('MOZ_ANDROID_PHOTON_SKIN', photon_skin)

Please make the `MOZ_ANDROID_XXX` agree with `--enable-XXX`.  There's no advantage to making readers look up the mapping in the `moz.configure` file.
Attachment #8877019 - Flags: review-
(In reply to Nick Alexander :nalexander from comment #9)
> Please loop me in to all of these build changes.  As it stands, this
> approach isn't needed.  It will be much better if we:
> 
> - overlay Photon resources on Australis resources, both in Gradle and in
> moz.build;
> - overlay Australis resources onto a set of shared resources, both in Gradle
> and in moz.build;

I think the main difference between our approach is 
1. We copy all resources at beginning and will drop resources in Australis eventually when Photon is ready, and
2. You prefer to use shared resources until Photon needs to modify them.

The benefit of whole resource duplication is for easy development:

We don't have to figure out which resource is shared/Australis-only/Photon-only right now. Since some visual design aren't finalized, fully duplication is the easiest way to keep Australis resource freeze. 

For each photon bugs, we can just add new resources into its resource folder, and remove unused files according to lint warning. Any changes in Photon won't effect Australis, and vice versa.

Also since we will drop Australis when Photon is ready, all duplicated resources in Australis will be removed eventually.
Comment on attachment 8877018 [details]
Bug 1372486 - Part 1: Move Fennec resources from base/resources to app/src/main/res.

https://reviewboard.mozilla.org/r/148352/#review153330
Attachment #8877018 - Flags: review?(cnevinchen) → review+
(In reply to Jing-wei Wu [:jwu] from comment #11)
> (In reply to Nick Alexander :nalexander from comment #9)
> > Please loop me in to all of these build changes.  As it stands, this
> > approach isn't needed.  It will be much better if we:
> > 
> > - overlay Photon resources on Australis resources, both in Gradle and in
> > moz.build;
> > - overlay Australis resources onto a set of shared resources, both in Gradle
> > and in moz.build;
> 
> I think the main difference between our approach is 
> 1. We copy all resources at beginning and will drop resources in Australis
> eventually when Photon is ready, and
> 2. You prefer to use shared resources until Photon needs to modify them.
> 
> The benefit of whole resource duplication is for easy development:
> 
> We don't have to figure out which resource is
> shared/Australis-only/Photon-only right now. Since some visual design aren't
> finalized, fully duplication is the easiest way to keep Australis resource
> freeze. 

You're thinking of this only in one direction: you're thinking about making Photon-specific changes and not wanting to touch Australis.  But there will be Australis/generic development, and you're making every Australis/generic change touch _two_ resources now.

I see 803 files in the shared resource folder.  You have something like 14 weeks to do the Photon work.  To decide what to do with each file, you will have to replace or delete ~10 resource files a day just to get through the list.  (And some are very complicated, like styles.xml!)  I don't think that's going to happen; you're going to have a tiny amount of Photon work on top of a _lot_ of shared/Australis resources.  I believe you will be much better off tactically adding and replacing resources in photon/res than this bulk copy.

Now, we may find that you need to copy-and-edit some files (like styles.xml) due to limitations in the aapt merging in moz.build.  But that will still be much better than this bulk copy, where the sets of files will immediately diverge and you will have a much harder time just understanding _what's changed_ between the two resource sets.

> For each photon bugs, we can just add new resources into its resource
> folder, and remove unused files according to lint warning. Any changes in
> Photon won't effect Australis, and vice versa.

This is also true if you make photon/res an overlay on top of main/res.
 
> Also since we will drop Australis when Photon is ready, all duplicated
> resources in Australis will be removed eventually.

This is also true if you remove australis/res when Australis is removed, and move australis/res/** into main/res.

Show me work-in-progress patches that you think require this copy operation, rather than an overlay operation.  As it is, this approach is not optimal, for the reasons I have pointed out.
Comment on attachment 8877019 [details]
Bug 1372486 - Part 2: Fix Fennec resource path in build/lint scripts.

https://reviewboard.mozilla.org/r/148354/#review153736

Please request again once the design is finalized. Thank you!
Attachment #8877019 - Flags: review?(cnevinchen)
Comment on attachment 8877019 [details]
Bug 1372486 - Part 2: Fix Fennec resource path in build/lint scripts.

https://reviewboard.mozilla.org/r/148354/#review153866

::: mobile/android/app/build.gradle:208
(Diff revision 3)
>                  }
>                  srcDir "${topsrcdir}/mobile/android/app/assets"
>              }
>          }
>  
> +        australis {

Okay, I'll file new patches ASAP that

1. move resources from base/resources to app/src/main/res, and
2. let Australis/Photon can put their specific resources into their own folders.
Summary: (photon) Duplicate resources for visual refresh → (photon) Support photon-only resources with mach/gradle build
Attachment #8877018 - Attachment is obsolete: true
Attachment #8877018 - Flags: review?(max)
Attachment #8877019 - Attachment is obsolete: true
Attachment #8877019 - Flags: review?(max)
Comment on attachment 8877019 [details]
Bug 1372486 - Part 2: Fix Fennec resource path in build/lint scripts.

https://reviewboard.mozilla.org/r/148354/#review154028

::: mobile/android/app/build.gradle
(Diff revision 4)
>              }
>  
>              res {
>                  srcDir "${topsrcdir}/${mozconfig.substs.MOZ_BRANDING_DIRECTORY}/res"
>                  srcDir "${project.buildDir}/generated/source/preprocessed_resources" // See syncPreprocessedResources.
> -                srcDir "${topsrcdir}/mobile/android/base/resources"

Huh, I wonder if we just changed the order that Gradle's resource merger will find things.  It shouldn't matter for Fennec, but it might matter for the one person I know who was customizing resources using `MOZ_BRANDING_DIRECTORY`.

In any case, roll on!

::: mobile/android/base/moz.build:1371
(Diff revision 4)
>      ]
>  
>  # Putting branding earlier allows branders to override default resources.
>  ANDROID_RES_DIRS += [
>      '/' + CONFIG['MOZ_BRANDING_DIRECTORY'] + '/res',
> -    'resources',
> +    '/mobile/android/app/src/main/res',

Same comment about ordering.  This is definitely correct for `moz.build`, so roll on.
Attachment #8877019 - Flags: review?(nalexander) → review+
Comment on attachment 8877951 [details]
Bug 1372486 - Part 3: Support mach/gradle build for Photon with its resources.

https://reviewboard.mozilla.org/r/149358/#review154032

r- just to make sure I got the Gradle directories correct.  I just checked, using `./mach gradle sourceSets`:
```
australis
---------
Compile configuration: australisCompile
build.gradle name: android.sourceSets.australis
Java sources: [mobile/android/app/src/australis/java]
Manifest file: mobile/android/app/src/australis/AndroidManifest.xml
Android resources: [mobile/android/app/src/australis/res]
Assets: [mobile/android/app/src/australis/assets]
AIDL sources: [mobile/android/app/src/australis/aidl]
RenderScript sources: [mobile/android/app/src/australis/rs]
JNI sources: [mobile/android/app/src/australis/jni]
JNI libraries: [mobile/android/app/src/australis/jniLibs]
Java-style resources: [mobile/android/app/src/australis/resources]

```

::: mobile/android/app/build.gradle:204
(Diff revision 1)
>              }
>          }
>  
> +        australis {
> +            res {
> +                srcDir "${topsrcdir}/mobile/android/australis/src/main/res"

I see where you're going, but the Gradle way of doing this is to use the per-flavor dimension directories, which in this case should be:

mobile/android/app/src/australis/res
mobile/android/app/src/photon/res

Those will get used by the Gradle build system automatically, and you shouldn't need any `res.srcDir` declarations at all.
Attachment #8877951 - Flags: review?(nalexander) → review-
Comment on attachment 8877965 [details]
Bug 1372486 - Part 4: Create a dummy files for empty resource folders.

https://reviewboard.mozilla.org/r/149364/#review154034

I'd prefer to land some meaningful file, like `res/values/skin_strings.xml` that has a single `<string skin="australis">` or, even better an actual resources that's different between Australis and Photon, but I won't block on it.

The `.mkdir.done` files are produced by the build system in the object directory, so I definitely don't want to check something with that name into the source directory.  If you really need these dummy files, choose something like `.not-part-of-the-build` or similar.
Attachment #8877965 - Flags: review?(nalexander) → review+
Comment on attachment 8877018 [details]
Bug 1372486 - Part 1: Move Fennec resources from base/resources to app/src/main/res.

https://reviewboard.mozilla.org/r/148352/#review154024

Assuming this is just `hg mv ...`, roll on!

::: commit-message-04981:1
(Diff revision 3)
> +Bug 1372486 - Part 1: Move Fennec resources from base/resources to app/src/main/res. r?nalexander,maliu,nechen,walkingice

I find it helpful to include the hg commands in the commit message for such things:

Trailing slashes can matter for hg mv; did you do:

hg mv mobile/android/base/resources/ app/src/main/res/
Attachment #8877018 - Flags: review?(nalexander) → review+
(In reply to Nick Alexander :nalexander from comment #23)
> Comment on attachment 8877018 [details]
> Bug 1372486 - Part 1: Move Fennec resources from base/resources to
> app/src/main/res.
> 
> https://reviewboard.mozilla.org/r/148352/#review154024
> 
> Assuming this is just `hg mv ...`, roll on!
> 
> ::: commit-message-04981:1
> (Diff revision 3)
> > +Bug 1372486 - Part 1: Move Fennec resources from base/resources to app/src/main/res. r?nalexander,maliu,nechen,walkingice
> 
> I find it helpful to include the hg commands in the commit message for such
> things:
> 
> Trailing slashes can matter for hg mv; did you do:
> 
> hg mv mobile/android/base/resources/ app/src/main/res/

The command I used is `hg mv mobile/android/base/resources app/src/main/res`, What is the difference between with and without trailing slashes? Should I file another patch?
(In reply to Jing-wei Wu [:jwu] from comment #24)
> (In reply to Nick Alexander :nalexander from comment #23)
> > Comment on attachment 8877018 [details]
> > Bug 1372486 - Part 1: Move Fennec resources from base/resources to
> > app/src/main/res.
> > 
> > https://reviewboard.mozilla.org/r/148352/#review154024
> > 
> > Assuming this is just `hg mv ...`, roll on!
> > 
> > ::: commit-message-04981:1
> > (Diff revision 3)
> > > +Bug 1372486 - Part 1: Move Fennec resources from base/resources to app/src/main/res. r?nalexander,maliu,nechen,walkingice
> > 
> > I find it helpful to include the hg commands in the commit message for such
> > things:
> > 
> > Trailing slashes can matter for hg mv; did you do:
> > 
> > hg mv mobile/android/base/resources/ app/src/main/res/
> 
> The command I used is `hg mv mobile/android/base/resources
> app/src/main/res`, What is the difference between with and without trailing
> slashes? Should I file another patch?

Nope, not if what you did works!  The slashes mostly have to do with what happens when the target directory already exists; it's the difference between moving foo/** into bar/ or moving foo/ into bar/foo/, IIRC.
Comment on attachment 8877951 [details]
Bug 1372486 - Part 3: Support mach/gradle build for Photon with its resources.

https://reviewboard.mozilla.org/r/149358/#review154522

This looks good!  If it's green on try, it's good for me!

I will draft a mail to mobile-firefox-dev explaining where the existing resources are and what the Australis/Photon/Gradle plan is.  I might send that without your review, since you (and others!) can correct me on the list; or I might get your feedback first.

Thanks for bringing this into line with my thinking!
Attachment #8877951 - Flags: review?(nalexander) → review+
Comment on attachment 8877965 [details]
Bug 1372486 - Part 4: Create a dummy files for empty resource folders.

https://reviewboard.mozilla.org/r/149364/#review154528

Looks good!

::: commit-message-8840b:3
(Diff revision 2)
> +Bug 1372486 - Part 4: Create a dummy files for empty resource folders. r?nalexander,maliu,nechen,walkingice
> +
> +Since Mercurial doesn't support empty folder, a dummy file is added to each res folers.

nit: s/folers/folders/

::: commit-message-8840b:4
(Diff revision 2)
> +Bug 1372486 - Part 4: Create a dummy files for empty resource folders. r?nalexander,maliu,nechen,walkingice
> +
> +Since Mercurial doesn't support empty folder, a dummy file is added to each res folers.
> +Them should be removed after we put Photon/Australis specific resources in their own res folders.

nit: s/Them/These files/
(In reply to Jing-wei Wu [:jwu] from comment #32)
> Try result:
> https://treeherder.mozilla.org/#/
> jobs?repo=try&revision=76623b1e5c2f3446db6d6827eb310c98fff0a632

I added a few jobs, most importantly the android-api-15-gradle job, which is the actual Photon-enabled build.
Keywords: checkin-needed
Comment on attachment 8877965 [details]
Bug 1372486 - Part 4: Create a dummy files for empty resource folders.

https://reviewboard.mozilla.org/r/149364/#review154736
Attachment #8877965 - Flags: review?(cnevinchen) → review+
Comment on attachment 8877951 [details]
Bug 1372486 - Part 3: Support mach/gradle build for Photon with its resources.

https://reviewboard.mozilla.org/r/149358/#review154738
Attachment #8877951 - Flags: review?(cnevinchen) → review+
Comment on attachment 8877019 [details]
Bug 1372486 - Part 2: Fix Fennec resource path in build/lint scripts.

https://reviewboard.mozilla.org/r/148354/#review154740
Attachment #8877019 - Flags: review?(cnevinchen) → review+
Pushed by ryanvm@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/3649a9346e36
Part 1: Move Fennec resources from base/resources to app/src/main/res. r=nalexander,nechen,walkingice
https://hg.mozilla.org/integration/autoland/rev/511276ae38a7
Part 2: Fix Fennec resource path in build/lint scripts. r=nalexander,nechen,walkingice
https://hg.mozilla.org/integration/autoland/rev/d60f1d602dca
Part 3: Support mach/gradle build for Photon with its resources. r=nalexander,nechen
https://hg.mozilla.org/integration/autoland/rev/da771468e363
Part 4: Create a dummy files for empty resource folders. r=nalexander,nechen
Keywords: checkin-needed
Backed out for build bustage failing TestMozbuildReading.test_orphan_file_patterns for mobile/android/base/moz.build:

https://hg.mozilla.org/integration/autoland/rev/ca11e8883c527f1d311e9f8995eea884075a4500
https://hg.mozilla.org/integration/autoland/rev/f26e5ef35d118ceeded19fe1ee1ba12552c9b1ff
https://hg.mozilla.org/integration/autoland/rev/f362fd82fd6005b357065992c7e0471532bd9ecd
https://hg.mozilla.org/integration/autoland/rev/b5bdd69dfa60f745727fb3d594199ddc8ce374b1

Push with bustage: https://treeherder.mozilla.org/#/jobs?repo=autoland&revision=da771468e3635f4488d08cf72144c8d64f25c3dd&filter-resultStatus=testfailed&filter-resultStatus=busted&filter-resultStatus=exception&filter-resultStatus=retry&filter-resultStatus=usercancel&filter-resultStatus=runnable
Build log: https://treeherder.mozilla.org/logviewer.html#?job_id=107918137&repo=autoland

TEST-UNEXPECTED-FAIL | /home/worker/workspace/build/src/config/tests/test_mozbuild_reading.py | TestMozbuildReading.test_orphan_file_patterns, line 105: The pattern 'resources/**' in a Files() entry in '/home/worker/workspace/build/src/mobile/android/base/moz.build' corresponds to no files in the tree.
AssertionError: The pattern 'resources/**' in a Files() entry in '/home/worker/workspace/build/src/mobile/android/base/moz.build' corresponds to no files in the tree.
Flags: needinfo?(topwu.tw)
Add a new patch to fix the resource path defined in mobile/android/base/moz.build.

The new try command that covers the test is `mach try -b do -p android-api-15,android-x86,linux64 -u robocop -t none`, and the result is here:

https://treeherder.mozilla.org/#/jobs?repo=try&revision=55b789ba84618ada9cb6f92e74d289043f18e524&selectedJob=108074408
Flags: needinfo?(topwu.tw)
Comment on attachment 8878995 [details]
Bug 1372486 - Part 5: Fix Fennec resources path defined in moz.build to pass `mach build check`.

https://reviewboard.mozilla.org/r/150268/#review154910
Attachment #8878995 - Flags: review?(cnevinchen) → review+
Comment on attachment 8878995 [details]
Bug 1372486 - Part 5: Fix Fennec resources path defined in moz.build to pass `mach build check`.

https://reviewboard.mozilla.org/r/150268/#review155174

::: mobile/android/base/moz.build:91
(Diff revision 1)
>      BUG_COMPONENT = ('Firefox for Android', 'Profile Handling')
>  
>  with Files('locales/**'):
>      BUG_COMPONENT = ('Firefox for Android', 'General')
>  
> -with Files('resources/**'):
> +with Files('../app/src/main/res/**'):

I think we should add '../app/src/australis/**' to 'General', and '../app/src/photon/**' to _something_ -- do we have a Photon component?  If not, let's add it to General.

We might make these _all_ '../app/src/*/res/...', since roughly speaking main, photon, australis will all divide into the same pieces.

Make sense?
Attachment #8878995 - Flags: review?(nalexander) → review+
Comment on attachment 8878995 [details]
Bug 1372486 - Part 5: Fix Fennec resources path defined in moz.build to pass `mach build check`.

https://reviewboard.mozilla.org/r/150268/#review155174

> I think we should add '../app/src/australis/**' to 'General', and '../app/src/photon/**' to _something_ -- do we have a Photon component?  If not, let's add it to General.
> 
> We might make these _all_ '../app/src/*/res/...', since roughly speaking main, photon, australis will all divide into the same pieces.
> 
> Make sense?

Yeah, use `*` for main/australis/photon makes more sense.
Keywords: checkin-needed
Pushed by ryanvm@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/cf646df15e71
Part 1: Move Fennec resources from base/resources to app/src/main/res. r=nalexander,nechen,walkingice
https://hg.mozilla.org/integration/autoland/rev/935cc1064edd
Part 2: Fix Fennec resource path in build/lint scripts. r=nalexander,nechen,walkingice
https://hg.mozilla.org/integration/autoland/rev/c0962acec73d
Part 3: Support mach/gradle build for Photon with its resources. r=nalexander,nechen
https://hg.mozilla.org/integration/autoland/rev/a4636f4caa44
Part 4: Create a dummy files for empty resource folders. r=nalexander,nechen
https://hg.mozilla.org/integration/autoland/rev/770a7a380874
Part 5: Fix Fennec resources path defined in moz.build to pass `mach build check`. r=nalexander,nechen
Keywords: checkin-needed
Attachment #8877018 - Flags: review?(max)
Attachment #8877019 - Flags: review?(max)
Attachment #8877951 - Flags: review?(max)
Attachment #8877965 - Flags: review?(max)
Attachment #8878995 - Flags: review?(max)
Depends on: 1366662
Blocks: 1366662
No longer depends on: 1366662
Blocks: 1374959
Comment on attachment 8877951 [details]
Bug 1372486 - Part 3: Support mach/gradle build for Photon with its resources.

https://reviewboard.mozilla.org/r/149358/#review156642
Attachment #8877951 - Flags: review?(walkingice0204)
Comment on attachment 8877965 [details]
Bug 1372486 - Part 4: Create a dummy files for empty resource folders.

https://reviewboard.mozilla.org/r/149364/#review156644
Attachment #8877965 - Flags: review?(walkingice0204)
Comment on attachment 8878995 [details]
Bug 1372486 - Part 5: Fix Fennec resources path defined in moz.build to pass `mach build check`.

https://reviewboard.mozilla.org/r/150268/#review156646
Attachment #8878995 - Flags: review?(walkingice0204)
Blocks: 1375448
Product: Firefox for Android → Firefox for Android Graveyard
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: