834176 - NON_OMNIJAR_FILES is not handled by the new packager

Status: RESOLVED FIXED

(Firefox Build System :: General, defect)

Description

[Mike Hommey [:glandium]]

It turns out at least thunderbird needs to declare additional NON_OMNIJAR_FILES.

Comment 1

[Mike Hommey [:glandium]]

Attachment: Use NON_OMNIJAR_FILES value in the new packager

Comment 2

[Gregory Szorc [:gps]]

Comment on attachment 705803 [details] [diff] [review]
Use NON_OMNIJAR_FILES value in the new packager

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

Seems pretty straightforward. Just have a few nits.

::: python/mozbuild/mozpack/packager/formats.py
@@ +200,5 @@
>      '''
>      Formatter for the omnijar package format.
>      '''
> +    def __init__(self, copier, omnijar_name, compress=True, optimize=True,
> +                 non_resource=[]):

Since this is a list, perhaps it should be non_resources?

@@ +258,5 @@
>          omnijar archive.
>          '''
>          base = self._get_base(path)
>          path = mozpack.path.split(mozpack.path.relpath(path, base))
> +        if any(mozpack.path.match(path, p.replace('*', '**'))

Why do you change search modes here? I'm guessing historical reasons to match the existing $(NON_OMNIJAR_FILES) lists from the apps that define it? Whatever the case, this deserves at least a comment. Should we also consider a follow-up bug to remove this?

::: toolkit/mozapps/installer/l10n-repack.py
@@ +182,5 @@
> +    parser.add_argument('l10n',
> +                        help='Directory containing the staged langpack')
> +    parser.add_argument('--non-resource', nargs='+', metavar='PATTERN',
> +                        default=[],
> +                        help='Extra files not to be considered as resources')

But I think singular "non-resource" is OK here since the argument can occur multiple times.

Comment 3

[Mike Hommey [:glandium]]

(In reply to Gregory Szorc [:gps] from comment #2)
> Why do you change search modes here? I'm guessing historical reasons to
> match the existing $(NON_OMNIJAR_FILES) lists from the apps that define it?
> Whatever the case, this deserves at least a comment. Should we also consider
> a follow-up bug to remove this?

I was pondering doing that at the caller level, but I guess it can stay here until further improvements to the packaging code.

Comment 4

[Mike Hommey [:glandium]]

https://hg.mozilla.org/mozilla-central/rev/bf60e2782355