Closed Bug 944800 Opened 6 years ago Closed 6 years ago

Move the information about delay loaded DLLs into moz.build

Categories

(Firefox Build System :: General, defect)

x86
macOS
defect
Not set

Tracking

(Not tracked)

RESOLVED FIXED
mozilla30

People

(Reporter: ehsan, Assigned: ehsan)

References

Details

Attachments

(1 file, 3 obsolete files)

Note to self: this is an example of what we need to do here: http://hg.mozilla.org/mozilla-central/rev/60139be512d4#l1.14
(totally untested)
Attachment #8342576 - Attachment is obsolete: true
Comment on attachment 8365616 [details] [diff] [review]
Move the information about delay loaded DLLs into moz.build; r=gps

Note that for the test changes, I just blindly followed what was there for other variables, so those parts are not tested.  The patch works as expected on Windows and doesn't change the build configuration on other platforms.
Attachment #8365616 - Flags: review?(gps)
ping?
Comment on attachment 8365616 [details] [diff] [review]
Move the information about delay loaded DLLs into moz.build; r=gps

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

LGMT.

::: python/mozbuild/mozbuild/frontend/emitter.py
@@ +247,5 @@
>              passthru.variables['VISIBILITY_FLAGS'] = ''
>  
> +        if sandbox['DELAYLOAD_DLLS']:
> +            passthru.variables['DELAYLOAD_LDFLAGS'] = [('-DELAYLOAD:%s' % dll) for dll in sandbox['DELAYLOAD_DLLS']]
> +            passthru.variables['USE_DELAYIMP'] = True

The docs say that DELAYLOAD_DLLS only has an impact when building on visual studio. Should we detect and error here if we encounter this variable on other configs? Or, should it silently no-op? I could go either way.

::: toolkit/library/moz.build
@@ +31,5 @@
> +    'wininet.dll',
> +    'winspool.dll'
> +]
> +
> +if CONFIG['MOZ_METRO']:

You lost the comment about "See nsDllMain for an explanation." I have no idea if that comment was still valid b/c I have no idea what nsDllMain is!
Attachment #8365616 - Flags: review?(gps) → review+
Comment on attachment 8365616 [details] [diff] [review]
Move the information about delay loaded DLLs into moz.build; r=gps

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

::: python/mozbuild/mozbuild/frontend/emitter.py
@@ +247,5 @@
>              passthru.variables['VISIBILITY_FLAGS'] = ''
>  
> +        if sandbox['DELAYLOAD_DLLS']:
> +            passthru.variables['DELAYLOAD_LDFLAGS'] = [('-DELAYLOAD:%s' % dll) for dll in sandbox['DELAYLOAD_DLLS']]
> +            passthru.variables['USE_DELAYIMP'] = True

I will update the docs, that was an oversight.  (It will be no-op when not building with MSVC.)

::: toolkit/library/moz.build
@@ +31,5 @@
> +    'wininet.dll',
> +    'winspool.dll'
> +]
> +
> +if CONFIG['MOZ_METRO']:

The comment is probably talking about http://mxr.mozilla.org/mozilla-central/source/toolkit/library/nsDllMain.cpp but that file doesn't describe much really, which is why I left out the comment.
Backed out because of test failures:

https://hg.mozilla.org/integration/mozilla-inbound/rev/6a25e4cef8da
https://tbpl.mozilla.org/?tree=Mozilla-Inbound&rev=b54e8c328c32

Greg, how do I run these tests locally?
Flags: needinfo?(gps)
mach build python/check
Flags: needinfo?(gps)
Attachment #8365616 - Attachment is obsolete: true
Attachment #8370942 - Flags: review?(gps)
The windows reds are... interesting!
I cannot reproduce the Windows build failures locally.  If nobody has ideas on what could be happening there, I may have to give up on this. :(
Smells like a "missing dll" popup waiting for a click, but we don't have screenshots.
Did you run "make package" locally? That's what would reproduce the issue.
(In reply to comment #14)
> Smells like a "missing dll" popup waiting for a click, but we don't have
> screenshots.

Yeah...  But the thing is, delay loading a DLL shouldn't change the way that the OS looks for it AFAIK.

In my local build, the following xul.dll imports are delay loaded (verified using Dependency Walker):

* secur32.dll
* msdmo.dll
* wininet.dll
* comdlg32.dll
* psapi.dll
* rasapi32.dll
* rasdlg.dll
* oleacc.dll

Unless I'm missing something, those are all expected.

> Did you run "make package" locally? That's what would reproduce the issue.

I did run ./mach package and that finished without any problems.

Any other ideas on what I may be doing wrong?
Depends on: 968497
Attachment #8370942 - Attachment is obsolete: true
Attachment #8370942 - Flags: review?(gps)
Attachment #8371092 - Flags: review?(gps)
FWIW we talked with glandium about this on IRC, turns out that I was misspelling the names of three DLLs in the move to moz.build.  The current working theory is that those DLLs do not exist on our builders, therefore loading xul.dll without them being around causes Windows to show the "foo.dll not found" message box, waiting for somebody to press Enter there, hence the timeout when running xpcshell.exe.
That did fix the build bustage on try!
Attachment #8371092 - Flags: review?(mh+mozilla)
Considering this is a typo fix, you don't need another review.
Comment on attachment 8371092 [details] [diff] [review]
Move the information about delay loaded DLLs into moz.build; r=gps

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

LGTM.
Attachment #8371092 - Flags: review?(mh+mozilla)
Attachment #8371092 - Flags: review?(gps)
Attachment #8371092 - Flags: review+
https://hg.mozilla.org/mozilla-central/rev/8b74966f80da
Status: NEW → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla30
Product: Core → Firefox Build System
You need to log in before you can comment on or make changes to this bug.