Closed
Bug 1101237
Opened 10 years ago
Closed 9 years ago
Remove distribution directory during install
Categories
(Thunderbird :: Installer, defect)
Tracking
(Not tracked)
RESOLVED
FIXED
Thunderbird 40.0
People
(Reporter: robert.strong.bugs, Assigned: jsbruner)
References
Details
Attachments
(1 file, 1 obsolete file)
1.07 KB,
patch
|
bbondy
:
review+
|
Details | Diff | Splinter Review |
Bug 1098677 made it so apps need to remove the distribution directory on install instead of using the common macro.
Reporter | ||
Comment 1•10 years ago
|
||
If you could own this bug and get it landed I would appreciate it.
Attachment #8524940 -
Flags: review?(josiah)
Assignee | ||
Updated•9 years ago
|
Assignee: josiah → nobody
Assignee | ||
Comment 4•9 years ago
|
||
Comment on attachment 8524940 [details] [diff] [review] Patch rev1 I instead meant to remove myself as the reviewer for three reasons. A) I'm very busy for another week or so. B) I don't actually have a Windows machine, and Windows VM builds take forever. And C) Because I don't have a ton of knowledge of the Windows installer/build stuff. Joshua is probably a better reviewer for all of those reasons. :)
Attachment #8524940 -
Flags: review?(josiah) → review?(Pidgeot18)
Comment 5•9 years ago
|
||
Comment on attachment 8524940 [details] [diff] [review] Patch rev1 Review of attachment 8524940 [details] [diff] [review]: ----------------------------------------------------------------- To be frank, I don't particularly know Windows installer logic at all, so I'm going to delegate review to Mike Hommey or someone else who knows this stuff.
Attachment #8524940 -
Flags: review?(Pidgeot18) → review?(mh+mozilla)
Reporter | ||
Comment 6•9 years ago
|
||
You could ask Brian Bondy (bbondy)
Updated•9 years ago
|
Attachment #8524940 -
Flags: review?(netzen)
Comment 7•9 years ago
|
||
Comment on attachment 8524940 [details] [diff] [review] Patch rev1 Review of attachment 8524940 [details] [diff] [review]: ----------------------------------------------------------------- ::: mail/installer/windows/nsis/installer.nsi @@ +228,5 @@ > + ${EndUnless} > + > + ${If} ${FileExists} "$INSTDIR\distribution" > + ${AndIf} $0 != "false" > + RmDir /r "$INSTDIR\distribution" I think it's safer here to check for equaling a specific value? like == "true".
Attachment #8524940 -
Flags: review?(netzen)
Reporter | ||
Comment 8•9 years ago
|
||
The removal is preferred over keeping it on pave over install.
Comment 9•9 years ago
|
||
Comment on attachment 8524940 [details] [diff] [review] Patch rev1 I'd rather someone who already knows nsi look at this.
Attachment #8524940 -
Flags: review?(mh+mozilla)
Assignee | ||
Comment 10•9 years ago
|
||
(In reply to Mike Hommey [:glandium] from comment #9) > Comment on attachment 8524940 [details] [diff] [review] > Patch rev1 > > I'd rather someone who already knows nsi look at this. Who would that be? Splinter's "Suggested Reviewers" is not very helpful.
Flags: needinfo?(mh+mozilla)
Comment 11•9 years ago
|
||
(In reply to Josiah Bruner [:JosiahOne] (needinfo! - Won't respond to CC)) from comment #10) > (In reply to Mike Hommey [:glandium] from comment #9) > > Comment on attachment 8524940 [details] [diff] [review] > > Patch rev1 > > > > I'd rather someone who already knows nsi look at this. > > Who would that be? Splinter's "Suggested Reviewers" is not very helpful. cf. comment 6?
Flags: needinfo?(mh+mozilla)
Assignee | ||
Updated•9 years ago
|
Attachment #8524940 -
Flags: review?(netzen)
Comment 12•9 years ago
|
||
Comment on attachment 8524940 [details] [diff] [review] Patch rev1 See comment 7
Attachment #8524940 -
Flags: review?(netzen)
Assignee | ||
Comment 13•9 years ago
|
||
Whoops, sorry. Made that change.
Attachment #8524940 -
Attachment is obsolete: true
Attachment #8598081 -
Flags: review?(netzen)
Assignee | ||
Comment 14•9 years ago
|
||
Try build verifying that it doesn't break builds or tests: https://treeherder.mozilla.org/#/jobs?repo=try-comm-central&revision=c8e206ff0e9f
Comment 15•9 years ago
|
||
Comment on attachment 8598081 [details] [diff] [review] Patch (Rev 2) Review of attachment 8598081 [details] [diff] [review]: ----------------------------------------------------------------- Thanks, I think there's one more change which I didn't notice before. ::: mail/installer/windows/nsis/installer.nsi @@ +226,5 @@ > + ReadINIStr $0 $1 "Install" "RemoveDistributionDir" > + ${EndIf} > + ${EndUnless} > + > + ${If} ${FileExists} "$INSTDIR\distribution" I think this block should go inside the Unless/EndUnless.
Attachment #8598081 -
Flags: review?(netzen)
Reporter | ||
Comment 16•9 years ago
|
||
Brian, just to verify that this will do the right thing... the distribution directory is supposed to be removed by default. This way if a person installs a partner build and then installs a non partner build the customizations are removed. So, if there isn't an ini file it should be removed and if there is an ini file it should be removed unless the ini specifies false
Comment 17•9 years ago
|
||
Comment on attachment 8524940 [details] [diff] [review] Patch rev1 Ah I thought we only wanted to do it if the INI was specified. OK then the original patch was OK. Thanks for clarifying Robert. Un-obsoleting the first patch and I'll obsolete the 2nd patch.
Attachment #8524940 -
Attachment is obsolete: false
Attachment #8524940 -
Flags: review+
Updated•9 years ago
|
Attachment #8598081 -
Attachment is obsolete: true
Assignee | ||
Comment 18•9 years ago
|
||
Thanks guys! Fixed -> https://hg.mozilla.org/comm-central/rev/99d4ab3b8b6f
Status: ASSIGNED → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
Target Milestone: --- → Thunderbird 40.0
You need to log in
before you can comment on or make changes to this bug.
Description
•