bugzilla.mozilla.org has resumed normal operation. Attachments prior to 2014 will be unavailable for a few days. This is tracked in Bug 1475801.
Please report any other irregularities here.

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

RESOLVED WORKSFORME

Status

()

Firefox for Android
Build Config & IDE Support
RESOLVED WORKSFORME
3 years ago
10 months ago

People

(Reporter: nalexander, Unassigned)

Tracking

Firefox Tracking Flags

(Not tracked)

Details

MozReview Requests

()

Submitter Diff Changes Open Issues Last Updated
Loading...
Error loading review requests:

Attachments

(6 attachments)

(Reporter)

Description

3 years ago
|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.
(Reporter)

Comment 1

3 years ago
Created attachment 8649396 [details]
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.
Attachment #8649396 - Flags: review?(gps)
(Reporter)

Comment 2

3 years ago
Created attachment 8649397 [details]
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.
Attachment #8649397 - Flags: review?(gps)
(Reporter)

Comment 3

3 years ago
Created attachment 8649398 [details]
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.
Attachment #8649398 - Flags: review?(s.kaspari)
(Reporter)

Comment 4

3 years ago
Created attachment 8649400 [details]
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.
Attachment #8649400 - Flags: review?(s.kaspari)
(Reporter)

Comment 5

3 years ago
Created attachment 8649401 [details]
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.
Attachment #8649401 - Flags: review?(s.kaspari)
(Reporter)

Comment 6

3 years ago
Created attachment 8649402 [details]
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.
Attachment #8649402 - Flags: review?(s.kaspari)
(Reporter)

Comment 7

3 years ago
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.
Flags: needinfo?(mh+mozilla)
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!
Attachment #8649398 - Flags: review?(s.kaspari) → review+
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!
Attachment #8649400 - Flags: review?(s.kaspari) → review+
Attachment #8649401 - Flags: review?(s.kaspari) → review+
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 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!
Attachment #8649402 - Flags: review?(s.kaspari) → review+
Flags: needinfo?(mh+mozilla)
(Reporter)

Updated

3 years ago
Blocks: 1183335
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.
Attachment #8649396 - Flags: review?(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.
Attachment #8649397 - Flags: review?(gps)
(Reporter)

Comment 14

3 years ago
I'm going to land the patches sebastian reviewed as Bug 1196970.
(Reporter)

Updated

3 years ago
Depends on: 1216434
(Reporter)

Updated

3 years ago
Depends on: 1218935
(Reporter)

Comment 15

2 years ago
This is no longer needed, and has been removed \o/  I think Bug 1123416 was the death knell for this.
Status: NEW → RESOLVED
Last Resolved: 2 years ago
Resolution: --- → WORKSFORME
(Reporter)

Updated

10 months ago
Component: Build Config → Build Config & IDE Support
Product: Core → Firefox for Android
You need to log in before you can comment on or make changes to this bug.