mac unification doesn't work with packaged addons

RESOLVED FIXED in Firefox 39

Status

defect
RESOLVED FIXED
4 years ago
Last year

People

(Reporter: Fallen, Assigned: Fallen)

Tracking

unspecified
mozilla40
All
macOS
Dependency tree / graph

Firefox Tracking Flags

(firefox39 fixed, firefox40 fixed)

Details

Attachments

(1 attachment, 6 obsolete attachments)

After bug 1147207, addons are packaged with a chrome.jar contains has all of the chrome content. Unfortunately when unifying builds that contain addons, there are two issues:

* The chrome.jar is considered a plain text file, unification will always fail
* install.rdf might contain <em:targetPlatform>, which will then always differ.

I've solved this by processing .jar files the same as already done for .xpi files and adding a special case for install.rdf to merge the target platforms.
Posted patch Fix - v1 (obsolete) β€” β€” Splinter Review
I'm pretty confident the try run will go green, so I'm uploading the patch for review already.

I'll need to have this backported to aurora/beta for the Thunderbird 38 release if possible.
Attachment #8591607 - Flags: review?(gps)
Attachment #8591607 - Flags: review?(gps) → review?(mh+mozilla)
Try builds are green, aside from a few I've starred that have references to other issues.
Comment on attachment 8591607 [details] [diff] [review]
Fix - v1

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

::: python/mozbuild/mozpack/unify.py
@@ +196,5 @@
> +                content1 = FIND_TARGET_PLATFORM.sub(lambda m: m.group(0) + platform2.group(0), content1)
> +            else:
> +                # If not, then just remove the target platform from the first file
> +                content1 = FIND_TARGET_PLATFORM.sub('', content1)
> +            return GeneratedFile(content1)

RDF fun: the em:* values can also be set as attributes of the Description element.

@@ +197,5 @@
> +            else:
> +                # If not, then just remove the target platform from the first file
> +                content1 = FIND_TARGET_PLATFORM.sub('', content1)
> +            return GeneratedFile(content1)
> +        elif path.endswith('.xpi') or path.endswith('.jar'):

That is very fishy. There should not be a .jar in the directory that is being packaged, as the jar is expected to be created by the packager. I'd prefer you investigate this instead of blindly handling them this way.
Attachment #8591607 - Flags: review?(mh+mozilla) → review-
(In reply to Mike Hommey [:glandium] from comment #4)
> Comment on attachment 8591607 [details] [diff] [review]
> Fix - v1
> 
> Review of attachment 8591607 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: python/mozbuild/mozpack/unify.py
> @@ +196,5 @@
> > +                content1 = FIND_TARGET_PLATFORM.sub(lambda m: m.group(0) + platform2.group(0), content1)
> > +            else:
> > +                # If not, then just remove the target platform from the first file
> > +                content1 = FIND_TARGET_PLATFORM.sub('', content1)
> > +            return GeneratedFile(content1)
> 
> RDF fun: the em:* values can also be set as attributes of the Description
> element.
Oh fun. I'll update the code to handle this.


> > +        elif path.endswith('.xpi') or path.endswith('.jar'):
> 
> That is very fishy. There should not be a .jar in the directory that is
> being packaged, as the jar is expected to be created by the packager. I'd
> prefer you investigate this instead of blindly handling them this way.

Ah sorry, I misstated. Its not the chrome.jar but instead the {calendar,lightning}-en-US.jar that's failing. I believe unzipping xpis is async now, so I'll try to fix this in comm-central by keeping them unpacked.
> (In reply to Mike Hommey [:glandium] from comment #4)
> > 
> > RDF fun: the em:* values can also be set as attributes of the Description
> > element.
> Oh fun. I'll update the code to handle this.

So IIUC then its valid to do <Description em:targetPlatform="Darwin_x86_64-gcc3">...</Description>. As its not possible to just add a further attribute with the same name and I don't think we should go into a full RDF parser here, don't you think its sufficient to support <em:targetPlatform> only as a tag? I could of course use the minidom parser and convert such attributes into tags, but it was my goal to modify the install.rdf as little as possible and only merge the target platforms.
Flags: needinfo?(mh+mozilla)
Posted patch Fix - v2 (obsolete) β€” β€” Splinter Review
Here is a new iteration, in case you are ok with keeping the <em:targetPlatform> detection as is.
Attachment #8591607 - Attachment is obsolete: true
Flags: needinfo?(mh+mozilla)
Attachment #8592242 - Flags: review?(mh+mozilla)
review ping, do you think we could get this in within the next few days so we can spin a Thunderbird beta with it?
Flags: needinfo?(mh+mozilla)
Comment on attachment 8592242 [details] [diff] [review]
Fix - v2

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

Sorry for the delay.

::: python/mozbuild/mozpack/test/test_unify.py
@@ +155,5 @@
> +                           '<em:targetPlatform>Darwin_x86_64-gcc3</em:targetPlatform>',
> +                           '<em:targetPlatform>Darwin_x86-gcc3</em:targetPlatform>'
> +                           ' ...'
> +                           ])),
> +                          ('distribution/extensions/emid2/install.rdf', '... no platform for a ...'),

