Update (re)packaging code to deal with extracting the installer vs just the zip.

RESOLVED FIXED in seamonkey2.1final

Status

RESOLVED FIXED
7 years ago
7 years ago

People

(Reporter: Callek, Assigned: Callek)

Tracking

unspecified
seamonkey2.1final
x86
Windows 7

Firefox Tracking Flags

(firefox5-, blocking-seamonkey2.1 final+)

Details

Attachments

(1 attachment, 1 obsolete attachment)

(Assignee)

Description

7 years ago
Created attachment 535831 [details] [diff] [review]
v0.5

The mozilla-side omnijar code uses the .exe to repackage installer, and that code also changed the unpack steps for the complete-mar generation during repackaging.

I have a patch, want feedback.

So far I have only tested that it *seems* to do what I want, and "compiles" for a repack locally, did not yet run the updates/repack tests on it.

Assigning feedback requests to a few people, since this *is* complex code. Patch is against m-2.0, but the rapid-release-repos have not changed much in this codepath afaict.
Attachment #535831 - Flags: feedback?(nrthomas)
Attachment #535831 - Flags: feedback?(mwu)
Attachment #535831 - Flags: feedback?(kairo)
Attachment #535831 - Flags: feedback?(bhearsum)
(Assignee)

Updated

7 years ago
blocking-seamonkey2.1: --- → final+

Comment 1

7 years ago
Why the change from double quotes to single quotes?
Why are you changing the nsinstall manifest?
(Assignee)

Comment 3

