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

Status: RESOLVED FIXED

(SeaMonkey :: Build Config, defect)

Comment 1

[Philip Chee]

http://mxr.mozilla.org/comm-central/search?string=venkman&find=/suite/

Comment 2

[Edmund Wong (:ewong)]

Attachment: Remove Venkman from suite/ (v1)

Comment 3

[Edmund Wong (:ewong)]

Attachment: Remove Venkman from suite/ (help patch) (v1)

Comment 4

[Jens Hatlak (:InvisibleSmiley)]

Comment on attachment 8445660 [details] [diff] [review]
Remove Venkman from suite/ (help patch) (v1)

Sad to see but looks OK.

Comment 5

[[e-mail]]

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.

Comment 6

[Philip Chee]

>>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.

Comment 7

[Frank Wein [:mcsmurf]]

Q: How will we make sure the Venkman addon gets disabled on newer SeaMonkey builds? Can we do this via addons.m.o?

Comment 8

[Robert Kaiser]

(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.

Comment 9

[patrickjdempsey]

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.

Comment 10

[patrickjdempsey]

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.

Comment 11

[Justin Wood (:Callek)]

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)

Comment 12

[Jorge Villalobos [:jorgev] (he/him)]

Venkman can be flagged as incompatible. I just need to know exactly for which Firefox / SeaMonkey versions it will break.

Comment 13

[rsx11m]

Patches for bug 800200 have been pushed to mozilla-inbound.
https://hg.mozilla.org/integration/mozilla-inbound/pushloghtml?changeset=d75a551eb075

Comment 14

[Philip Chee]

I think you missed one file:
Error: c:\t1\hg\objdir-sm\suite\installer\package-manifest:145: Missing file(s): bin/components/jsdservice.xpt

Comment 15

[Philip Chee]

(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.

Comment 16

[Edmund Wong (:ewong)]

(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.

Comment 17

[[e-mail]]

(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.

Comment 18

[Frank Wein [:mcsmurf]]

Attachment: Updated patch

Patch no longer applied due to some changes in package-manifest.in, fixed that and refreshed the patch (replaces Attachment 8445659 [details] [diff]).

Comment 19

[Philip Chee]

> 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 20

[Frank Wein [:mcsmurf]]

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.

Comment 21

[[e-mail]]

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.

Comment 22

[Frank Wein [:mcsmurf]]

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.

Comment 23

[Edmund Wong (:ewong)]

Attachment: Remove venkman from Suite/ (v2)

Requesting a new review because the removed-files.in and packages-files.in 
were changed.

Comment 24

[[e-mail]]

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?]

Comment 25

[Edmund Wong (:ewong)]

Attachment: Remove venkman from Suite/ (v3)

Added components/jsdservice.xpt to removed-files.in.

Comment 26

[Edmund Wong (:ewong)]

Pushed to comm-central:
  https://hg.mozilla.org/comm-central/rev/491e0cb86346  (help)
  https://hg.mozilla.org/comm-central/rev/c81b39ef6c40

Comment 27

[Edmund Wong (:ewong)]

https://hg.mozilla.org/comm-central/rev/c81b39ef6c40 caused bustage in TB.

will push a fix.

Comment 28

[Edmund Wong (:ewong)]

Attachment: Bustage fix patch.

Post-push review.

Comment 29

[Justin Wood (:Callek)]

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.

Comment 30

[Edmund Wong (:ewong)]

Attachment: Bustage fix patch. (v2)

Comment 31

[Edmund Wong (:ewong)]

Pushed to comm-central:
https://hg.mozilla.org/comm-central/rev/39029ec3c358

Comment 32

[Edmund Wong (:ewong)]

With that done, closing bug.