Closed Bug 956609 Opened 6 years ago Closed 6 years ago

Land the IM front-end in comm-central

Categories

(Instantbird :: Other, defect)

defect
Not set

Tracking

(Not tracked)

RESOLVED FIXED

People

(Reporter: clokep, Assigned: clokep)

References

()

Details

Attachments

(16 files, 23 obsolete files)

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
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.
Attached patch 1 - Copy instantbird/ to im/ (obsolete) — Splinter Review
Attachment #8355940 - Flags: review?(florian)
Attached patch 2 - Remove Makefiles from chat/ (obsolete) — Splinter Review
Attachment #8355941 - Flags: review?(florian)
Attachment #8355942 - Flags: review?(florian)
Attachment #8355943 - Flags: review?(florian)
Attached patch 5 - s/instantbird/im/ (obsolete) — Splinter Review
Attachment #8355944 - Flags: review?(florian)
Attached patch 6 - Port bug 870565 (obsolete) — Splinter Review
Attachment #8355945 - Flags: review?(florian)
Attachment #8355946 - Flags: review?(florian)
Attached patch 8 - Revert Makefile.in in chat/ (obsolete) — Splinter Review
Attachment #8355947 - Flags: review?(florian)
Attached patch 9 - s/core_abspath/$(abspath)/ (obsolete) — Splinter Review
Attachment #8355948 - Flags: review?(florian)
Attachment #8355949 - Flags: review?(florian)
Attachment #8355950 - Flags: review?(florian)
Attached patch 12 - Handle MODULE in im/ (obsolete) — Splinter Review
Attachment #8355951 - Flags: review?(florian)
Attached patch 13 - Handle XPIDLSRCS in im/ (obsolete) — Splinter Review
Attachment #8355952 - Flags: review?(florian)
Attachment #8355953 - Flags: review?(florian)
Attachment #8355954 - Flags: review?(florian)
Attached patch 16 - Add Instantbird's 7zstub (obsolete) — Splinter Review
Attachment #8355955 - Flags: review?(florian)
Attachment #8355957 - Flags: review?(florian)
Attached patch 19 - Fix package.manifest (obsolete) — Splinter Review
Attachment #8355958 - Flags: review?(florian)
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-
Attachment #8355940 - Attachment is obsolete: true
Attachment #8355955 - Attachment is obsolete: true
Attachment #8355955 - Flags: review?(florian)
Attachment #8355941 - Attachment is obsolete: true
Attachment #8355942 - Attachment is obsolete: true
Attachment #8355947 - Attachment is obsolete: true
Attachment #8355941 - Flags: review?(florian)
Attachment #8355942 - Flags: review?(florian)
Attachment #8355947 - Flags: review?(florian)
Attachment #8357542 - Flags: review?(florian)
Attachment #8355944 - Attachment is obsolete: true
Attachment #8355944 - Flags: review?(florian)
Attachment #8357544 - Flags: review?(florian)
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
Attached patch 6 - Port bug 870565 v2 (obsolete) — Splinter Review
Attachment #8355945 - Attachment is obsolete: true
Attachment #8357547 - Flags: review?(florian)
Attachment #8355946 - Attachment is obsolete: true
Attachment #8355948 - Attachment is obsolete: true
Attachment #8355948 - Flags: review?(florian)
Attachment #8357550 - Flags: review?(florian)
Attachment #8355949 - Attachment is obsolete: true
Attachment #8357551 - Flags: review?(florian)
Attachment #8357552 - Flags: review?(florian)
Attachment #8355950 - Attachment is obsolete: true
Attachment #8355951 - Attachment is obsolete: true
Attachment #8355952 - Attachment is obsolete: true
Attachment #8357553 - Flags: review?(florian)
Attachment #8355953 - Attachment is obsolete: true
Attachment #8357554 - Flags: review?(florian)
Attachment #8355954 - Attachment is obsolete: true
Attachment #8355954 - Flags: review?(florian)
Attachment #8357555 - Flags: review?(florian)
Attachment #8355957 - Attachment is obsolete: true
Attachment #8357556 - Flags: review?(florian)
Attachment #8355958 - Attachment is obsolete: true
Attachment #8357557 - Flags: review?(florian)
Attachment #8355959 - Attachment is obsolete: true
Attachment #8357558 - Flags: review?(florian)
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.
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-
(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)
(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)
(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+
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)
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
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-
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
Closed: 6 years ago
Resolution: --- → FIXED
Depends on: 955009
You need to log in before you can comment on or make changes to this bug.