Avoid using $(wildcard) 10 years ago Masatoshi Kimura [:emk] 1.39 KB, patch	Details \| Diff \| Splinter Review
0001-Bug-1245701-Allow-absolute-paths-with-wildcards-in-F.patch 10 years ago Michael Shal [:mshal] 2.52 KB, patch	aleth : feedback+ glandium : feedback+	Details \| Diff \| Splinter Review
MozReview Request: Bug 1245701 - Allow absolute paths with wildcards in FINAL_TARGET_FILES; r?glandium 10 years ago Michael Shal [:mshal] 58 bytes, text/x-review-board-request	glandium : review+	Details
MozReview Request: Bug 1245701 - Port build/win32 install rules to moz.build; r?glandium 10 years ago Michael Shal [:mshal] 58 bytes, text/x-review-board-request	glandium : review+	Details

👁 Image	Masatoshi Kimura [:emk] Assignee
	Description • 10 years ago

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.

👁 Image	poiru
	Comment 1 • 10 years ago

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

👁 Image	Masatoshi Kimura [:emk] Assignee
	Comment 2 • 10 years ago

(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.

👁 Image	Alex Kontos
	Comment 3 • 10 years ago

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"

👁 Image	Masatoshi Kimura [:emk] Assignee
	Comment 4 • 10 years ago

The short path didn't help me, either.

👁 Image	Masatoshi Kimura [:emk] Assignee
	Comment 5 • 10 years ago

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.

👁 Image	Masatoshi Kimura [:emk] Assignee
	Comment 6 • 10 years ago

Attached patch Avoid using $(wildcard) — Details — Splinter Review

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)

👁 Image	Gregory Szorc [:gps]
	Comment 7 • 10 years ago

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?

👁 Image	Mike Hommey [:glandium] OoO until Jun 18
	Comment 8 • 10 years ago

(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).

👁 Image	Masatoshi Kimura [:emk] Assignee
	Comment 9 • 10 years ago

https://treeherder.mozilla.org/#/jobs?repo=try&revision=02e9c66843a1

👁 Image	Mike Hommey [:glandium] OoO until Jun 18
	Comment 10 • 10 years ago

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)

👁 Image	Masatoshi Kimura [:emk] Assignee
	Comment 11 • 10 years ago

When will bug 1259382 be fixed? I have to apply the patch locally until it is fixed.

👁 Image	Mike Hommey [:glandium] OoO until Jun 18
	Comment 12 • 10 years ago

This week.

👁 Image	Mike Hommey [:glandium] OoO until Jun 18
	Comment 13 • 10 years ago

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)

👁 Image	Michael Shal [:mshal]
	Comment 14 • 10 years ago

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)

👁 Image	Michael Shal [:mshal]
	Comment 15 • 10 years ago

Attached patch 0001-Bug-1245701-Allow-absolute-paths-with-wildcards-in-F.patch — Details — Splinter Review

: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)

👁 Image	aleth [:aleth]
	Comment 16 • 10 years ago

Attachment #8738286 - Flags: feedback?(mh+mozilla) → feedback+

👁 Image	Michael Shal [:mshal]
	Comment 17 • 10 years ago

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)

👁 Image	Mike Hommey [:glandium] OoO until Jun 18
	Comment 18 • 10 years ago

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+

👁 Image

aleth [:aleth]