NON_OMNIJAR_FILES is not handled by the new packager

RESOLVED FIXED in mozilla21

Status

()

Core
Build Config
RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: glandium, Assigned: glandium)

Tracking

Trunk
mozilla21
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment)

(Assignee)

Description

5 years ago
It turns out at least thunderbird needs to declare additional NON_OMNIJAR_FILES.
(Assignee)

Comment 1

5 years ago
Created attachment 705803 [details] [diff] [review]
Use NON_OMNIJAR_FILES value in the new packager
Attachment #705803 - Flags: review?(gps)

Comment 2

5 years ago
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.
Attachment #705803 - Flags: review?(gps) → review+
(Assignee)

Comment 3

5 years ago
(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.
(Assignee)

Comment 4

5 years ago
https://hg.mozilla.org/mozilla-central/rev/bf60e2782355
Status: NEW → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla21
You need to log in before you can comment on or make changes to this bug.