Land the IM front-end in comm-central

RESOLVED FIXED

Status

Instantbird
Other
RESOLVED FIXED
4 years ago
3 years ago

People

(Reporter: clokep, Assigned: clokep)

Tracking

Details

(URL)

Attachments

(16 attachments, 23 obsolete attachments)

11.84 KB, patch
florian
: review+
Details | Diff | Splinter Review
910 bytes, patch
florian
: review+
Details | Diff | Splinter Review
3.31 MB, patch
Details | Diff | Splinter Review
8.92 KB, patch
florian
: review+
Details | Diff | Splinter Review
4.60 KB, patch
florian
: review+
Details | Diff | Splinter Review
1.44 KB, patch
florian
: review+
Details | Diff | Splinter Review
1.66 KB, patch
florian
: review+
Details | Diff | Splinter Review
2.07 KB, patch
florian
: review+
Details | Diff | Splinter Review
1.38 KB, patch
florian
: review+
Details | Diff | Splinter Review
2.14 KB, patch
florian
: review+
Details | Diff | Splinter Review
3.70 KB, patch
florian
: review+
Details | Diff | Splinter Review
3.02 KB, patch
florian
: review+
Details | Diff | Splinter Review
1.50 KB, patch
florian
: review+
Details | Diff | Splinter Review
18.22 KB, patch
florian
: review+
Details | Diff | Splinter Review
1.26 KB, patch
florian
: review+
Details | Diff | Splinter Review
2.58 KB, patch
florian
: review+
Details | Diff | Splinter Review
(Assignee)

Description

4 years ago
It would simplify landing of chat/ changes for Thunderbird and Instantbird if these two products existed in the same repository. This would avoid the need for the "Port chat/ changes bugs" (bug 842183, bug 842183, bug 812921, bug 799068, bug 776901 and bug 753807) which are very prone to cause errors. They also have the awkwardness of having to port a bunch of UI changes to go with backend changes all at once instead of handling them individually in each bug.
(Assignee)

Comment 1

4 years ago
Created attachment 8355940 [details] [diff] [review]
1 - Copy instantbird/ to im/
Attachment #8355940 - Flags: review?(florian)
(Assignee)

Comment 2

4 years ago
Created attachment 8355941 [details] [diff] [review]
2 - Remove Makefiles from chat/
Attachment #8355941 - Flags: review?(florian)
(Assignee)

Comment 3

4 years ago
Created attachment 8355942 [details] [diff] [review]
3 - Revert moz.build files in chat/
Attachment #8355942 - Flags: review?(florian)
(Assignee)

Comment 4

4 years ago
Created attachment 8355943 [details] [diff] [review]
4 - Remove references to purple/
Attachment #8355943 - Flags: review?(florian)
(Assignee)

Comment 5

4 years ago
Created attachment 8355944 [details] [diff] [review]
5 - s/instantbird/im/
Attachment #8355944 - Flags: review?(florian)
(Assignee)

Comment 6

4 years ago
Created attachment 8355945 [details] [diff] [review]
6 - Port bug 870565
Attachment #8355945 - Flags: review?(florian)
(Assignee)

Comment 7

4 years ago
Created attachment 8355946 [details] [diff] [review]
7 - Fix JAR_MANIFESTS in moz.build files
Attachment #8355946 - Flags: review?(florian)
(Assignee)

Comment 8

4 years ago
Created attachment 8355947 [details] [diff] [review]
8 - Revert Makefile.in in chat/
Attachment #8355947 - Flags: review?(florian)
(Assignee)

Comment 9

4 years ago
Created attachment 8355948 [details] [diff] [review]
9 - s/core_abspath/$(abspath)/
Attachment #8355948 - Flags: review?(florian)
(Assignee)

Comment 10

4 years ago
Created attachment 8355949 [details] [diff] [review]
10 - Handle EXTRA_JS_MODULES in im/
Attachment #8355949 - Flags: review?(florian)
(Assignee)

Comment 11

4 years ago
Created attachment 8355950 [details] [diff] [review]
11 - Handle EXTRA_COMPONENTS in im/
Attachment #8355950 - Flags: review?(florian)
(Assignee)

Comment 12