I'm pretty sure in the case there's only one platform, it should be in install.rdf.

::: python/mozbuild/mozpack/unify.py
@@ +196,5 @@
> +                content1 = FIND_TARGET_PLATFORM.sub(lambda m: m.group(0) + platform2.group(0), content1)
> +            else:
> +                # If not, then just remove the target platform from the first file
> +                content1 = FIND_TARGET_PLATFORM.sub('', content1)
> +            return GeneratedFile(content1)

If you're not going to support em:targetPlatform as an argument, which is fine, you should explicitly reject it instead of letting it slip through.
Attachment #8592242 - Flags: review?(mh+mozilla) → feedback+
(In reply to Mike Hommey [:glandium] from comment #9)
> I'm pretty sure in the case there's only one platform, it should be in
> install.rdf.

Actually, no.
Flags: needinfo?(mh+mozilla)
Posted patch Fix - v3 (obsolete) β€” β€” Splinter Review
I went ahead and supported em:targetPlatform as attribute, because the regex for detecting it is not far off from the regex to fix it. The regex is not simple and there may be some edge cases I didn't cover. If you think we should go with a different approach let me know.

Not requesting review yet because I still have some try runs to complete, but I thought I'd upload this already for feedback on the regex.
Attachment #8592242 - Attachment is obsolete: true
Attachment #8595410 - Flags: feedback?(mh+mozilla)
Comment on attachment 8595410 [details] [diff] [review]
Fix - v3

The m-c try run went fairly well, there are a few failures but to me they don't look related. I've retriggered a few to see what happens:

https://treeherder.mozilla.org/#/jobs?repo=try&revision=db21abf9efa3

The c-c try run went well, although there was a calendar failure I don't think its not related to unification.

https://treeherder.mozilla.org/#/jobs?repo=try-comm-central&revision=93a09fd1ff86

Switching to review based on these results.
Attachment #8595410 - Flags: feedback?(mh+mozilla) → review?(mh+mozilla)
Comment on attachment 8595410 [details] [diff] [review]
Fix - v3

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

::: python/mozbuild/mozpack/unify.py
@@ +22,5 @@
>  import subprocess
>  from collections import OrderedDict
>  
> +FIND_TARGET_PLATFORM = re.compile(r'<(?:[-._0-9A-Za-z]+:)?targetPlatform>(?P<platform>[^<]*)</(?:[-._0-9A-Za-z]+:)?targetPlatform>')
> +FIND_TARGET_PLATFORM_ATTR = re.compile(r'(?P<tagattr><(?:[-._0-9A-Za-z]+:)?Description[^>]+?)(?P<ns>[-._0-9A-Za-z]+):targetPlatform=[\'"](?P<platform>[^\'"]+)[\'"](?P<restattrs>[^>]*?>)')

verbose regexp would be nicer (https://docs.python.org/2/library/re.html#re.X)

s/restattrs/otherattrs/

I think you can add a \s* between tagattr and targetPlatform so the extra space is removed.

@@ +205,5 @@
> +
> +            # Transform attribute based em:targetPlatform into tag based, if
> +            # applicable.
> +            content1 = transform_targetplatform_attr(content1)
> +            content2 = transform_targetplatform_attr(content2)

Considering the function is a one-liner, and that it's far, I'd rather it being inlined. You could do something like:

content1, content2 = (
    FIND_TARGET_PLATFORM_ATTR.sub(
        ...,
        f.open().read()
    ) for f in (file1, file2)
)

@@ +209,5 @@
> +            content2 = transform_targetplatform_attr(content2)
> +
> +            platform2 = FIND_TARGET_PLATFORM.search(content2)
> +            if platform2:
> +                print platform2.group(0)

remove debug output :)

@@ +212,5 @@
> +            if platform2:
> +                print platform2.group(0)
> +                # If the second file has a target platform, add it to the first
> +                # if it also has one.
> +                content1 = FIND_TARGET_PLATFORM.sub(lambda m: m.group(0) + platform2.group(0), content1)

You might as well just return GeneratedFile(FIND_TARGET_PLATFORM.sub(...))

@@ +215,5 @@
> +                # if it also has one.
> +                content1 = FIND_TARGET_PLATFORM.sub(lambda m: m.group(0) + platform2.group(0), content1)
> +            else:
> +                # If not, then just remove the target platform from the first file
> +                content1 = FIND_TARGET_PLATFORM.sub('', content1)

likewise

or, in fact, you could even do:

platform2 = FIND_TARGET_PLATFORM.search(content2)
return FIND_TARGET_PLATFORM.sub(lambda m: m.group(0) + platform2.group(0) if platform2 else '', content1)
Attachment #8595410 - Flags: review?(mh+mozilla) → feedback+
Posted patch Fix - v4 (obsolete) β€” β€” Splinter Review
All comments taken care of, added more tests and changed a few tests so I could check against a common variable.
Attachment #8595410 - Attachment is obsolete: true
Attachment #8596450 - Flags: review?(mh+mozilla)
Posted patch Fix - v4 (obsolete) β€” β€” Splinter Review
Sorry, wrong patch. Here is the right version.
Attachment #8596450 - Attachment is obsolete: true
Attachment #8596450 - Flags: review?(mh+mozilla)
Attachment #8596451 - Flags: review?(mh+mozilla)
Comment on attachment 8596451 [details] [diff] [review]
Fix - v4

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

::: python/mozbuild/mozpack/test/test_unify.py
@@ +177,5 @@
> +            '... <Description>',
> +            '<em:targetPlatform>Darwin_x86_64-gcc3</em:targetPlatform>',
> +            '<em:targetPlatform>Darwin_x86-gcc3</em:targetPlatform>',
> +            '</Description> ...'
> +        ])

