575838 - Build scripts should be able to handle @BINPATH@/extensions/[e-mail]/* in package-manifest.in for the installer

Status: RESOLVED FIXED

(Firefox :: Installer, defect)

Description

[Robert Strong (they/them - no direct email)]

Spinoff of bug 575566.

Comment 1

[Robert Strong (they/them - no direct email)]

Attachment: Patch in progress

Ted, turns out do_copydir was setting the working directory and since there was a todo to "pass individual files to do_copyfile, not do_copydir" I decided to implement that as simply as it could be. Also, Packager.pm expects full paths and we've been passing relative paths for the installer for just about ever. Turns out that Packager.pm can also take a dir and perform a recursive copy which I don't think I like... I think anyone wanting to recursively copy should explicitly do so with dirname/*.

Comment 2

[Robert Strong (they/them - no direct email)]

Attachment: patch

Actually, this might be good enough for review. What do you think about removing support for copying directories recursively without the * wildcard?

Comment 3

[Robert Strong (they/them - no direct email)]

Attachment: patch - use absolute paths (checked in)

Turns out that just using absolute paths when packaging also fixes this. The additional cleanup is for a couple of incorrect indents in the file where PACKAGER_COPY is called.

Comment 4

[Robert Strong (they/them - no direct email)]

Attachment: Packager.pm cleanup

I'm fine with or without the changes to Packager.pm though I do think this is cleaner. Still think that the "# if it's a directory, do recursive copy." should be removed and instead just require package manifests to explicitly specify /* when including directories.

Comment 5

[Robert Strong (they/them - no direct email)]

Also, the removal of MOZ_OPTIONAL_PKG_LIST is just cleanup. MOZ_OPTIONAL_PKG_LIST doesn't need to be specified when there are no optional files because of bug 398867.

Comment 6

[Robert Strong (they/them - no direct email)]

If we don't go with the Packager.pm changes I'd like to add a check that dies if the path passed in is relative.

Comment 7

[Robert Strong (they/them - no direct email)]

Attachment: Firefox package-manifest.in patch (checked in)

Reverts the package-manifest.in to what it was before bug 575566 landed.

Comment 8

[Robert Strong (they/them - no direct email)]

Attachment: SeaMonkey patch (checked in)

Comment 9

[Robert Strong (they/them - no direct email)]

Attachment: Thunderbird patch (checked in)

Comment 10

[Robert Strong (they/them - no direct email)]

I've also verified that gloda, etc. gets packaged properly for SeaMonkey and Thunderbird in both the Win installer and Win zip builds.

Comment 11

[Phil Ringnalda (:philor)]

If you drop support for directories without a *, there's a copied-around chunk of "; Mac bundle stuff" at the top of everyone's package-manifest that needs *s.

Comment 12

[Robert Strong (they/them - no direct email)]

Comment on attachment 455087 [details] [diff] [review]
Packager.pm cleanup

Probably best to do cleanup of Packager.pm in another bug if at all... personally, I think a python script specifically for installer packaging would be a cleaner way to go.

Thanks Philor, I saw that as well and am glad to have a second set of eyes looking at the changes.

Comment 13

[Justin Wood (:Callek)]

(In reply to comment #3)
> Created an attachment (id=455086) [details]
> patch - use absolute paths
> 
> Turns out that just using absolute paths when packaging also fixes this. The
> additional cleanup is for a couple of incorrect indents in the file where
> PACKAGER_COPY is called.

I wonder if this behaves differently with pymake, vs. GNUMake.

Comment 14

[Robert Strong (they/them - no direct email)]

Possibly... note in Packager.pm reads as follows:
NOTE: Source and destination directories must be absolute paths.
  Relative paths will NOT work.  Also, the destination directory
  must NOT be a subdirectory of the source directory.

so it should be changed to absolute paths even if it does.

Comment 15

[(not currently active) Ted Mielczarek]

Replacing Packager.pm is bug 511648, and khuey has a start there.

Comment 16

[Robert Strong (they/them - no direct email)]

Thanks for the bug number and awesome. I still think we should take the relative path to absolute path patch along with the manifest changes.

Comment 17

[Justin Wood (:Callek)]

Comment on attachment 455091 [details] [diff] [review]
SeaMonkey patch (checked in)

Clearing review requests pending m-c adopting this approach.

Comment 18

[Robert Strong (they/them - no direct email)]

Wow... I'm not going to land the comm-central changes unless mozilla-central adopts this approach... it is just common sense.

Comment 19

[Robert Strong (they/them - no direct email)]

Comment on attachment 455092 [details] [diff] [review]
Thunderbird patch (checked in)

Hey Phil, I'm hoping to get these changes into mozilla-central relatively soon and I'm more than willing to provide a Thunderbird patch for these changes and to land it at the same time as I land the mozilla-central changes.

Comment 20

[Robert Strong (they/them - no direct email)]

Comment on attachment 455091 [details] [diff] [review]
SeaMonkey patch (checked in)

Since this cleanup isn't critical to SeaMonkey when this change lands this can be handled in a separate bug for SeaMonkey.

Comment 21

[Phil Ringnalda (:philor)]

Comment on attachment 455092 [details] [diff] [review]
Thunderbird patch (checked in)

Yay!

Comment 22

[Justin Wood (:Callek)]

Comment on attachment 455091 [details] [diff] [review]
SeaMonkey patch (checked in)

I just wasn't sure from bug that ted was wanting this approach. So was holding off until I could officially test myself, but if philor is willing to review tb side without testing, I'll hit this now.

Comment 23

[(not currently active) Ted Mielczarek]

Comment on attachment 455088 [details] [diff] [review]
Firefox package-manifest.in patch (checked in)

Nice. I hate Packager.pm so much.

Comment 24

[Robert Strong (they/them - no direct email)]

Comment on attachment 455086 [details] [diff] [review]
patch - use absolute paths (checked in)

Drivers, this makes it so the installer uses absolute paths when packaging which is the same way that our other packages use it.

Comment 25

[Robert Strong (they/them - no direct email)]

Comment on attachment 455086 [details] [diff] [review]
patch - use absolute paths (checked in)

Pushed to mozilla-central
http://hg.mozilla.org/mozilla-central/rev/bf4cec83e4d2

Comment 26

[Robert Strong (they/them - no direct email)]

Comment on attachment 455088 [details] [diff] [review]
Firefox package-manifest.in patch (checked in)

Pushed to mozilla-central

Still need to push the SeaMonkey and Thunderbird patches so leaving open for the moment

Comment 27

[Robert Strong (they/them - no direct email)]

Comment on attachment 455091 [details] [diff] [review]
SeaMonkey patch (checked in)

Pushed to comm-central
http://hg.mozilla.org/comm-central/rev/1bd82beb0eff

Comment 28

[Robert Strong (they/them - no direct email)]

Comment on attachment 455092 [details] [diff] [review]
Thunderbird patch (checked in)

Pushed to comm-central
http://hg.mozilla.org/comm-central/rev/6fe0b79da992

Comment 29

[Robert Strong (they/them - no direct email)]

All patches checked in