Closed Bug 1029513 Opened 6 years ago Closed 5 years ago

Stop building Venkman because Bug 800200 removes JSD1 which Venkman depends on.

Categories

(SeaMonkey :: Build Config, defect)

defect
Not set

Tracking

(Not tracked)

RESOLVED FIXED
seamonkey2.30

People

(Reporter: philip.chee, Assigned: ewong)

References

Details

Attachments

(3 files, 4 obsolete files)

No description provided.
Assignee: nobody → ewong
Status: NEW → ASSIGNED
Attached patch Remove Venkman from suite/ (v1) (obsolete) — Splinter Review
Attachment #8445659 - Flags: review?(neil)
Comment on attachment 8445660 [details] [diff] [review]
Remove Venkman from suite/ (help patch) (v1)

Sad to see but looks OK.
Attachment #8445660 - Flags: review?(jh) → review+
Comment on attachment 8445659 [details] [diff] [review]
Remove Venkman from suite/ (v1)

>-  for index in ['CHATZILLA', 'INSPECTOR', 'VENKMAN', 'COMM', 'MOZILLA',
>+  for index in ['CHATZILLA', 'INSPECTOR', 'COMM', 'MOZILLA',
>                 'LDAPSDKS']:
[Nit: Might be able to put this on one line now.]

>diff --git a/suite/branding/nightly/icons/svg/venkman-window.svg b/suite/branding/nightly/icons/svg/venkman-window.svg
>deleted file mode 100644
Why don't I see any other deleted icons?

>-@BINPATH@/components/jsdebugger.xpt
> @BINPATH@/components/jsdservice.xpt
You removed the wrong file, I think.

>diff --git a/suite/installer/removed-files.in b/suite/installer/removed-files.in
Should we be adding stuff here, rather than removing it?

>       ${EndIf}
>     ${EndIf}
> 
>-    ; If Venkman is installed and this install includes Venkman remove it
>-    ; from the installation directory. This will remove it if the user
>-    ; deselected Venkman on the components page.
...
>-      ${EndIf}
>-    ${EndIf}
>   ${EndIf}
Nit: extraneous blank line.

>   ${EndIf}
> 
>-  ${If} ${FileExists} "$EXEDIR\optional\distribution\extensions\{f13b157f-b174-47e7-a34d-4815ddfdfeb8}.xpi"
...
>-  ${EndIf}
> FunctionEnd
Nit: extraneous blank line.
Attachment #8445659 - Flags: feedback?(bugzilla)
>>diff --git a/suite/branding/nightly/icons/svg/venkman-window.svg b/suite/branding/nightly/icons/svg/venkman-window.svg
>>deleted file mode 100644
> Why don't I see any other deleted icons?
I *think* we should keep the .svg files in the tree for future reference. The PNGs and ICOs generated can all go however.
Q: How will we make sure the Venkman addon gets disabled on newer SeaMonkey builds? Can we do this via addons.m.o?
(In reply to Frank Wein [:mcsmurf] from comment #7)
> Q: How will we make sure the Venkman addon gets disabled on newer SeaMonkey
> builds? Can we do this via addons.m.o?

We can mark venkman only compatible up to the version where we need to deprecate it. We should test if that is enough to disable it where it's already installed.
Venkman already has not had it's max-compatibility upgraded since Gecko 14. However, max-compatibility has been ignored since Gecko 10.  In order to keep it from being installed, the max compatibility should be upped to Gecko 31 (or whatever the last version is) and then strictCompatibility should be set to true.
Note that this will likely require a new release of Venkman extension with the install.RDF modified for those restrictions, but users with Addons updates disabled may experience problems unless the extension is blocklisted.  If crashes are anticipated, Mozilla might have to consider blocking all extensions that use JSD after Gecko 32.
Jorgev,

where-for-bug/what do you need to know if we wanted Venkman blacklisted for SeaMonkey ver X+ and Firefox/TB ver X+

Specifically not due to malicious code but merely incompat.  (imo that will be our best bet)
Flags: needinfo?(jorge)
Venkman can be flagged as incompatible. I just need to know exactly for which Firefox / SeaMonkey versions it will break.
Flags: needinfo?(jorge)
Depends on: 800200
I think you missed one file:
Error: c:\t1\hg\objdir-sm\suite\installer\package-manifest:145: Missing file(s): bin/components/jsdservice.xpt
(In reply to Philip Chee from comment #14)
> I think you missed one file:
> Error: c:\t1\hg\objdir-sm\suite\installer\package-manifest:145: Missing
> file(s): bin/components/jsdservice.xpt
Oops was looking at the wrong objdir. Sorry about that.
(In reply to Philip Chee from comment #15)
> (In reply to Philip Chee from comment #14)
> > I think you missed one file:
> > Error: c:\t1\hg\objdir-sm\suite\installer\package-manifest:145: Missing
> > file(s): bin/components/jsdservice.xpt
> Oops was looking at the wrong objdir. Sorry about that.

Is this supposed to be for this bug?  I filed bug 1031960.
(In reply to Philip Chee from comment #14)
> I think you missed one file:
> Error: c:\t1\hg\objdir-sm\suite\installer\package-manifest:145: Missing
> file(s): bin/components/jsdservice.xpt

As mentioned in comment #5, the patch removes the wrong file. But as per bug 1031960, that part of the patch isn't needed any more anyway.
Attached patch Updated patch (obsolete) — Splinter Review
Patch no longer applied due to some changes in package-manifest.in, fixed that and refreshed the patch (replaces Attachment 8445659 [details] [diff]).
> deleted file mode 100644
> --- a/suite/branding/nightly/icons/svg/venkman-window.svg
I still that the SVG file should not be removed since it still can be used as a reference for building new SVGs.
Comment on attachment 8445659 [details] [diff] [review]
Remove Venkman from suite/ (v1)

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

Overall a good patch! In addition to the review Neil did you need to fix a few other small things.

On the .svg files: I don't have a strong opinion on keep or delete. Weak keep I think.

::: suite/installer/removed-files.in
@@ +85,5 @@
>  chrome/icons/default/main-window16.xpm
>  chrome/icons/default/messengerWindow.xpm
>  chrome/icons/default/messengerWindow16.xpm
>  chrome/icons/default/msgcomposeWindow.xpm
>  chrome/icons/default/msgcomposeWindow16.xpm

We should add
#ifdef MOZ_OMNIJAR
distribution/extensions/{f13b157f-b174-47e7-a34d-4815ddfdfeb8}.xpi
#ifdef LOCALE_BUILD
distribution/extensions/langpack-@AB_CD@@venkman.mozilla.org.xpi
#endif
#endif

to removed-files.in. Those two files were shipped in the default build configuration (omnijar enabled, locale depends on build) and so should be removed again.

::: suite/installer/windows/nsis/installer.nsi
@@ +863,5 @@
>  Function leaveSummary
>    ${If} $InstallType != ${INSTALLTYPE_CUSTOM}
>      ; Set DOM Inspector, Venkman, ChatZilla to be installed
>      SectionSetFlags ${DOMI_IDX} 1
>      SectionSetFlags ${VENKMAN_IDX} 1

This line ("SectionSetFlags ${VENKMAN_IDX} 1") needs to be removed.
Attachment #8445659 - Flags: feedback?(bugzilla) → feedback+
Comment on attachment 8445659 [details] [diff] [review]
Remove Venkman from suite/ (v1)

r=me with mcsmurf's and these changes fixed:

(venkman-window.svg)
Need to delete all the icons, not just the svg version. (Ratty, they can be undeleted if necessary.)

(package-manifest.in)
>-@BINPATH@/components/jsdebugger.xpt
> @BINPATH@/components/jsdservice.xpt
This is wrong, and mcsmurf didn't fix it when he merged the jsdservice removal :-( jsdservice.xpt has been removed to fix our builders, so this can just go.

(removed-files.in)
>-chrome/icons/default/venkman-window.xpm
>-chrome/icons/default/venkman-window16.xpm
Ideally you would make two sets of packages, one without, one with your patch, and add the files that aren't in the new package to this list. I think mcsmurf found two of the three lines you need to add, but he missed venkman-window.ico (maybe because he didn't build on Windows). But removing entries from this list is wrong, because those are things that need to be removed when upgrading from older installations.
Attachment #8445659 - Flags: review?(neil) → review+
Well, yes, better don't remove those old files from removed-files.in. Probably by now most people should have upgraded to a build that does not require those lines (not sure what was the last version where Venkman was not bundled as extension), but better be safe here. So add to removed-files.in these lines:
#ifdef MOZ_OMNIJAR
distribution/extensions/{f13b157f-b174-47e7-a34d-4815ddfdfeb8}.xpi
#ifdef LOCALE_BUILD
distribution/extensions/langpack-@AB_CD@@venkman.mozilla.org.xpi
#endif
#endif
#ifdef MOZ_GTK2
chrome/icons/default/venkman-window.png
chrome/icons/default/venkman-window16.png
chrome/icons/default/venkman-window48.png
#elifdef XP_WIN32
chrome/icons/default/venkman-window.ico
#endif

and don't remove anything from that file.
Attached patch Remove venkman from Suite/ (v2) (obsolete) — Splinter Review
Requesting a new review because the removed-files.in and packages-files.in 
were changed.
Attachment #8445659 - Attachment is obsolete: true
Attachment #8453812 - Attachment is obsolete: true
Attachment #8455072 - Flags: review?(neil)
Comment on attachment 8455072 [details] [diff] [review]
Remove venkman from Suite/ (v2)

>-@BINPATH@/components/jsdservice.xpt
[This has already been removed from package-manifest.in on trunk (comm-central changeset ca6ee1b01aee) but does it need to be added to removed-files.in?]
Attachment #8455072 - Flags: review?(neil) → review+
Added components/jsdservice.xpt to removed-files.in.
Attachment #8455072 - Attachment is obsolete: true
Attachment #8455829 - Flags: review+
Status: ASSIGNED → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → seamonkey2.30
https://hg.mozilla.org/comm-central/rev/c81b39ef6c40 caused bustage in TB.

will push a fix.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Attached patch Bustage fix patch. (obsolete) — Splinter Review
Post-push review.
Attachment #8455848 - Flags: review?(standard8)
Status: REOPENED → ASSIGNED
Comment on attachment 8455848 [details] [diff] [review]
Bustage fix patch.

I'll r+ but ideally we'd have allowed --skip-venkman to do what it says, "skip" venkman which is no longer useable for us.
Attachment #8455848 - Flags: review?(standard8) → review+
Attachment #8455848 - Attachment is obsolete: true
Attachment #8455853 - Flags: review?(standard8)
Attachment #8455853 - Flags: review?(standard8) → review+
With that done, closing bug.
Status: ASSIGNED → RESOLVED
Closed: 5 years ago5 years ago
Resolution: --- → FIXED
Depends on: 1040225
Blocks: 1042038
Depends on: 1044999
You need to log in before you can comment on or make changes to this bug.