Build scripts should be able to handle @BINPATH@/extensions/testpilot@labs.mozilla.com/* in package-manifest.in for the installer

RESOLVED FIXED in mozilla2.0b3

Status

()

Toolkit
NSIS Installer
RESOLVED FIXED
8 years ago
8 years ago

People

(Reporter: rstrong, Assigned: rstrong)

Tracking

Trunk
mozilla2.0b3
x86
Windows 7
Points:
---
Bug Flags:
in-testsuite -
in-litmus -

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(4 attachments, 3 obsolete attachments)

Created attachment 455072 [details] [diff] [review]
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/*.
Attachment #455072 - Flags: feedback?(ted.mielczarek)
Created attachment 455073 [details] [diff] [review]
patch

Actually, this might be good enough for review. What do you think about removing support for copying directories recursively without the * wildcard?
Attachment #455072 - Attachment is obsolete: true
Attachment #455073 - Flags: review?(ted.mielczarek)
Attachment #455072 - Flags: feedback?(ted.mielczarek)
Created attachment 455086 [details] [diff] [review]
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.
Attachment #455086 - Flags: review?(ted.mielczarek)
Created attachment 455087 [details] [diff] [review]
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.
Attachment #455073 - Attachment is obsolete: true
Attachment #455087 - Flags: review?(ted.mielczarek)
Attachment #455073 - Flags: review?(ted.mielczarek)
Attachment #455086 - Attachment is patch: true
Attachment #455086 - Attachment mime type: application/octet-stream → text/plain
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.
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.
Created attachment 455088 [details] [diff] [review]
Firefox package-manifest.in patch (checked in)

Reverts the package-manifest.in to what it was before bug 575566 landed.
Attachment #455088 - Flags: review?(ted.mielczarek)
Created attachment 455091 [details] [diff] [review]
SeaMonkey patch (checked in)
Attachment #455091 - Flags: review?(bugspam.Callek)
Created attachment 455092 [details] [diff] [review]
Thunderbird patch (checked in)
Attachment #455092 - Flags: review?(bugspam.Callek)
I've also verified that gloda, etc. gets packaged properly for SeaMonkey and Thunderbird in both the Win installer and Win zip builds.
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.
Attachment #455087 - Attachment is obsolete: true
Attachment #455087 - Flags: review?(ted.mielczarek)
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.
(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.
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.
Replacing Packager.pm is bug 511648, and khuey has a start there.
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.

Updated

8 years ago
Attachment #455092 - Flags: review?(bugspam.Callek)
Comment on attachment 455091 [details] [diff] [review]
SeaMonkey patch (checked in)

Clearing review requests pending m-c adopting this approach.
Attachment #455091 - Flags: review?(bugspam.Callek)
Wow... I'm not going to land the comm-central changes unless mozilla-central adopts this approach... it is just common sense.
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.
Attachment #455092 - Flags: review?(philringnalda)
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.
Attachment #455091 - Attachment is obsolete: true
Attachment #455092 - Flags: review?(philringnalda) → review+
Comment on attachment 455092 [details] [diff] [review]
Thunderbird patch (checked in)

Yay!
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.
Attachment #455091 - Flags: review+

Updated

8 years ago
Attachment #455091 - Attachment is obsolete: false
Attachment #455086 - Flags: review?(ted.mielczarek) → review+
Comment on attachment 455088 [details] [diff] [review]
Firefox package-manifest.in patch (checked in)

Nice. I hate Packager.pm so much.
Attachment #455088 - Flags: review?(ted.mielczarek) → review+
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.
Attachment #455086 - Flags: approval2.0?
Attachment #455088 - Flags: approval2.0?

Updated

8 years ago
Attachment #455086 - Flags: approval2.0? → approval2.0+

Updated

8 years ago
Attachment #455088 - Flags: approval2.0? → approval2.0+
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
Attachment #455086 - Attachment description: patch - use absolute paths → patch - use absolute paths (checked in)
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
Attachment #455088 - Attachment description: Firefox package-manifest.in patch → Firefox package-manifest.in patch (checked in)
Comment on attachment 455091 [details] [diff] [review]
SeaMonkey patch (checked in)

Pushed to comm-central
http://hg.mozilla.org/comm-central/rev/1bd82beb0eff
Attachment #455091 - Attachment description: SeaMonkey patch → SeaMonkey patch (checked in)
Comment on attachment 455092 [details] [diff] [review]
Thunderbird patch (checked in)

Pushed to comm-central
http://hg.mozilla.org/comm-central/rev/6fe0b79da992
Attachment #455092 - Attachment description: Thunderbird patch → Thunderbird patch (checked in)
All patches checked in
Status: NEW → RESOLVED
Last Resolved: 8 years ago
Flags: in-testsuite-
Flags: in-litmus-
Resolution: --- → FIXED
Target Milestone: --- → mozilla2.0b3

Updated

8 years ago
Blocks: 582597
You need to log in before you can comment on or make changes to this bug.