VS2015 CRT DLLs are not packaged somehow

RESOLVED FIXED in Firefox 48

Status

defect
RESOLVED FIXED
4 years ago
Last year

People

(Reporter: emk, Assigned: emk)

Tracking

unspecified
mozilla48
Dependency tree / graph

Firefox Tracking Flags

(firefox48 fixed)

Details

Attachments

(4 attachments)

When I built Firefox with MSVC 2015, I could not find VC2015 CRT DLLs in $objdir/dist/bin. Those file are not found in the installer when I built an installer.
If I only set WIN32_REDIST_DIR in my .mozconfig, vcruntime140.dll and msvcp140.dll is copied.
If I also set WIN_UCRT_REDIST_DIR and comment out <https://hg.mozilla.org/mozilla-central/rev/0fc068ae3289#l5.22>, ucrtbase.dll is also copied. So the environment variable should be correct. But if I un-comment out the line, no files are copied including vcruntime and ucrtbase.
My guess is that this due to spaces in your WIN_UCRT_REDIST_DIR path (wildcard doesn't like spaces). When I tested this, I didn't have spaces in my path.
Assignee: nobody → birunthan
Status: NEW → ASSIGNED
(In reply to Birunthan Mohanathas [:poiru] from comment #1)
> My guess is that this due to spaces in your WIN_UCRT_REDIST_DIR path
> (wildcard doesn't like spaces). When I tested this, I didn't have spaces in
> my path.

Yeah, my WIN_UCRT_REDIST_DIR contains spaces because I installed the SDK to the default location.
I tried using the short path name but it still doesn't move the DLLs:
export WIN_UCRT_REDIST_DIR="C:/PROGRA~2/WI3CF2~1/10/Redist/ucrt/DLLs/x64"
The short path didn't help me, either.
I copied Redist directories to a path that does not contain space, but api-ms-win-*.dll is not still copied. $(wildcard) does not work for me somehow.
Stealing.

The default install path contains not only spaces but also parenthesis. This patch will deal with both.
Assignee: birunthan → VYV03354
Attachment #8735031 - Flags: review?(mh+mozilla)
Comment on attachment 8735031 [details] [diff] [review]
Avoid using $(wildcard)

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

Do we have enough of configure in Python that we could move the finding of these files into Python configure and set a variable with the list of UCRT file basenames so we don't have to do ugly $(shell) or $(wildcard) hacks in make files?
(In reply to Gregory Szorc [:gps] from comment #7)
> Comment on attachment 8735031 [details] [diff] [review]
> Avoid using $(wildcard)
> 
> Review of attachment 8735031 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Do we have enough of configure in Python that we could move the finding of
> these files into Python configure and set a variable with the list of UCRT
> file basenames so we don't have to do ugly $(shell) or $(wildcard) hacks in
> make files?

Not yet, but soon. That should be possible after bug 1259382, and I'm currently on it (because we only want to check the UCRT if we build with MSVC).
Comment on attachment 8735031 [details] [diff] [review]
Avoid using $(wildcard)

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

Seeing how the patch is, I'd rather way for bug 1259382 and move this straight to python configure. That will be less awful.
Attachment #8735031 - Flags: review?(mh+mozilla)
When will bug 1259382 be fixed? I have to apply the patch locally until it is fixed.
This week.
Now that I'm a stage of bug 1259382 where there is enough for the purpose of this bug, I'm thinking it would be better served by moving those REDIST_FILES to moz.build, using FINAL_TARGET_FILES. Unfortunately, the recursive make backend rejects wildcards when using absolute paths (%), and even if it didn't, it looks like at quick glance that it doesn't handle things more subtle than /some/dir/*.

Mike, do you think you can give this a spin? This would remove almost everything from build/win32/Makefile.in (the remainder is a make check rule that could be moved elsewhere).
Flags: needinfo?(mshal)
Do you need wildcards in the directory parts? It's pretty easy to add support for the '$(wildcard $(WIN_UCRT_REDIST_DIR)/api-ms-win-*.dll)' case with what we have, but something like $(DIRNAME)/**/*.foo is harder to do correctly, since we don't really know where the break between the base path and the pattern should be. Maybe we could fudge it to say everything before the first path part with a '*' is the base, and everything after is the pattern. But I suspect it would be better to explicitly give the two pieces in moz.build as a tuple or something, like (CONFIG[WIN_UCRT_REDIST_DIR], '**/*.foo')

If we don't need wildcard directories for this (from build/win32/Makefile.in, it doesn't look like it), the upcoming patch should suffice.
Flags: needinfo?(mshal)
:glandium, does this suffice for what you want to do?

:aleth, can you check if this breaks where you're using srcdir-rooted wildcards in c-c?
Flags: needinfo?(aleth)
Attachment #8738286 - Flags: feedback?(mh+mozilla)
Comment on attachment 8738286 [details] [diff] [review]
0001-Bug-1245701-Allow-absolute-paths-with-wildcards-in-F.patch

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

This looks like it should be fine, thanks.
Attachment #8738286 - Flags: feedback?(mh+mozilla) → feedback+
Comment on attachment 8738286 [details] [diff] [review]
0001-Bug-1245701-Allow-absolute-paths-with-wildcards-in-F.patch

Oops, I also wanted feedback from glandium.
Attachment #8738286 - Flags: feedback?(mh+mozilla)
Comment on attachment 8738286 [details] [diff] [review]
0001-Bug-1245701-Allow-absolute-paths-with-wildcards-in-F.patch

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

::: python/mozbuild/mozbuild/backend/recursivemake.py
@@ +1248,5 @@
>                  assert not isinstance(f, RenamedSourcePath)
>                  dest = mozpath.join(reltarget, path, f.target_basename)
>                  if not isinstance(f, ObjDirPath):
>                      if '*' in f:
> +                        if f.startswith('/') or f.startswith('%'):

f.startswith('%') should be replaced with isinstance(f, AbsolutePath)
Attachment #8738286 - Flags: feedback?(mh+mozilla) → feedback+
Flags: needinfo?(aleth)
:emk, can you confirm that these two patches (#c19, #c20) fix the issue for you?
Flags: needinfo?(VYV03354)
Works for me. Thanks!
Flags: needinfo?(VYV03354)
Comment on attachment 8739147 [details]
MozReview Request: Bug 1245701 - Allow absolute paths with wildcards in FINAL_TARGET_FILES; r?glandium

https://reviewboard.mozilla.org/r/45067/#review42157
Attachment #8739147 - Flags: review?(mh+mozilla) → review+
Comment on attachment 8739148 [details]
MozReview Request: Bug 1245701 - Port build/win32 install rules to moz.build; r?glandium

https://reviewboard.mozilla.org/r/45069/#review42159
Attachment #8739148 - Flags: review?(mh+mozilla) → review+
https://hg.mozilla.org/mozilla-central/rev/6a769c9909d9
https://hg.mozilla.org/mozilla-central/rev/06cc94fa4cdf
Status: ASSIGNED → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla48
Depends on: 1264276
Product: Core → Firefox Build System
You need to log in before you can comment on or make changes to this bug.