Something else that would help readability, is, for the create_one calls, using something like:

x86_64 = 'Darwin_x86_64-gcc3'
x86 = 'Darwin_x86-gcc3'
platform = '... <Description><em:targetPlatform>%s</em:targetPlatform></Description> ...'
platform_attr = '... <Description em:targetPlatform="%s"> ...'

then use combinations:
  platform_attr % x86
  platform_attr % x86_64
  platform % x86
  platform % x86_64

Also, since paths are not part of the equation, you could choose something shorter than 'distribution/extensions/'

@@ +187,5 @@
> +                          ('distribution/extensions/emid4/install.rdf', '... no platform for a ...'),
> +                          ('distribution/extensions/emid5/install.rdf', both_platforms_merged),
> +                          ('distribution/extensions/emid6/install.rdf', both_platforms_merged),
> +                          ('distribution/extensions/emid7/install.rdf', both_platforms_merged),
> +                          ('distribution/extensions/emid8/install.rdf', '... <Description> ...'),

it would probably feel less weird if there was a </Description> here.

I think I'd be more comfortable with tests that have dummy attrs and sub elements to <Description>, such that no test result doesn't have a <Description>.

::: python/mozbuild/mozpack/unify.py
@@ +25,5 @@
> +# Regular expressions for unifying install.rdf
> +FIND_TARGET_PLATFORM = re.compile(r"""
> +    <(?:[-._0-9A-Za-z]+:)?targetPlatform>           # The targetPlatform tag, with any namespace
> +    (?P<platform>[^<]*)                             # The actual platform value
> +    </(?:[-._0-9A-Za-z]+:)?targetPlatform>          # The closing tag

python regexp are fancy, you can even make it match the same namespace. (search for backreference on https://docs.python.org/2/library/re.html)

@@ +29,5 @@
> +    </(?:[-._0-9A-Za-z]+:)?targetPlatform>          # The closing tag
> +    """, re.X)
> +FIND_TARGET_PLATFORM_ATTR = re.compile(r"""
> +    (?P<tag><(?:[-._0-9A-Za-z]+:)?Description)\s+   # The opening part of the <Description> tag
> +    (?P<attrs>[^>]*?)                               # The initial attributes

There should be a \s* somewhere around here or later, so that <Description foo="" targetPlatform="bar" bar="baz"> doesn't end up as <Description foo=""  bar="baz"> (with two spaces between the end of the foo attr, and the beginning of bar)

Actually, moving the \s+ from above here should be enough.

@@ +30,5 @@
> +    """, re.X)
> +FIND_TARGET_PLATFORM_ATTR = re.compile(r"""
> +    (?P<tag><(?:[-._0-9A-Za-z]+:)?Description)\s+   # The opening part of the <Description> tag
> +    (?P<attrs>[^>]*?)                               # The initial attributes
> +    (?P<ns>[-._0-9A-Za-z]+):targetPlatform=         # The targetPlatform attribute, with any namespace

the colon should be part of the capture for the namespace, and the namespace should be optional, so that if there isn't a namespace prefix for some reason, it still works properly (it's theoretically possible, if Description is prefixed and the extension manager namespace is made the default ; that's what you get from doing xml parsing with regexps ;) ).

@@ -36,4 @@
>              return True
>      return False
>  
> -

Don't remove this empty line. PEP8 wants two lines between classes
Attachment #8596451 - Flags: review?(mh+mozilla) → feedback+
Posted patch Fix - v5 (obsolete) β€” β€” Splinter Review
Here is a patch with comments taken care of. Not sure I did enough on the sub-elements and attributes part, I made sure that the Description tag is closed but I didn't add any further atrributes. Let me know if I should add in a few more there.
Attachment #8596451 - Attachment is obsolete: true
Attachment #8597132 - Flags: review?(mh+mozilla)
Comment on attachment 8597132 [details] [diff] [review]
Fix - v5

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

::: python/mozbuild/mozpack/test/test_unify.py
@@ +129,5 @@
>  
> +        # Test install.rdf unification
> +        x86_64 = 'Darwin_x86_64-gcc3'
> +        x86 = 'Darwin_x86-gcc3'
> +        platform = '... <Description><em:targetPlatform>%s</em:targetPlatform></Description> ...'

Please make the tests use more attrs and sub-elements.

Something like

<Description em:bar="bar" em:qux="qux"><em:foo>foo</em:foo><em:targetPlatform>platform</em:targetPlatform><em:baz>baz</em:baz></Description>

and

<Description em:bar="bar" em:targetPlatform="platform" em:qux="qux"><em:foo>foo</em:foo><em:baz>baz</em:baz></Description>

Ideally, you should test with and without namespaces on Description, and with and without the "em:" prefix (do that in a loop?)
Attachment #8597132 - Flags: review?(mh+mozilla) → review+
Posted patch Fix - v6 β€” β€” Splinter Review
Testing all combinations of namespaces now. Putting together all the strings ended up being a lot of lines, so I decided to make it a bit more readable by putting the expected result closer to the input. I hope you like it :)


Approval Request Comment
[Feature/regressing bug #]: Feature
[User impact if declined]: Cannot package Lightning with Thunderbird 38
[Describe test coverage new/current, TreeHerder]: Fairly thorough test, previous iteration has successful try run. Test passes locally on mac.
[Risks and why]: Default theme is the only packaged addon in Firefox, but it has no target platform so it won't be modified.
[String/UUID change made/needed]: none
Attachment #8597132 - Attachment is obsolete: true
Attachment #8597751 - Flags: review+
Attachment #8597751 - Flags: approval-mozilla-release?
Attachment #8597751 - Flags: approval-mozilla-beta?
Attachment #8597751 - Flags: approval-mozilla-aurora?
Oh, about approval-mozilla-release, not sure if its needed to be pushed to the branch the current betas are being built on.
Comment on attachment 8597751 [details] [diff] [review]
Fix - v6

I am sorry but this is too late in the 38 cycle. Next beta is b9, that is mean if we have a regression, we would fix in the rc. We cannot take this risk.
This will have to be managed in the tb repository itself.
Attachment #8597751 - Flags: approval-mozilla-release?
Attachment #8597751 - Flags: approval-mozilla-release-
Attachment #8597751 - Flags: approval-mozilla-beta?
Attachment #8597751 - Flags: approval-mozilla-beta-
Attachment #8597751 - Flags: approval-mozilla-aurora?
Attachment #8597751 - Flags: approval-mozilla-aurora+
https://hg.mozilla.org/mozilla-central/rev/b85a60205e15
Status: ASSIGNED → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla40
For the record, landed this on http://hg.mozilla.org/releases/mozilla-release/rev/f7170ad49667 which is on the THUNDERBIRD_38_VERBRANCH branch (and therefore does not affect Firefox)
Sorry, wrong changeset. I don't know where that came from. http://hg.mozilla.org/releases/mozilla-release/rev/f743fa7d9b84

Updated

Last year
Product: Core → Firefox Build System
You need to log in before you can comment on or make changes to this bug.