1176133 - Replace |mach gradle-install| with build-backend code

Status: RESOLVED WORKSFORME

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

Description

[Nick Alexander :nalexander [he/him]]

|mach gradle-install| has run its course.  It's been nice having a light-weight implementation rather than a full-fledged build backend, but it's a manual step that needs to be done at just the right time (after build and package).  Since all it is doing is arranging some symlinks and preprocessing, I think we may be able to add a light-weight build subcontext that keeps $OBJDIR/mobile/android/gradle up to date.

I know of two things that need to be done:

* make the Gradle build lazy about copying omni.ja and .so libraries from the object directory (right now |mach gradle-install| fails if the objdir hasn't been packaged once);

* implement the build-backend bits for arranging symlinks.  Since files come and go from the gradle directory, this pretty much needs to "own" that directory -- removing extraneous files, ignoring some files, etc.  The manifest processing needs to be improved to handle this.

Comment 1

[Nick Alexander :nalexander [he/him]]

Attachment: MozReview Request: Bug 1176133 - Part 1: Allow ignoring paths that would be removed by FileCopier. r?gps

Bug 1176133 - Part 1: Allow ignoring paths that would be removed by FileCopier. r?gps

It's not possible (well, easy) to add a manifest type ignoring a
pattern in the destination directory.  This is because of when a
copier tracks destination files with individual entries.  Since a
copier is not tied to a particular destination directory, it's not
possible to walk the destination directory to determine the complete
set of files to ignore.  Reworking the copiers to handle patterns
requires significant effort.

The approach in this patch is conceptually simple and intended for
internal consumers only.  It adds a predicate to selectively not
delete unregistered files (the ones we would like to specify with a
manifest pattern).  There is an unfortunate wrinkle: an unregistered
file that would have been deleted can now cause its parent directory
to be silently preserved.  If we felt strongly, we could say the
parent directory was not deleted as well.

Comment 2

[Nick Alexander :nalexander [he/him]]

Attachment: MozReview Request: Bug 1176133 - Part 2: Use FileCopier and filter predicate in |mach gradle-install| directly. r?gps

Bug 1176133 - Part 2: Use FileCopier and filter predicate in |mach gradle-install| directly. r?gps

There's no need to invoke a process here, and even if there was a
reason, there's no smooth way to expose a general filter predicate to
the command line.

Comment 3

[Nick Alexander :nalexander [he/him]]

Attachment: MozReview Request: Bug 1176133 - Part 3: Keep all preprocessed_code references in the objdir. r?sebastian

Bug 1176133 - Part 3: Keep all preprocessed_code references in the objdir. r?sebastian

This was just an oversight.  The Gradle configuration referenced
topsrcdir rather than having a symlink via the objdir.  This didn't
impact the Gradle build, but it did make the preprocessed_code Gradle
project appear outside of the root Gradle project in IntelliJ.

Comment 4

[Nick Alexander :nalexander [he/him]]

Attachment: MozReview Request: Bug 1176133 - Part 4: Copy omni.ja, native libraries, and asset libraries from dist/fennec manually. r?sebastian

Bug 1176133 - Part 4: Copy omni.ja, native libraries, and asset libraries from dist/fennec manually. r?sebastian

This allows us to not require dist/fennec/* to exist in the object
directory at gradle-install time.  It gets us one small step closer to
being able to sit down to a fresh source tree and open a Fennec
project in IntelliJ.

Comment 5

[Nick Alexander :nalexander [he/him]]

Attachment: MozReview Request: Bug 1176133 - Part 5: Copy preprocessed code from objdir to build directory during Gradle build. r?sebastian

Bug 1176133 - Part 5: Copy preprocessed code from objdir to build directory during Gradle build. r?sebastian

This means we don't require the directory in the object directory at
gradle-install time.  We're not concerned if the source files are
missing, since we have code to ensure they're fresh already; and if
they are missing, we'll quickly fail as we try to compile with missing
sources.

Comment 6

[Nick Alexander :nalexander [he/him]]

Attachment: MozReview Request: Bug 1176133 - Part 6: Copy preprocessed resources from objdir to build directory during Gradle build. r?sebastian

Bug 1176133 - Part 6: Copy preprocessed resources from objdir to build directory during Gradle build. r?sebastian

This means we don't require the directory in the object directory at
gradle-install time.  We're not concerned if the resource files are
missing, since we have code to ensure they're fresh already; and if
they are missing, we'll quickly fail as we try to process the resource
set.

Comment 7

[Nick Alexander :nalexander [he/him]]

gps: the changes don't quite get us to removing gradle-install.  Follow-up patch to come when I work out the last objdir/pre-processing details.

glandium: NI just so you're aware of this; I've been flagging gps for reviews somewhat arbitrarily.

sebastian: mcomella: this makes |mach gradle-install| *remove* cruft that's outdated; it gets us closer to installing files like the IntelliJ project configuration XML.  One step at a time.

Comment 8

[Sebastian Kaspari (:sebastian; :pocmo)]

Comment on attachment 8649398 [details]
MozReview Request: Bug 1176133 - Part 3: Keep all preprocessed_code references in the objdir. r?sebastian

https://reviewboard.mozilla.org/r/16389/#review14777

Ship It!

Comment 9

[Sebastian Kaspari (:sebastian; :pocmo)]

Comment on attachment 8649400 [details]
MozReview Request: Bug 1176133 - Part 4: Copy omni.ja, native libraries, and asset libraries from dist/fennec manually. r?sebastian

https://reviewboard.mozilla.org/r/16391/#review14787

Ship It!

Comment 10

[Sebastian Kaspari (:sebastian; :pocmo)]

Comment on attachment 8649401 [details]
MozReview Request: Bug 1176133 - Part 5: Copy preprocessed code from objdir to build directory during Gradle build. r?sebastian

https://reviewboard.mozilla.org/r/16393/#review14789

Ship It!

Comment 11

[Sebastian Kaspari (:sebastian; :pocmo)]

Comment on attachment 8649402 [details]
MozReview Request: Bug 1176133 - Part 6: Copy preprocessed resources from objdir to build directory during Gradle build. r?sebastian

https://reviewboard.mozilla.org/r/16395/#review14791

Ship It!

Comment 12

[Gregory Szorc [:gps]]

Comment on attachment 8649396 [details]
MozReview Request: Bug 1176133 - Part 1: Allow ignoring paths that would be removed by FileCopier. r?gps

https://reviewboard.mozilla.org/r/16385/#review14917

::: python/mozbuild/mozpack/copier.py:192
(Diff revision 1)
> +             remove_unaccounted_predicate=None,

How about remove_unaccounted_fn? "predicate" to me sounds like a noun, not a function. Perhaps you know something about computer science that I don't.

::: python/mozbuild/mozpack/copier.py:346
(Diff revision 1)
> +        if not callable(remove_unaccounted_predicate):
> +            remove_unaccounted_predicate = None

Please do argument validation at top of functions.

::: python/mozbuild/mozpack/copier.py:399
(Diff revision 1)
> +            if remove_unaccounted_predicate:
> +                if not remove_unaccounted_predicate(mozpath.relpath(d, destination)):
> +                    result.ignored_directories.add(d)
> +                    continue

FileCopier purposefully tries to be ignorant about directories and only cares about files. Passing a directory into remove_unaccounted_predicate breaks this pattern.

The code above ensures that directories without files are removed. So this code only prevents subsequent writers of files in "ignored" directories from having to recreate the directory. But they presumably have to do this anyway. So I see no purpose for this complexity.

::: python/mozbuild/mozpack/test/test_copier.py:401
(Diff revision 1)
> +            return path in ['emptydir', 'populateddir/second'] or \

Nit: use tuples for immutable sequences.

Remove emptydir from this since that code will be deleted.

::: python/mozbuild/mozpack/test/test_copier.py:413
(Diff revision 1)
> +    def test_remove_unaccounted_filter_leaves_directory_that_would_be_deleted(self):

This can be deleted once you delete the corresponding code.

Comment 13

[Gregory Szorc [:gps]]

Comment on attachment 8649397 [details]
MozReview Request: Bug 1176133 - Part 2: Use FileCopier and filter predicate in |mach gradle-install| directly. r?gps

https://reviewboard.mozilla.org/r/16387/#review14921

I could give r+. But since you have to rework the first patch (maybe - read comments here), I'll wait to see v2.

::: mobile/android/mach_commands.py:11
(Diff revision 1)
> +from mozpack.copier import FileCopier
> +from mozpack.manifests import InstallManifest

We defer module-level imports in mach_commands.py files for performance reasons. Please leave these imports where they were.

::: mobile/android/mach_commands.py:149
(Diff revision 1)
> -        manifest_path = os.path.join(self.topobjdir, 'mobile', 'android', 'gradle.manifest')
> -        with FileAvoidWrite(manifest_path) as f:
> -            m.write(fileobj=f)
> +        def should_delete(path):
> +            return not any(mozpath.match(path, pattern) for pattern in
> +                ('.deps/**', '.idea/**', '.gradle/**', 'build/**', '*/build/**', '**/*.iml'))

This reminds me of code in InstallManifest which expands wildcards at population time to determine what files to copy/symlink. And this reminds me that up until your first commit, the rules for FileCopier were static at call time.

You *could* modify the InstallManifest in this method to add ignore entries for all files that you find, using a matcher to quickly find them. Then your first commit doesn't need to exist.

I'm not opposed to the functionality you added in the previous commit. But I'm not convinced it is necessary. Am I missing something?

::: mobile/android/mach_commands.py:164
(Diff revision 1)
> -            if not code:
> +            COMPLETE = 'From {dest}:\nKept {existing} existing; Added/updated {updated}; ' \
> +                'Removed {rm_files} files and {rm_dirs} directories; ' \
> +                'Ignored {ig_files} files and {ig_dirs} directories.'
> +            print(COMPLETE.format(dest=destdir,
> +                existing=result.existing_files_count,
> +                updated=result.updated_files_count,
> +                rm_files=result.removed_files_count,
> +                rm_dirs=result.removed_directories_count,
> +                ig_files=result.ignored_files_count,
> +                ig_dirs=result.ignored_directories_count))

Don't feel obligated to do this unless you think it is important.

Comment 14

[Nick Alexander :nalexander [he/him]]

I'm going to land the patches sebastian reviewed as Bug 1196970.

Comment 15

[Nick Alexander :nalexander [he/him]]

This is no longer needed, and has been removed \o/  I think Bug 1123416 was the death knell for this.