1582937 - Remove header dependencies between mozglue/dllservices and browser/app/winlauncher

Status: RESOLVED FIXED

(Core :: mozglue, defect, P1)

Description

[Jorg K (CEST = GMT+2)]

Pretty busted right now, and this
https://hg.mozilla.org/mozilla-central/rev/8e2da9ff5f5aace0988f8d358aafdba04cddf950#l1.12
is just the beginning.

The main issue is:
0:08.66 c:/mozilla-source/comm-central/mozglue/dllservices/LoaderObserver.h(11,10): fatal error: 'mozilla/NtLoaderAPI.h' file not found
0:08.66 #include "mozilla/NtLoaderAPI.h"

Comment 1

[Jorg K (CEST = GMT+2)]

And also:
0:09.45 c:/mozilla-source/comm-central/obj-x86_64-pc-mingw32/dist/include\mozilla/glue/WindowsDllServices.h(12,10): fatal error: 'mozilla/LoaderAPIInterfaces.h' file not found
0:09.45 #include "mozilla/LoaderAPIInterfaces.h"

Comment 2

[Jorg K (CEST = GMT+2)]

Attachment: 1582937-C-C.patch

Comment 3

[Jorg K (CEST = GMT+2)]

Attachment: 1582937-M-C.patch

If I hard-code some some paths, I can get it to compile (so far). So something has gone wrong with some Makefile or build magic :-(

Comment 4

[Jorg K (CEST = GMT+2)]

I wonder whether this affects Windows only. I'll push the C-C patch and retrigger the Dailies.

Comment 5

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/comm-central/rev/1eb948124d13
Port bug 1542830: adjustments to mail/app/nsMailApp.cpp. rs=bustage-fix DONTBUILD

Comment 6

[Jorg K (CEST = GMT+2)]

Hmm, the M-C patch also doesn't let me compile, after a while I get:

0:13.19 In file included from c:/mozilla-source/comm-central/toolkit/crashreporter/minidump-analyzer/minidump-analyzer.cpp:30:
0:13.19 c:/mozilla-source/comm-central/obj-x86_64-pc-mingw32/dist/include\mozilla/glue/WindowsDllServices.h(12,10): fatal error: '../../browser/app/winlauncher/freestanding/LoaderAPIInterfaces.h' file not found
0:13.19 #include "../../browser/app/winlauncher/freestanding/LoaderAPIInterfaces.h"

Strangely the automation run is into 19 minutes and hasn't failed. So is it just me?

Comment 7

[Jorg K (CEST = GMT+2)]

Comment on attachment 9094383 [details] [diff] [review]
1582937-C-C.patch

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

::: mail/app/nsMailApp.cpp
@@ +24,5 @@
>  #include "nsIFile.h"
>  
>  #ifdef XP_WIN
> +#  include "LauncherProcessWin.h"
> +#  include "mozilla/WindowsDllBlocklist.h"

BTW, # include "LauncherProcessWin.h" was added to M-C here:
https://hg.mozilla.org/mozilla-central/rev/eb5c30217df9#l5.12
Looks like we didn't port it back then, so I've added it now to align with M-C again.

Comment 8

[Jorg K (CEST = GMT+2)]

Comment on attachment 9094383 [details] [diff] [review]
1582937-C-C.patch

This should be good for review.

Comment 9

[Jorg K (CEST = GMT+2)]

OK, a clobber build got me further, but like in automation it fails at the end:
https://treeherder.mozilla.org/#/jobs?repo=comm-central&revision=1eb948124d131b9e064c5ea44750a03a666544c6&selectedJob=267794793
z:/task_1569052759/build/src/obj-thunderbird/dist/include\mozilla/glue/WindowsDllServices.h(12,10): fatal error: 'mozilla/LoaderAPIInterfaces.h' file not found

https://treeherder.mozilla.org/logviewer.html#/jobs?job_id=267794793&repo=comm-central&lineNumber=14762

Comment 10

[Richard Marti (:Paenglab)]

Comment on attachment 9094383 [details] [diff] [review]
1582937-C-C.patch

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

::: mail/app/nsMailApp.cpp
@@ +23,5 @@
>  #include "nsCOMPtr.h"
>  #include "nsIFile.h"
>  
>  #ifdef XP_WIN
> +#  include "LauncherProcessWin.h"

Doesn't this need also the other files added by bug 1454745 and the change in moz.build?

Comment 11

[Jorg K (CEST = GMT+2)]

Hmm, you mean browser/app/LauncherProcessWin.cpp/h and

+    SOURCES += [
+        'LauncherProcessWin.cpp',
+    ]

Good question. If we don't port that, I guess adding the include file also doesn't make sense.

But that's just a secondary issue. Right now, we need to fix the bustage. BTW, to get going again locally, I did:
hg qbackout -s -r da7e775c4051fa5803873a409da0ffaf5cb0a864:6fcb417f7ff4048f0237e4949639d305198e20ad
to backout bug 1542830 so I can work again. Sadly, there is more "weekend bustage" I need to take care of.

Comment 12

[Jorg K (CEST = GMT+2)]

Looking at this again, the basic issue is that source code in mozglue/dllservices relies on code in browser/app/winlauncher. That won't work for TB. I tried copying the entire winlauncher directory to mail/app, but had no success compiling it so far.

Comment 13

[Jorg K (CEST = GMT+2)]

Attachment: patch-to-copy-winlauncher.patch

OK, some tweaks to moz.build + a copy of browser's winlauncher. It doesn't compile.

Comment 14

[Jorg K (CEST = GMT+2)]

Comment on attachment 9094383 [details] [diff] [review]
1582937-C-C.patch

Yes, I'll remove the include again since we don't have app\winlauncher\LauncherProcessWin.h so far.

Comment 15

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/comm-central/rev/2bd686fea9ae
Remove include of app/winlauncher/LauncherProcessWin.h again which Thunderbird doesn't have. r=me

Comment 16

[Rob Lemley [:rjl]]

Attachment: llvm-dlltool-commmail.patch

mach configure completed and the build is progressing.

Comment 17

[Jorg K (CEST = GMT+2)]

Attachment: patch-to-copy-winlauncher.patch

OK, this should work together with Rob's llvm-dlltool-commmail.patch for M-C.

As the commit message says: Copy browser/app/winlauncher to mail/app/winlauncher.

Comment 18

[Jorg K (CEST = GMT+2)]

So, with both patches together, TB compiles and runs. Time to ask for reviews.

Comment 19

[Jorg K (CEST = GMT+2)]

Comment on attachment 9094410 [details] [diff] [review]
llvm-dlltool-commmail.patch

Whoever comes first. This is pretty urgent.

Comment 20

[Jorg K (CEST = GMT+2)]

Comment on attachment 9094411 [details] [diff] [review]
patch-to-copy-winlauncher.patch

Straight copy with some build stuff also copied. Can we improve the
```
+    LOCAL_INCLUDES += [
+        '/comm/mail/app/winlauncher',
+    ]
```

Oh yes, there are some Firefox strings I should change to Thunderbird.

Comment 21

[(No longer employed by Mozilla) Aaron Klotz]

Comment on attachment 9094410 [details] [diff] [review]
llvm-dlltool-commmail.patch

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

You shouldn't need any of this stuff for Thunderbird. Let's sort out the parts that you actually need to build instead of pulling in the world.

Comment 22

[(No longer employed by Mozilla) Aaron Klotz]

Comment on attachment 9094411 [details] [diff] [review]
patch-to-copy-winlauncher.patch

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

You should not need to pull in the world from the launcher process to get Thunderbird to build. Let's isolate the stuff that is actually needed and just take care of that.

Comment 23

[(No longer employed by Mozilla) Aaron Klotz]

Also, in Firefox we build xpcshell.exe and plugin-container.exe just fine without linking them against launcher process libs, so there's no reason why we need to do that for thunderbird.exe. We just need to sort out mozglue's header dependencies.

Comment 24

[Jorg K (CEST = GMT+2)]

I whole-heartedly agree with comment #21 to comment #23. I don't want to fork that stuff and maintain it. I much prefer a "small" solution. Can you give us a hand with "We just need to sort out mozglue's header dependencies"?

Comment 25

[(No longer employed by Mozilla) Aaron Klotz]

Attachment: Bug 1582937: Remove mozglue -> browser header dependencies; r=mhowell!

Comment 26

[(No longer employed by Mozilla) Aaron Klotz]

Can you try my patch in https://phabricator.services.mozilla.com/D46715 and see if I got everything?

Comment 27

[Jorg K (CEST = GMT+2)]

Compiles for me, thanks!

Comment 28

[(No longer employed by Mozilla) Aaron Klotz]

Moving to mozglue.

Comment 29

[(No longer employed by Mozilla) Aaron Klotz]

I'll fold this into the next landing of bug 1542830