4 years ago
Created attachment 8355951 [details] [diff] [review]
12 - Handle MODULE in im/
Attachment #8355951 - Flags: review?(florian)
(Assignee)

Comment 13

4 years ago
Created attachment 8355952 [details] [diff] [review]
13 - Handle XPIDLSRCS in im/
Attachment #8355952 - Flags: review?(florian)
(Assignee)

Comment 14

4 years ago
Created attachment 8355953 [details] [diff] [review]
14 - Handle all changes to mintrayr Makefile.in
Attachment #8355953 - Flags: review?(florian)
(Assignee)

Comment 15

4 years ago
Created attachment 8355954 [details] [diff] [review]
15 - Stop referencing preprocessor.py
Attachment #8355954 - Flags: review?(florian)
(Assignee)

Comment 16

4 years ago
Created attachment 8355955 [details] [diff] [review]
16 - Add Instantbird's 7zstub
Attachment #8355955 - Flags: review?(florian)
(Assignee)

Comment 17

4 years ago
Created attachment 8355956 [details] [diff] [review]
17 - Remove incomplete linkage stuff that Florian already asked me to remove a bunch of times
Attachment #8355956 - Flags: review?(florian)
(Assignee)

Comment 18

4 years ago
Created attachment 8355957 [details] [diff] [review]
18 - Port changes to app/Makefile.in
Attachment #8355957 - Flags: review?(florian)
(Assignee)

Comment 19

4 years ago
Created attachment 8355958 [details] [diff] [review]
19 - Fix package.manifest
Attachment #8355958 - Flags: review?(florian)
(Assignee)

Comment 20

4 years ago
Created attachment 8355959 [details] [diff] [review]
20 - Extra changes to get build/packaging to work on Mac OS X
Attachment #8355959 - Flags: review?(florian)
(Assignee)

Comment 21

4 years ago
Comment on attachment 8355940 [details] [diff] [review]
1 - Copy instantbird/ to im/

The patch copying instantbird/ to im/ doesn't actually need review as I believe Florian wishes to hg convert the current instantbird repo in such a way as to get some of the history of the Instantbird repository. This just servers as a baseline for the other patches in the series.
Attachment #8355940 - Flags: review?(florian)
Attachment #8355943 - Flags: review?(florian) → review+
Comment on attachment 8355944 [details] [diff] [review]
5 - s/instantbird/im/

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

::: im/confvars.sh
@@ +14,5 @@
>  MOZ_APP_VERSION=`cat $MOZ_APP_VERSION_TXT`
>  INSTANTBIRD_VERSION=$MOZ_APP_VERSION
>  
> +MOZ_BRANDING_DIRECTORY=im/branding/nightly
> +MOZ_OFFICIAL_BRANDING_DIRECTORY=im/branding/release

Is our official branding going to im/branding/release, or other-licenses/branding/instantbird/ ?
Comment on attachment 8355945 [details] [diff] [review]
6 - Port bug 870565

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

As a port of bug 870565 this is fine. In the future I wonder if we should move add_tier_dir('app', 'extensions') to after 'chat', so that we can build purple/ inside the extensions/ folder, after the chat idl files have already been processed.
Attachment #8355945 - Flags: review?(florian) → review+
Comment on attachment 8355946 [details] [diff] [review]
7 - Fix JAR_MANIFESTS in moz.build files

The changes here are fine. There are similar JAR_MANIFEST changes inside attachment 8355942 [details] [diff] [review], I wonder if these 2 attachments should be merged. On the other hand, the chat vs im separation also makes some sense, so r+ anyway.
Attachment #8355946 - Flags: review?(florian) → review+
Comment on attachment 8355948 [details] [diff] [review]
9 - s/core_abspath/$(abspath)/

Seems fine, do we know which Thunderbird changeset matches these changes?
Attachment #8355949 - Flags: review?(florian) → review+
Comment on attachment 8355950 [details] [diff] [review]
11 - Handle EXTRA_COMPONENTS in im/

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