7 years ago
(In reply to comment #2)
> Why are you changing the nsinstall manifest?

not intentional, feel free to ignore it.

Comment 4

7 years ago
Comment on attachment 535831 [details] [diff] [review]
v0.5

Looks kinds hackish but I guess we need it if we want to support optional extensions in the installer, which SeaMonkey probably still wants. As SeaMonkey 2.1 needs this in the mozilla-2.0 branch, I'm not sure how this can land anyhow, but I guess it should be there yesterday... ;-(

The packager.mk change looks usable - as I said kinds hackish though.
Attachment #535831 - Flags: feedback?(kairo) → feedback+
Comment on attachment 535831 [details] [diff] [review]
v0.5

Ignoring the first hunk, I realize I know this code very little. Lets see what mwu says.
Attachment #535831 - Flags: feedback?(nrthomas)
Comment on attachment 535831 [details] [diff] [review]
v0.5

I'm not an appropriate reviewer for this.
Attachment #535831 - Flags: feedback?(bhearsum)

Comment 7

7 years ago
Comment on attachment 535831 [details] [diff] [review]
v0.5

>+  mv core $(MOZ_PKG_DIR) && \
>+  (if [ -d optional ]; then ls -1 optional > ../optional_file_list.txt; mv optional/* $(MOZ_PKG_DIR); fi;)

Hmm, in the SeaMonkey case, the only entry under optional/ should be distribution/ so I wonder if core/ also contains a distribution/ this might fail because mv cannot overwrite an existing distribution/ with a new one. Maybe we should copy here instead (and, if needed to get rid of the original, then just |rm -rf optional|).

Comment 8

7 years ago
(In reply to comment #7)
> >+  (if [ -d optional ]; then ls -1 optional > ../optional_file_list.txt; mv optional/* $(MOZ_PKG_DIR); fi;)

I replaced that line with this on the questionable machine:

  (if [ -d optional ]; then ls -1 optional > ../optional_file_list.txt; cp -rp optional/* $(MOZ_PKG_DIR); rm -rf optional; fi;)

...and the before-failing repackage-zip call succeeded on that machine.

I think we should just try and go with that one instead. Callek?

Comment 9

7 years ago
Callek gave me an rs+ via email and I deployed http://hg.mozilla.org/releases/mozilla-2.0/rev/ff616f6ced1835adf3a50ad2eda5034f8693b732 (SeaMonkey-specific relbranch). I *think* the outcome of the redone repacks means that this works fine, but I'll let Callek chime in and update the patch here.

Comment 10

7 years ago
Comment on attachment 535831 [details] [diff] [review]
v0.5

>+  (if [ -d optional ]; then ls -1 optional > ../optional_file_list.txt; mv optional/* $(MOZ_PKG_DIR); fi;)

How about |find optional|? It'll leave less junk in the list than ls -l.

Another option would be to use the packaging manifest and MOZ_OPTIONAL_PKG_LIST to identify what belongs in optional, but I don't know if that would work due to whatever we do in packaging. That would be preferred if possible since it would make the question of whether you unpack the installer or the zip not matter anymore. But, I don't feel too strongly either way.
Attachment #535831 - Flags: feedback?(mwu) → feedback+
(Assignee)

Comment 11

7 years ago
CC-ing joduinn in the hopes that this might be able to land for Firefox5//moz-beta.
tracking-firefox5: --- → ?
(Assignee)

Comment 12

7 years ago
Created attachment 539112 [details] [diff] [review]
tested version

This is the tested version we used on SeaMonkey 2.1 and works fine for Gecko 5.

The OPTIONAL method suggested will actually make this code cleaner/better in the end, but I don't want to break what I know works (even though would be nicer) in the short amount of time I have to devote to this, and the timeframe I need to get this in.
Attachment #535831 - Attachment is obsolete: true
Attachment #539112 - Flags: review?(khuey)
Comment on attachment 539112 [details] [diff] [review]
tested version

This looks good to me.

Nominating for beta approval for Callek.
Attachment #539112 - Flags: review?(khuey)
Attachment #539112 - Flags: review+
Attachment #539112 - Flags: approval-mozilla-beta?

Comment 14

7 years ago
Comment on attachment 539112 [details] [diff] [review]
tested version

Nominating for aurora as well (Callek, I have tried and it does apply fine). SeaMonkey will need this in one way or another (regular or relbranch) for all builds that have repacks, on all branches.
Attachment #539112 - Flags: approval-mozilla-aurora?

Updated

7 years ago
Attachment #539112 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+

Comment 15

7 years ago
Comment on attachment 539112 [details] [diff] [review]
tested version

Actually, I think we want to let this bake on m-c first before landing on aurora or beta
Attachment #539112 - Flags: approval-mozilla-aurora+ → approval-mozilla-aurora?
(Assignee)

Comment 16

7 years ago
Landed on m-c for Gecko 7
http://hg.mozilla.org/mozilla-central/rev/54f18b2b93d2

TODO:
* Keep an eye on repacks, make sure that none fail due to a strange edge-case I failed to see/test properly.
* Get approval's triaged.
* Land on branches [either relbranch or default based on approval step]

(marking t-m for SeaMonkey2.1 as this landed on the relbranch for that)
Status: NEW → RESOLVED
Last Resolved: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → seamonkey2.1final

Comment 17

7 years ago
Comment on attachment 539112 [details] [diff] [review]
tested version

Approved for aurora, we're still debating on beta
Attachment #539112 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Too late for FF5 (it goes out tomorrow!)
tracking-firefox5: ? → -
Attachment #539112 - Flags: approval-mozilla-beta? → approval-mozilla-beta-
(Assignee)

Updated

7 years ago
Depends on: 666518
(Assignee)

Comment 19

7 years ago
(In reply to comment #16)
> Landed on m-c for Gecko 7
> http://hg.mozilla.org/mozilla-central/rev/54f18b2b93d2

Backout out on -inbound due to SeaMokey Bug 666518 no longer requiring this.

> TODO:
> * Keep an eye on repacks, make sure that none fail due to a strange
> edge-case I failed to see/test properly.

This was done, and no problems.

> * Get approval's triaged.
> * Land on branches [either relbranch or default based on approval step]

* I got aurora approval, but never landed there, won't now.
* Landed on a relbranch of m-beta for SM2.2b1 [http://hg.mozilla.org/releases/mozilla-beta/rev/ac0dae3c3041] but won't reuse that relbranch, so no need to backout there.

> (marking t-m for SeaMonkey2.1 as this landed on the relbranch for that)

Would mark WONTFIX but this was shipped in 2.1 final, so technically it was fixed.

Updated

7 years ago
Attachment #539112 - Flags: approval-mozilla-aurora+ → approval-mozilla-aurora-
(Assignee)

Updated

7 years ago
Duplicate of this bug: 646309
You need to log in before you can comment on or make changes to this bug.