Open Bug 1158018 Opened 7 years ago Updated 4 years ago

Package manifests are too hard to maintain

Categories

(Firefox Build System :: General, defect)

defect
Not set
normal

Tracking

(Not tracked)

ASSIGNED

People

(Reporter: jcranmer, Assigned: jcranmer)

References

Details

Attachments

(1 file, 1 obsolete file)

Attached patch smarter-packaging (obsolete) — Splinter Review
glandium had an idea of scanning the manifests and basically turning the package-manifest more into a list of files not to package rather than a list of files to package.
Attachment #8597013 - Flags: feedback?(mh+mozilla)
Comment on attachment 8597013 [details] [diff] [review]
smarter-packaging

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

::: python/mozbuild/mozpack/packager/__init__.py
@@ +417,5 @@
> +        Remove any bin/ prefix.
> +        '''
> +        if mozpath.basedir(path, ['bin']) == 'bin':
> +            return mozpath.relpath(path, 'bin')
> +        return path

I think you could replace this with:
normalize_path = SimpleManifestSink.normalize_path

@@ +432,5 @@
> +            self._found_files[dest] = f
> +            if is_manifest(p):
> +                self._manifests.add(p)
> +                self._add_manifest(component, p, f)
> +        if not added and not from_manifest:

I don't think from_manifest should skip the error here.

@@ +460,5 @@
> +                self._found_files.pop(dest)
> +            except KeyError:
> +                errors.error('Never added file: %s' % dest)
> +        if not removed:
> +            errors.error('Missing file(s): %s' % pattern)

It seems to me we should keep track of why some file was added, and remove it if all the reasons it was included are removed. At least for manifests.
That is, let's say we have foo.manifest that references bar.manifest and baz.manifest, and each of them also reference a directory of the same name without extension:

foo.manifest
-foo

would be equivalent to (with SimpleManifestSink)

foo.manifest
bar.manifest
bar
baz.manifest
baz

,

foo.manifest
-bar.manifest

would be equivalent to

foo.manifest
foo
baz.manifest
baz

but if bar.manifest also has a reference to the baz directory,

foo.manifest
-bar.manifest

would be equivalent to

foo.manifest
foo
baz.manifest
baz

That is, because baz is referenced from both bar.manifest and baz.manifest, removing bar.manifest wouldn't remove it.

But,

foo.manifest
-baz

would.

@@ +462,5 @@
> +                errors.error('Never added file: %s' % dest)
> +        if not removed:
> +            errors.error('Missing file(s): %s' % pattern)
> +
> +    def close(self, auto_root_manifest=True):

auto_root_manifest is only used when no manifest is given, in which case we should just continue using the SimpleManifestSink. So no need to keep this path.

::: toolkit/mozapps/installer/packager.mk
@@ +39,5 @@
> +		$(if $(filter-out 0,$(MOZ_PKG_FATAL_WARNINGS)),,--ignore-errors) \
> +		$(addprefix --unify ,$(UNIFY_DIST)) \
> +		$(MOZ_PKG_MANIFEST) $(MOZ_NEW_PKG_MANIFEST) $(DIST)
> +endif
> +endif

I'm not a big fan of duplicating this complex command line. I'd rather have a mode of packager.py that takes both manifests.
Attachment #8597013 - Flags: feedback?(mh+mozilla)
(In reply to Mike Hommey [:glandium] from comment #1)
> @@ +432,5 @@
> > +            self._found_files[dest] = f
> > +            if is_manifest(p):
> > +                self._manifests.add(p)
> > +                self._add_manifest(component, p, f)
> > +        if not added and not from_manifest:
> 
> I don't think from_manifest should skip the error here.

 0:02.47 Error: /src/build/trunk/browser/dist/bin/chrome/toolkit.manifest:4: Missing file(s): bin/chrome/toolkit/content/global-platform
 0:02.48 Error: /src/build/trunk/browser/dist/bin/chrome/toolkit.manifest:5: Missing file(s): bin/chrome/toolkit/content/global-region

Thanks to <https://dxr.mozilla.org/comm-central/source/mozilla/toolkit/content/jar.mn#3>, and there wasn't any easy way to avoid this issue.

> It seems to me we should keep track of why some file was added, and remove
> it if all the reasons it was included are removed. At least for manifests.

My plan was to designate removal as such:
1. Removing a file would remove it from the list, regardless of how many ways it was added.
2. Removing a manifest would be equivalent to not specifying it in the first place.

> I'm not a big fan of duplicating this complex command line. I'd rather have
> a mode of packager.py that takes both manifests.

check-package.py was only ever meant to be a temporary bridge to verify that manifests weren't changed.
What I'm mostly looking for feedback here is on the removal lists in the package manifest files, although other feedback is appreciated.

I pushed the result to try here: <https://treeherder.mozilla.org/#/jobs?repo=try&revision=00287bee0ece>, so this appears to be the set of changes that doesn't impact packaging for browser, android, and desktop mulet builds. This isn't the final patch (I will split up the changes), but salient points that might otherwise make you go WTF:
1. The b2g manifest effectively ends up causing all the test executables (along with many other things) to be packaged on OS X. It looks like it's a bad port of the v2 signing changes (the desktop port changed the old directory to the list I modified in the b2g-installer, the b2g port just changed it to @RESPATH@). I'm aware that the OS X B2G build failed, but I don't consider that necessary to fix prior to feedback.
2. The addoncompat.manifest file (unpackaged on Fennec) has a manifest entry for a file which is never added to EXTRA_JS_COMPONENTS. So trying to remove that manifest on Fennec caused the packager to fail, since it was trying to remove a file that wasn't added.
3. The changes to mobile/android/package-manifest.in should be the minimum necessary to cause that manifest to not cause packaging issues, allowing me to turn on fatal errors. In the final version, I intend to have an early patch to turn on fatal errors prior to the hg cp/mv to the new manifest.
4. It doesn't look like there's actually a need to import b2g's package-manifest.in in mulet builds.
5. I don't intend to keep the check-packager.py stuff for long, only for as long as it takes me to convert all of the package manifests in mozilla- and comm-central.

The main troubling aspect is the presence of several deletion lines. While the non-packaging of, say, xpctest.xpt is understandable, the non-packaging of, say, dom_workers.xpt feels more like a result of a packaging oversight than any conscious packaging decision.
Attachment #8597013 - Attachment is obsolete: true
Attachment #8613860 - Flags: feedback?(nalexander)
Attachment #8613860 - Flags: feedback?(mh+mozilla)
Comment on attachment 8613860 [details] [diff] [review]
smarter-packaging

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

I am eager to see this are improved but really don't understand the details.  Be aware of https://bugzilla.mozilla.org/show_bug.cgi?id=901059.  If you were to post an updated patch to that ticket (the intersection of that work and yours, say), we could clean mobile/android's package-manifest.in right now.

::: mobile/android/installer/Makefile.in
@@ +97,5 @@
> +ifdef MOZ_NEW_PKG_MANIFEST_P
> +# When MOZ_CHROME_MULTILOCALE is defined, we write multilocale.json like:
> +# {"locales": ["en-US", "de", "ar", ...]}
> +
> +$(MOZ_NEW_PKG_MANIFEST): $(MOZ_NEW_PKG_MANIFEST_P) $(GLOBAL_DEPS) FORCE

Can we avoid this duplication?

::: mobile/android/installer/package-manifest.in
@@ -17,5 @@
>  [@AB_CD@]
>  @BINPATH@/chrome/@AB_CD@@JAREXT@
>  @BINPATH@/chrome/@AB_CD@.manifest
>  @BINPATH@/@PREF_DIR@/mobile-l10n.js
> -@BINPATH@/searchplugins/*

So much of this list is difficult to verify.  I did see a lot of things that I know shouldn't be shipped with Fennec, but ... land it at the beginning of the cycle :)

::: toolkit/mozapps/installer/packager.py
@@ +258,5 @@
>      parser.add_argument('--jarlog', default='', help='File containing jar ' +
>                          'access logs')
>      parser.add_argument('--optimizejars', action='store_true', default=False,
>                          help='Enable jar optimizations')
> +    parser.add_argument('--simple', action='store_true', default=False,

Simple is vague.  Why not --recursive and make this opt-out?  Or double-down on terminology with --use-new-package-manifest or something.
Attachment #8613860 - Flags: feedback?(nalexander)
Comment on attachment 8613860 [details] [diff] [review]
smarter-packaging

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

::: browser/installer/Makefile.in
@@ +113,5 @@
> +
> +$(MOZ_NEW_PKG_MANIFEST): $(MOZ_NEW_PKG_MANIFEST_P) $(GLOBAL_DEPS)
> +	$(call py_action,preprocessor,$(DEFINES) $(ACDEFINES) $(MOZ_NEW_PKG_MANIFEST_P) -o $@)
> +
> +GARBAGE += $(MOZ_NEW_PKG_MANIFEST)

The packager itself is doing preprocessing. Let's not replicate this manual preprocessing that shouldn't be done for the current manifests anyways. Let's fix that, btw (will file a separate bug).

::: python/mozbuild/mozpack/packager/__init__.py
@@ +407,5 @@
> +        self._finder = finder
> +        self.packager = SimplePackager(formatter)
> +        self._closed = False
> +        self._manifests = set()
> +        self._found_files = dict()

Feels like OrderedDict() would be better.

@@ +427,5 @@
> +                self._found_files[dest] = (f, [source_manifest])
> +            if is_manifest(p):
> +                self._manifests.add(p)
> +                self._add_manifest(component, p, f)
> +        if not added and source_manifest is None:

I still don't think the error should be skipped. What kind of legitimate case is this "source_manifest" check meant to allow?

@@ +442,5 @@
> +            if isinstance(e, ManifestEntryWithRelPath):
> +                self.add(component, e.path, path)
> +            elif isinstance(e, ManifestResource) and ':' not in e.target:
> +                retarget = ManifestEntryWithRelPath(e.base, e.target).path
> +                if retarget != e.base:

so, let's say there's a resource entry with . as target, this will skip adding the files from there. Is this wanted?

@@ +460,5 @@
> +            removed = True
> +            dest = mozpath.join(component.destdir, RecursiveManifestSink.normalize_path(p))
> +            try:
> +                if source_manifest is None:
> +                    self._found_files.pop(dest)

AFAIK, it's more idiomatic to do del dict[key]

@@ +472,5 @@
> +
> +            if is_manifest(p):
> +                self._remove_manifest(component, p, f)
> +        if not removed:
> +            errors.error('Missing file(s): %s' % pattern)

This probably needs a different error message from the add() case to make the difference in error more explicit.

@@ +480,5 @@
> +        if hasattr(file, 'path'):
> +            # Find the directory the given path is relative to.
> +            b = mozpath.normsep(file.path)
> +            if b.endswith('/' + path) or b == path:
> +                base = os.path.normpath(b[:-len(path)])

With the one you add in RecursiveManifestSink._add_manifest and the one we already have in SimplePackager._add_manifest_file, that's three occurrences of these 4 lines. Factor out in a function?

@@ +485,5 @@
> +        for e in parse_manifest(base, path, file.open()):
> +            if isinstance(e, ManifestEntryWithRelPath):
> +                self.remove(component, e.path, path)
> +            elif isinstance(e, ManifestResource) and ':' not in e.target:
> +                pass

Remove this line

@@ +488,5 @@
> +            elif isinstance(e, ManifestResource) and ':' not in e.target:
> +                pass
> +                retarget = ManifestEntryWithRelPath(e.base, e.target).path
> +                if retarget != e.base:
> +                    self.remove(component, retarget, path)

In fact, this function is very much the same as _add_manifest, but just calls different methods. Maybe worth factoring?

@@ +490,5 @@
> +                retarget = ManifestEntryWithRelPath(e.base, e.target).path
> +                if retarget != e.base:
> +                    self.remove(component, retarget, path)
> +
> +    def close(self, auto_root_manifest=True):

Same comment as before, auto_root_manifest is only used when no manifest is given, in which case we should just continue using the SimpleManifestSink. So no need to keep this path.

@@ +504,5 @@
> +            path = mozpath.dirname(mozpath.commonprefix(paths))
> +            for p, f in self._finder.find(mozpath.join(path,
> +                                          'chrome.manifest')):
> +                if not p in self._manifests:
> +                    self.packager.add(SimpleManifestSink.normalize_path(p), f)

RecursiveManifestSink.normalize_path

::: toolkit/mozapps/installer/packager.mk
@@ +42,5 @@
> +ifdef MOZ_NEW_PKG_MANIFEST
> +	$(PYTHON) $(MOZILLA_DIR)/toolkit/mozapps/installer/check-package.py $(DEFINES) \
> +		$(if $(filter-out 0,$(MOZ_PKG_FATAL_WARNINGS)),,--ignore-errors) \
> +		$(addprefix --unify ,$(UNIFY_DIST)) \
> +		$(MOZ_PKG_MANIFEST) $(MOZ_NEW_PKG_MANIFEST) $(DIST)

Still not a big fan of the duplicated command line, even if it's temporary. How many temporary things are still around years later? ;) Specifically, this will be useful for other apps to transition to the new manifests, so it is actually good to keep around for a while.

::: toolkit/mozapps/installer/packager.py
@@ +258,5 @@
>      parser.add_argument('--jarlog', default='', help='File containing jar ' +
>                          'access logs')
>      parser.add_argument('--optimizejars', action='store_true', default=False,
>                          help='Enable jar optimizations')
> +    parser.add_argument('--simple', action='store_true', default=False,

I think it would be better to have some sort of header in the manifest indicating its format, than to rely on an argument. And with that, it might be better to use MOZ_PKG_MANIFEST for the new manifest and add a MOZ_OLD_PKG_MANIFEST for the validation phase.
Attachment #8613860 - Flags: feedback?(mh+mozilla)
Depends on: 1184446
Depends on: 1185259
Depends on: 1185260
Depends on: 1185263
(In reply to Mike Hommey [:glandium] from comment #5)
> I still don't think the error should be skipped. What kind of legitimate
> case is this "source_manifest" check meant to allow?

If I drop the check, I get packaging manifest errors from <https://dxr.mozilla.org/comm-central/source/mozilla/toolkit/content/jar.mn#3>:
 0:02.47 Error: /src/build/trunk/browser/dist/bin/chrome/toolkit.manifest:4: Missing file(s): bin/chrome/toolkit/content/global-platform
 0:02.48 Error: /src/build/trunk/browser/dist/bin/chrome/toolkit.manifest:5: Missing file(s): bin/chrome/toolkit/content/global-region

It's not clear to me that these lines can be deleted from the toolkit manifest.

> I think it would be better to have some sort of header in the manifest
> indicating its format, than to rely on an argument. And with that, it might
> be better to use MOZ_PKG_MANIFEST for the new manifest and add a
> MOZ_OLD_PKG_MANIFEST for the validation phase.

I do hate the name _NEW_PKG_MANIFEST, but since the sink needs to be passed into the parser, it's not clear how to (easily) cause a manifest header to switch which sink to use...
(In reply to Joshua Cranmer [:jcranmer] from comment #6)
> (In reply to Mike Hommey [:glandium] from comment #5)
> > I still don't think the error should be skipped. What kind of legitimate
> > case is this "source_manifest" check meant to allow?
> 
> If I drop the check, I get packaging manifest errors from
> <https://dxr.mozilla.org/comm-central/source/mozilla/toolkit/content/jar.
> mn#3>:
>  0:02.47 Error: /src/build/trunk/browser/dist/bin/chrome/toolkit.manifest:4:
> Missing file(s): bin/chrome/toolkit/content/global-platform
>  0:02.48 Error: /src/build/trunk/browser/dist/bin/chrome/toolkit.manifest:5:
> Missing file(s): bin/chrome/toolkit/content/global-region
> 
> It's not clear to me that these lines can be deleted from the toolkit
> manifest.

They almost can: bug 780562
Duplicate of this bug: 1320390
Duplicate of this bug: 1229559
Product: Core → Firefox Build System
You need to log in before you can comment on or make changes to this bug.