::: im/components/moz.build
@@ +18,5 @@
> +else:
> +    EXTRA_COMPONENTS += ['profileMigrator.js', 'profileMigrator.manifest']
> +
> +EXTRA_PP_COMPONENTS += [
> +	'ibCommandLineHandler.js',

I think we tried to have this line close to 'ibCommandLineHandler.manifest' before. Any reason for changing the order of these definitions?


This file has mixed tabs and spaces for the indent. How are the moz.build files usually indented?
Attachment #8355950 - Flags: review?(florian) → review-
Attachment #8355951 - Flags: review?(florian) → review+
Comment on attachment 8355952 [details] [diff] [review]
13 - Handle XPIDLSRCS in im/

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

r+ but please be careful with the indentation here (I asked in a previous review comment to fix the indentation in this moz.build file.
Attachment #8355952 - Flags: review?(florian) → review+
Comment on attachment 8355953 [details] [diff] [review]
14 - Handle all changes to mintrayr Makefile.in

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

Seems reasonable. One nit.

::: im/components/mintrayr/moz.build
@@ +19,5 @@
> +
> +LIBRARY_NAME = 'trayToolkit'
> +
> +XPIDL_SOURCES += [
> +	'trayIToolkit.idl',

Mixed tab/spaces in this file too.
Attachment #8355953 - Flags: review?(florian) → review-
Comment on attachment 8355954 [details] [diff] [review]
15 - Stop referencing preprocessor.py

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

::: im/installer/windows/Makefile.in
@@ +60,3 @@
>  	$(PYTHON) $(MOZILLA_SRCDIR)/toolkit/mozapps/installer/windows/nsis/preprocess-locale.py \
>            --preprocess-locale $(MOZILLA_SRCDIR) \
> +	  $(call EXPAND_LOCALE_SRCDIR,im/locales)/installer $(AB_CD) $(CONFIG_DIR)

Wasn't this change intended to go in the s/instantbird/im/ patch?

@@ +70,3 @@
>  	$(PYTHON) $(MOZILLA_SRCDIR)/toolkit/mozapps/installer/windows/nsis/preprocess-locale.py \
>  	  --preprocess-locale $(MOZILLA_SRCDIR) \
> +	  $(call EXPAND_LOCALE_SRCDIR,im/locales)/installer $(AB_CD) $(CONFIG_DIR)

Same here.
Comment on attachment 8355955 [details] [diff] [review]
16 - Add Instantbird's 7zstub

Is this an exact copy of the file currently in the Instantbird repository?
Attachment #8355956 - Flags: review?(florian) → review+
Comment on attachment 8355957 [details] [diff] [review]
18 - Port changes to app/Makefile.in

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

::: im/app/Makefile.in
@@ -46,5 @@
> -
> -ifneq (,$(filter OS2 WINNT,$(OS_ARCH)))
> -PROGRAM = $(MOZ_APP_NAME)$(BIN_SUFFIX)
> -else
> -PROGRAM = $(MOZ_APP_NAME)-bin$(BIN_SUFFIX)

Thunderbird has a rule at http://hg.mozilla.org/comm-central/annotate/8198dbdebd96/mail/app/Makefile.in#l141 to replace this.

@@ -57,5 @@
>  ifneq (,$(filter OS2 WINNT,$(OS_ARCH)))
>  STUBNAME = $(MOZ_APP_NAME)$(BIN_SUFFIX)
>  else
>  STUBNAME = $(MOZ_APP_NAME)-bin$(BIN_SUFFIX)
>  endif

Why is this now done unconditionally? This is only for the case where a libxul sdk was provided at configure time, isn't it?

I think we need to look at all the stuff done in ifdef LIBXUL_SDK parts of the Thunderbird makefile.
Attachment #8355957 - Flags: review?(florian) → review-
Comment on attachment 8355959 [details] [diff] [review]
20 - Extra changes to get build/packaging to work on Mac OS X

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

::: im/app/Makefile.in
@@ +295,5 @@
>  ifdef MOZ_DEBUG
>  MAC_APP_NAME := $(MAC_APP_NAME)Debug
>  endif
>  
> +tools:: $(PROGRAM)

This should be its own changeset, with a clear reference to the bug we are porting.

::: im/installer/package-manifest.in
@@ +145,5 @@
>  #ifdef XP_MACOSX
>  @BINPATH@/components/ibDockBadge.js
>  @BINPATH@/components/ibDockBadge.manifest
>  #else
> +@BINPATH@/components/components.manifest

This should be part of "19 - Fix package.manifest"
Attachment #8355959 - Flags: review?(florian) → review-
Comment on attachment 8355958 [details] [diff] [review]
19 - Fix package.manifest

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

The title here isn't really correct (there's no package.manifest file).

See the review comment of "20 - Extra changes to get build/packaging to work on Mac OS X" for the reason for the r-.
Attachment #8355958 - Flags: review?(florian) → review-
Comment on attachment 8355945 [details] [diff] [review]
6 - Port bug 870565

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

::: im/app.mozbuild
@@ +4,5 @@
>  
>  app_libxul_dirs = []
>  app_libxul_static_dirs = []
>  
> +include('../bridge/bridge.mozbuild')

Joshua pointed out that we don't want this line.
Attachment #8355945 - Flags: review+ → review-
(Assignee)

Comment 35

4 years ago
Created attachment 8357541 [details] [diff] [review]
1 & 16 - Copy instantbird/ to im/ v2
Attachment #8355940 - Attachment is obsolete: true
Attachment #8355955 - Attachment is obsolete: true
Attachment #8355955 - Flags: review?(florian)
(Assignee)

Comment 36

4 years ago
Created attachment 8357542 [details] [diff] [review]
2 & 3 & 8 - Remove/revert Makefile.in and revert moz.build in chat/ v2
Attachment #8355942 - Attachment is obsolete: true
Attachment #8355947 - Attachment is obsolete: true
Attachment #8355941 - Attachment is obsolete: true
Attachment #8355942 - Flags: review?(florian)
Attachment #8355947 - Flags: review?(florian)
Attachment #8357542 - Flags: review?(florian)
Attachment #8355941 - Flags: review?(florian)
(Assignee)

Comment 37

4 years ago
Created attachment 8357544 [details] [diff] [review]
5 - s/instantbird/im/ v2
Attachment #8355944 - Attachment is obsolete: true
Attachment #8355944 - Flags: review?(florian)
Attachment #8357544 - Flags: review?(florian)
(Assignee)

Updated

4 years ago
Attachment #8357542 - Attachment description: 2 & 3 & 8 - Remove/revert Makefile.in and revert moz.build in chat/ → 2 & 3 & 8 - Remove/revert Makefile.in and revert moz.build in chat/ v2
(Assignee)

Comment 38

4 years ago
Created attachment 8357547 [details] [diff] [review]
6 - Port bug 870565 v2
Attachment #8355945 - Attachment is obsolete: true
Attachment #8357547 - Flags: review?(florian)
(Assignee)

Comment 39

4 years ago
Created attachment 8357549 [details] [diff] [review]
7 - Fix JAR_MANIFESTS in moz.build files v2
Attachment #8357549 - Flags: review?(florian)
(Assignee)

Comment 40

4 years ago
Created attachment 8357550 [details] [diff] [review]
9 - s/core_abspath/$(abspath)/ v2
Attachment #8355946 - Attachment is obsolete: true
Attachment #8355948 - Attachment is obsolete: true
Attachment #8355948 - Flags: review?(florian)
Attachment #8357550 - Flags: review?(florian)
(Assignee)

Comment 41

4 years ago
Created attachment 8357551 [details] [diff] [review]
10 - Handle EXTRA_JS_MODULES in im/ v2
Attachment #8355949 - Attachment is obsolete: true
Attachment #8357551 - Flags: review?(florian)
(Assignee)

Comment 42

4 years ago
Created attachment 8357552 [details] [diff] [review]
11 - Handle EXTRA_COMPONENTS in im/ v2
Attachment #8357552 - Flags: review?(florian)
(Assignee)

Comment 43

4 years ago
Created attachment 8357553 [details] [diff] [review]
12 & 13 - Handle MODULE and XPIDLSRCS in im/ v2
Attachment #8355950 - Attachment is obsolete: true
Attachment #8355951 - Attachment is obsolete: true
Attachment #8355952 - Attachment is obsolete: true
Attachment #8357553 - Flags: review?(florian)
(Assignee)

Comment 44

4 years ago
Created attachment 8357554 [details] [diff] [review]
14 - Handle all changes to mintrayr Makefile.in v2
Attachment #8355953 - Attachment is obsolete: true
Attachment #8357554 - Flags: review?(florian)
(Assignee)

Comment 45

4 years ago
Created attachment 8357555 [details] [diff] [review]
15 - Stop referencing preprocessor.py v2
Attachment #8355954 - Attachment is obsolete: true
Attachment #8355954 - Flags: review?(florian)
Attachment #8357555 - Flags: review?(florian)
(Assignee)

Comment 46

4 years ago
Created attachment 8357556 [details] [diff] [review]
18 - Port changes to app/Makefile.in v2
Attachment #8355957 - Attachment is obsolete: true
Attachment #8357556 - Flags: review?(florian)
(Assignee)

Comment 47

4 years ago
Created attachment 8357557 [details] [diff] [review]
19 - Fix package-manifest.in v2
Attachment #8355958 - Attachment is obsolete: true
Attachment #8357557 - Flags: review?(florian)
(Assignee)

Comment 48

4 years ago
Created attachment 8357558 [details] [diff] [review]
20 - Mac OS changes v2
Attachment #8355959 - Attachment is obsolete: true
Attachment #8357558 - Flags: review?(florian)
(Assignee)

Comment 49

4 years ago
Response to your review comments. I probably should have split these up by patch. Sorry for doing it in a stupid way.

(In reply to Florian Quèze [:florian] [:flo] from comment #22)
> Comment on attachment 8355944 [details] [diff] [review]
> 5 - s/instantbird/im/
>
> Review of attachment 8355944 [details] [diff] [review]:
> -----------------------------------------------------------------
>
> ::: im/confvars.sh
> @@ +14,5 @@
> >  MOZ_APP_VERSION=`cat $MOZ_APP_VERSION_TXT`
> >  INSTANTBIRD_VERSION=$MOZ_APP_VERSION
> >
> > +MOZ_BRANDING_DIRECTORY=im/branding/nightly
> > +MOZ_OFFICIAL_BRANDING_DIRECTORY=im/branding/release
>
> Is our official branding going to im/branding/release, or
> other-licenses/branding/instantbird/ ?

I think we'll move this to other-licenses/branding/instantbird/ as part of the hg convert, so I'll do this in the first commit. I've updated this path to reflect this move.

(In reply to Florian Quèze [:florian] [:flo] from comment #23)
> Comment on attachment 8355945 [details] [diff] [review]
> 6 - Port bug 870565
>
> Review of attachment 8355945 [details] [diff] [review]:
> -----------------------------------------------------------------
>
> As a port of bug 870565 this is fine. In the future I wonder if we should
> move add_tier_dir('app', 'extensions') to after 'chat', so that we can build
> purple/ inside the extensions/ folder, after the chat idl files have already
> been processed.
I've moved this. I did not include any

(In reply to Florian Quèze [:florian] [:flo] from comment #34)
> Comment on attachment 8355945 [details] [diff] [review]
> 6 - Port bug 870565
>
> Review of attachment 8355945 [details] [diff] [review]:
> -----------------------------------------------------------------
>
> ::: im/app.mozbuild
> @@ +4,5 @@
> >
> >  app_libxul_dirs = []
> >  app_libxul_static_dirs = []
> >
> > +include('../bridge/bridge.mozbuild')
>
> Joshua pointed out that we don't want this line.
I've removed this.

(In reply to Florian Quèze [:florian] [:flo] from comment #24)
> Comment on attachment 8355946 [details] [diff] [review]
> 7 - Fix JAR_MANIFESTS in moz.build files
>
> The changes here are fine. There are similar JAR_MANIFEST changes inside
> attachment 8355942 [details] [diff] [review], I wonder if these 2
> attachments should be merged. On the other hand, the chat vs im separation
> also makes some sense, so r+ anyway.

The ones in attachment 8355942 [details] [diff] [review] are automated hg rm / hg revert, the ones here were made by hand. They should be kept separate, I think.

(In reply to Florian Quèze [:florian] [:flo] from comment #25)
> Comment on attachment 8355948 [details] [diff] [review]
> 9 - s/core_abspath/$(abspath)/
>
> Seems fine, do we know which Thunderbird changeset matches these changes?

Bug 928966, I've added that to the current commit message.

(In reply to Florian Quèze [:florian] [:flo] from comment #26)
> Comment on attachment 8355950 [details] [diff] [review]
> 11 - Handle EXTRA_COMPONENTS in im/
>
> Review of attachment 8355950 [details] [diff] [review]:
> -----------------------------------------------------------------
>
> ::: im/components/moz.build
> @@ +18,5 @@
> > +else:
> > +    EXTRA_COMPONENTS += ['profileMigrator.js', 'profileMigrator.manifest']
> > +
> > +EXTRA_PP_COMPONENTS += [
> > +	'ibCommandLineHandler.js',
>
> I think we tried to have this line close to 'ibCommandLineHandler.manifest'
> before. Any reason for changing the order of these definitions?

No real reason, I've moved it back.

> This file has mixed tabs and spaces for the indent. How are the moz.build
> files usually indented?

Spaces, I've checked the files and they SHOULD have all spaces now. (As also mentioned in comment #27 and comment #28.)

(In reply to Florian Quèze [:florian] [:flo] from comment #29)
> Comment on attachment 8355954 [details] [diff] [review]
> 15 - Stop referencing preprocessor.py
>
> Review of attachment 8355954 [details] [diff] [review]:
> -----------------------------------------------------------------
>
> ::: im/installer/windows/Makefile.in
> @@ +60,3 @@
> >  	$(PYTHON) $(MOZILLA_SRCDIR)/toolkit/mozapps/installer/windows/nsis/preprocess-locale.py \
> >            --preprocess-locale $(MOZILLA_SRCDIR) \
> > +	  $(call EXPAND_LOCALE_SRCDIR,im/locales)/installer $(AB_CD) $(CONFIG_DIR)
>
> Wasn't this change intended to go in the s/instantbird/im/ patch?

Yes (for both of them), I've moved them.

(In reply to Florian Quèze [:florian] [:flo] from comment #30)
> Comment on attachment 8355955 [details] [diff] [review]
> 16 - Add Instantbird's 7zstub
>
> Is this an exact copy of the file currently in the Instantbird repository?

I've moved this into the first patch. I'm assuming this will be done as part of the convert.

(In reply to Florian Quèze [:florian] [:flo] from comment #31)
> Comment on attachment 8355957 [details] [diff] [review]
> 18 - Port changes to app/Makefile.in
>
> Review of attachment 8355957 [details] [diff] [review]:
> -----------------------------------------------------------------
>
> ::: im/app/Makefile.in
> @@ -46,5 @@
> > -
> > -ifneq (,$(filter OS2 WINNT,$(OS_ARCH)))
> > -PROGRAM = $(MOZ_APP_NAME)$(BIN_SUFFIX)
> > -else
> > -PROGRAM = $(MOZ_APP_NAME)-bin$(BIN_SUFFIX)
>
> Thunderbird has a rule at
> http://hg.mozilla.org/comm-central/annotate/8198dbdebd96/mail/app/Makefile.
> in#l141 to replace this.

OK, I added this back in.

> @@ -57,5 @@
> >  ifneq (,$(filter OS2 WINNT,$(OS_ARCH)))
> >  STUBNAME = $(MOZ_APP_NAME)$(BIN_SUFFIX)
> >  else
> >  STUBNAME = $(MOZ_APP_NAME)-bin$(BIN_SUFFIX)
> >  endif
>
> Why is this now done unconditionally? This is only for the case where a
> libxul sdk was provided at configure time, isn't it?
>
> I think we need to look at all the stuff done in ifdef LIBXUL_SDK parts of
> the Thunderbird makefile.

This file is extremely different than mail/app/Makefile.in. I might need a bit more of help looking at this file.

(In reply to Florian Quèze [:florian] [:flo] from comment #32)
> Comment on attachment 8355959 [details] [diff] [review]
> 20 - Extra changes to get build/packaging to work on Mac OS X
>
> Review of attachment 8355959 [details] [diff] [review]:
> -----------------------------------------------------------------
>
> ::: im/app/Makefile.in
> @@ +295,5 @@
> >  ifdef MOZ_DEBUG
> >  MAC_APP_NAME := $(MAC_APP_NAME)Debug
> >  endif
> >
> > +tools:: $(PROGRAM)
>
> This should be its own changeset, with a clear reference to the bug we are
> porting.

Done (bug 912292, for the record).

> ::: im/installer/package-manifest.in
> @@ +145,5 @@
> >  #ifdef XP_MACOSX
> >  @BINPATH@/components/ibDockBadge.js
> >  @BINPATH@/components/ibDockBadge.manifest
> >  #else
> > +@BINPATH@/components/components.manifest
>
> This should be part of "19 - Fix package.manifest"

Done.

(In reply to Florian Quèze [:florian] [:flo] from comment #33)
> Comment on attachment 8355958 [details] [diff] [review]
> 19 - Fix package.manifest
>
> Review of attachment 8355958 [details] [diff] [review]:
> -----------------------------------------------------------------
>
> The title here isn't really correct (there's no package.manifest file).

I'll fix this for the next uploaded patch.
(Assignee)

Updated

4 years ago
Comment on attachment 8357542 [details] [diff] [review]
2 & 3 & 8 - Remove/revert Makefile.in and revert moz.build in chat/ v2

This will need to be updated once bug 957918 lands. There's no way we can add "protocols/vkontakte" again for Instantbird ;).
Attachment #8357542 - Flags: review?(florian) → review-
Comment on attachment 8357547 [details] [diff] [review]
6 - Port bug 870565 v2

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

::: im/app.mozbuild
@@ +12,3 @@
>  
> +add_tier_dir('app', '../chat')
> +add_tier_dir('app', '../im')

im/app needs to be the last folder we build, so if we are hoping that purple/ will be built by the extensions folder, 'im' needs to be after 'extensions'.
Attachment #8357547 - Flags: review?(florian) → review-
Attachment #8357544 - Flags: review?(florian) → review+
Attachment #8357549 - Flags: review?(florian) → review+
Attachment #8357550 - Flags: review?(florian) → review+
Attachment #8357551 - Flags: review?(florian) → review+
Attachment #8357552 - Flags: review?(florian) → review+
Attachment #8357553 - Flags: review?(florian) → review+
Attachment #8357554 - Flags: review?(florian) → review+
Attachment #8357555 - Flags: review?(florian) → review+
Attachment #8357557 - Flags: review?(florian) → review+
Attachment #8357558 - Flags: review?(florian) → review+
Comment on attachment 8357556 [details] [diff] [review]
18 - Port changes to app/Makefile.in v2

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

Seems good otherwise, but I think I would be more confident if we had verified that a build works on all OSes with this change applied.

::: im/app/Makefile.in
@@ +128,5 @@
> +ifneq (,$(filter-out OS2 WINNT,$(OS_ARCH)))
> +libs::
> +	cp -p $(MOZ_APP_NAME)$(BIN_SUFFIX) $(DIST)/bin/$(MOZ_APP_NAME)-bin$(BIN_SUFFIX)
> +
> +GARBAGE += $(addprefix $(DIST)/bin/defaults/pref/, all.js all-instantbird.js channel-prefs.js chat.js)

There's no chat.js file, I think you wanted chat-prefs.js.
Attachment #8357556 - Flags: review?(florian) → review-
(Assignee)

Comment 53

4 years ago
Created attachment 8358181 [details] [diff] [review]
2 & 3 & 8 - Remove/revert Makefile.in and revert moz.build in chat/ v3

(In reply to Florian Quèze [:florian] [:flo] from comment #50)
> Comment on attachment 8357542 [details] [diff] [review]
> 2 & 3 & 8 - Remove/revert Makefile.in and revert moz.build in chat/ v2
> 
> This will need to be updated once bug 957918 lands. There's no way we can
> add "protocols/vkontakte" again for Instantbird ;).

Done.
Attachment #8357542 - Attachment is obsolete: true
Attachment #8358181 - Flags: review?(florian)
(Assignee)

Comment 54

4 years ago
Created attachment 8358182 [details] [diff] [review]
6 - Port bug 870565 v3

(In reply to Florian Quèze [:florian] [:flo] from comment #51)
> Comment on attachment 8357547 [details] [diff] [review]
> 6 - Port bug 870565 v2
> 
> Review of attachment 8357547 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: im/app.mozbuild
> @@ +12,3 @@
> >  
> > +add_tier_dir('app', '../chat')
> > +add_tier_dir('app', '../im')
> 
> im/app needs to be the last folder we build, so if we are hoping that
> purple/ will be built by the extensions folder, 'im' needs to be after
> 'extensions'.

I added the extensions part between chat/ and im/.
Attachment #8357547 - Attachment is obsolete: true
Attachment #8358182 - Flags: review?(florian)
(Assignee)

Comment 55

4 years ago
Created attachment 8358183 [details] [diff] [review]
18 - Port changes to app/Makefile.in v3

(In reply to Florian Quèze [:florian] [:flo] from comment #52)
> Comment on attachment 8357556 [details] [diff] [review]
> 18 - Port changes to app/Makefile.in v2
> 
> Review of attachment 8357556 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Seems good otherwise, but I think I would be more confident if we had
> verified that a build works on all OSes with this change applied.
> 
> ::: im/app/Makefile.in
> @@ +128,5 @@
> > +ifneq (,$(filter-out OS2 WINNT,$(OS_ARCH)))
> > +libs::
> > +	cp -p $(MOZ_APP_NAME)$(BIN_SUFFIX) $(DIST)/bin/$(MOZ_APP_NAME)-bin$(BIN_SUFFIX)
> > +
> > +GARBAGE += $(addprefix $(DIST)/bin/defaults/pref/, all.js all-instantbird.js channel-prefs.js chat.js)
> 
> There's no chat.js file, I think you wanted chat-prefs.js.

I changed this. I'll see if I can get a try build working?
Attachment #8358183 - Flags: review?(florian)
Attachment #8358182 - Flags: review?(florian) → review+
Attachment #8358183 - Flags: review?(florian) → review+
Attachment #8358181 - Flags: review?(florian) → review+
(Assignee)

Comment 56

4 years ago
Created attachment 8358433 [details] [diff] [review]
2 & 3 & - Remove/revert Makefile.in and revert moz.build in chat/ v4

I did something really funky in my last patch and I have no idea what happened, but it's definitely wrong. This patch essentially becomes a no-op after the latest ib->c-c merge, besides removing the Makefile.in files.
Attachment #8358181 - Attachment is obsolete: true
Attachment #8358433 - Flags: review?(florian)
(Assignee)

Comment 57

4 years ago
Comment on attachment 8358181 [details] [diff] [review]
2 & 3 & 8 - Remove/revert Makefile.in and revert moz.build in chat/ v3

This is the proper patch, turns out I was diffing against a repo of IB with patches applied to it.
Attachment #8358181 - Attachment is obsolete: false
(Assignee)

Updated

4 years ago
Attachment #8358433 - Attachment is obsolete: true
Attachment #8358433 - Flags: review?(florian)
Comment on attachment 8358183 [details] [diff] [review]
18 - Port changes to app/Makefile.in v3

(In reply to Florian Quèze [:florian] [:flo] from comment #52)
> Comment on attachment 8357556 [details] [diff] [review]
> 18 - Port changes to app/Makefile.in v2

> Seems good otherwise, but I think I would be more confident if we had
> verified that a build works on all OSes with this change applied.

Actually, it doesn't work on Mac.

> ::: im/app/Makefile.in
> @@ +128,5 @@
> > +ifneq (,$(filter-out OS2 WINNT,$(OS_ARCH)))
> > +libs::
> > +	cp -p $(MOZ_APP_NAME)$(BIN_SUFFIX) $(DIST)/bin/$(MOZ_APP_NAME)-bin$(BIN_SUFFIX)

This rule needs to be after the inclusion of rules.mk
Attachment #8358183 - Flags: review+ → review-
Created attachment 8358861 [details] [diff] [review]
18 - Port changes to app/Makefile.in v4

New patch addressing comment 58. (r+'ing as it's clokep's patch with my review comment addressed)
Attachment #8357556 - Attachment is obsolete: true
Attachment #8358183 - Attachment is obsolete: true
Attachment #8358861 - Flags: review+
https://hg.mozilla.org/comm-central/pushloghtml?changeset=f81a23bfefcd
Status: ASSIGNED → RESOLVED
Last Resolved: 4 years ago
Resolution: --- → FIXED
(Assignee)

Updated

4 years ago
Depends on: 955009
Fixed path to the Halloween branding: https://hg.mozilla.org/comm-central/rev/d59eb3bf90e9
You need to log in before you can comment on or make changes to this bug.