Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
10407053:
Copy VC CRT DLLs for component build even when VS is not installed. (Closed)
|
Created:
8 years, 7 months ago by dmazzoni Modified:
8 years, 7 months ago CC:
chromium-reviews, pam+watch_chromium.org Base URL:
http://git.chromium.org/chromium/src.git@master Visibility:
Public. |
DescriptionCopy VC CRT DLLs for component build even when VS is not installed.
BUG=127233
TEST=Build mini_installer on system that doesn't have VS installed, test that msvc*dll get copied to build dir.
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=138440
Patch Set 1 #
Total comments: 15
Patch Set 2 : Move to separate function, improve warnings #Patch Set 3 : Get rid of string format operator #
Total comments: 6
Patch Set 4 : Switch to os.path #
Total comments: 1
Patch Set 5 : Fix nit #Messages
Total messages: 13 (0 generated)
See comments below. http://codereview.chromium.org/10407053/diff/1/chrome/tools/build/win/create_... File chrome/tools/build/win/create_installer_archive.py (right): http://codereview.chromium.org/10407053/diff/1/chrome/tools/build/win/create_... chrome/tools/build/win/create_installer_archive.py:371: if not crt_dlls: Now that this is getting bigger I would extract all of the CRT DLL copying in a separate function above and just call it from here. http://codereview.chromium.org/10407053/diff/1/chrome/tools/build/win/create_... chrome/tools/build/win/create_installer_archive.py:373: sys_dll_dir = "C:/Windows/SysWOW64" Aren't these the 64-bit DLLs? Chrome is always 32-bit no? http://codereview.chromium.org/10407053/diff/1/chrome/tools/build/win/create_... chrome/tools/build/win/create_installer_archive.py:378: crt_dlls = glob.glob("%s/msvc*0d.dll" % sys_dll_dir) I'd do this inside the "if build_dir.endswith('Debug/'):" of the previous section (`if not crt_dlls` only of course), this duplicates "if not crt_dlls" which is a fast check, but avoids duplicating ".endswith()" check which is slower. Subsequently I suggest we remove the warning in the "else" section above and replace it by a warning "if not crt_dlls" still at the end of all of this.
A couple more things below... http://codereview.chromium.org/10407053/diff/1/chrome/tools/build/win/create_... File chrome/tools/build/win/create_installer_archive.py (right): http://codereview.chromium.org/10407053/diff/1/chrome/tools/build/win/create_... chrome/tools/build/win/create_installer_archive.py:375: sys_dll_dir = "C:/Windows/System32" These DLLs are found all over the place. I'm not convinced the DLLs in System32 are the ones we want. Maybe msbuild\... has them somewhere? If so, maybe we want to only copy those instead of the ones from VS as VS uses msbuild underneath anyways...? robertshield@ would probably know more. http://codereview.chromium.org/10407053/diff/1/chrome/tools/build/win/create_... chrome/tools/build/win/create_installer_archive.py:380: crt_dlls = glob.glob("%s/msvc*0.dll" % sys_dll_dir) .format() has been preferred over % for string formatting in this function. This has already been discussed here: https://chromiumcodereview.appspot.com/9701050/diff/15002/chrome/tools/build/...
lgtm once gab's comments are responded to. http://codereview.chromium.org/10407053/diff/1/chrome/tools/build/win/create_... File chrome/tools/build/win/create_installer_archive.py (right): http://codereview.chromium.org/10407053/diff/1/chrome/tools/build/win/create_... chrome/tools/build/win/create_installer_archive.py:373: sys_dll_dir = "C:/Windows/SysWOW64" On 2012/05/20 14:42:54, gab wrote: > Aren't these the 64-bit DLLs? Chrome is always 32-bit no? Wow64 means "Windows 32-bit on Windows 64 bit", so this directory contains 32 bit dlls. http://codereview.chromium.org/10407053/diff/1/chrome/tools/build/win/create_... chrome/tools/build/win/create_installer_archive.py:375: sys_dll_dir = "C:/Windows/System32" On 2012/05/20 15:05:01, gab wrote: > These DLLs are found all over the place. I'm not convinced the DLLs in System32 > are the ones we want. > > Maybe msbuild\... has them somewhere? If so, maybe we want to only copy those > instead of the ones from VS as VS uses msbuild underneath anyways...? > > robertshield@ would probably know more. I don't know offhand which dlls are picked up when using msbuild without VS installed. I suspect they are the ones in the system dir but would have to check. If they are then this is fine.
http://codereview.chromium.org/10407053/diff/1/chrome/tools/build/win/create_... File chrome/tools/build/win/create_installer_archive.py (right): http://codereview.chromium.org/10407053/diff/1/chrome/tools/build/win/create_... chrome/tools/build/win/create_installer_archive.py:371: if not crt_dlls: On 2012/05/20 14:42:54, gab wrote: > Now that this is getting bigger I would extract all of the CRT DLL copying in a > separate function above and just call it from here. Done. http://codereview.chromium.org/10407053/diff/1/chrome/tools/build/win/create_... chrome/tools/build/win/create_installer_archive.py:373: sys_dll_dir = "C:/Windows/SysWOW64" On 2012/05/20 17:36:16, robertshield wrote: > On 2012/05/20 14:42:54, gab wrote: > > Aren't these the 64-bit DLLs? Chrome is always 32-bit no? > > Wow64 means "Windows 32-bit on Windows 64 bit", so this directory contains 32 > bit dlls. Worth a comment; I certainly wouldn't have guessed this. http://codereview.chromium.org/10407053/diff/1/chrome/tools/build/win/create_... chrome/tools/build/win/create_installer_archive.py:375: sys_dll_dir = "C:/Windows/System32" On 2012/05/20 17:36:16, robertshield wrote: > On 2012/05/20 15:05:01, gab wrote: > > These DLLs are found all over the place. I'm not convinced the DLLs in > System32 > > are the ones we want. > > > > Maybe msbuild\... has them somewhere? If so, maybe we want to only copy those > > instead of the ones from VS as VS uses msbuild underneath anyways...? > > > > robertshield@ would probably know more. > > I don't know offhand which dlls are picked up when using msbuild without VS > installed. I suspect they are the ones in the system dir but would have to > check. If they are then this is fine. FWIW, I searched my system and did not find them anywhere else. Unless I missed something, winsdk doesn't provide an equivalent to the "redist" directory with all of the needed runtime DLLs, but I think it does install them to your system directory if they're not already there. One possible reason is that winsdk is compatible with multiple versions of VS (even though it includes one compiler toolchain), while each VS install has exactly one set of runtime DLLs. http://codereview.chromium.org/10407053/diff/1/chrome/tools/build/win/create_... chrome/tools/build/win/create_installer_archive.py:378: crt_dlls = glob.glob("%s/msvc*0d.dll" % sys_dll_dir) On 2012/05/20 14:42:54, gab wrote: > I'd do this inside the "if build_dir.endswith('Debug/'):" of the previous > section (`if not crt_dlls` only of course), this duplicates "if not crt_dlls" > which is a fast check, but avoids duplicating ".endswith()" check which is > slower. I'd argue that the "if os.access" is by far the most expensive check. Since having VS installed is the more common case, I think it makes sense to defer that computation to the case where the DLLs weren't found in the obvious place. Instead, I moved the build_dir.endswith computation to the top so that it's only done once. Does that work? > Subsequently I suggest we remove the warning in the "else" section above and > replace it by a warning "if not crt_dlls" still at the end of all of this. I split it into two separate warnings, take a look. I thought it'd be simpler to default to Release dlls if the build type couldn't be determined. My reasoning is that if someone's copying the target to another machine, it's far more likely they're copying a Release build. It couldn't hurt. http://codereview.chromium.org/10407053/diff/1/chrome/tools/build/win/create_... chrome/tools/build/win/create_installer_archive.py:380: crt_dlls = glob.glob("%s/msvc*0.dll" % sys_dll_dir) On 2012/05/20 15:05:01, gab wrote: > .format() has been preferred over % for string formatting in this function. > > This has already been discussed here: > https://chromiumcodereview.appspot.com/9701050/diff/15002/chrome/tools/build/... Actually in this case a simple string append might be the most readable. Does that look okay? If not I'm happy to use the new .format() awesomeness.
On Mon, May 21, 2012 at 2:28 PM, <dmazzoni@chromium.org> wrote: > I'd argue that the "if os.access" is by far the most expensive check. > ...after glob, of course.
Looks great, one remaining nit from me around the path matching http://codereview.chromium.org/10407053/diff/1/chrome/tools/build/win/create_... File chrome/tools/build/win/create_installer_archive.py (right): http://codereview.chromium.org/10407053/diff/1/chrome/tools/build/win/create_... chrome/tools/build/win/create_installer_archive.py:375: sys_dll_dir = "C:/Windows/System32" On 2012/05/21 21:28:39, Dominic Mazzoni wrote: > On 2012/05/20 17:36:16, robertshield wrote: > > On 2012/05/20 15:05:01, gab wrote: > > > These DLLs are found all over the place. I'm not convinced the DLLs in > > System32 > > > are the ones we want. > > > > > > Maybe msbuild\... has them somewhere? If so, maybe we want to only copy > those > > > instead of the ones from VS as VS uses msbuild underneath anyways...? > > > > > > robertshield@ would probably know more. > > > > I don't know offhand which dlls are picked up when using msbuild without VS > > installed. I suspect they are the ones in the system dir but would have to > > check. If they are then this is fine. > > FWIW, I searched my system and did not find them anywhere else. Unless I missed > something, winsdk doesn't provide an equivalent to the "redist" directory with > all of the needed runtime DLLs, but I think it does install them to your system > directory if they're not already there. > > One possible reason is that winsdk is compatible with multiple versions of VS > (even though it includes one compiler toolchain), while each VS install has > exactly one set of runtime DLLs. Ok, interesting. Since all that really matters is that the runtime DLLs being copied are the same as the ones that are built against, this looks fine to me. http://codereview.chromium.org/10407053/diff/1003/chrome/tools/build/win/crea... File chrome/tools/build/win/create_installer_archive.py (right): http://codereview.chromium.org/10407053/diff/1003/chrome/tools/build/win/crea... chrome/tools/build/win/create_installer_archive.py:342: is_debug = build_dir.endswith('Debug/') is the trailing slash guaranteed to be present? If not, can you use os.path.normpath()? Also, os.path.basename() would be slightly better than endswith() (though that requires normpath() to be called first). http://codereview.chromium.org/10407053/diff/1003/chrome/tools/build/win/crea... chrome/tools/build/win/create_installer_archive.py:360: # On a 64-bit system, 32-bit dlls are in SysWOW64 (don't ask). indeed :-)
Very nice. See comments below. http://codereview.chromium.org/10407053/diff/1/chrome/tools/build/win/create_... File chrome/tools/build/win/create_installer_archive.py (right): http://codereview.chromium.org/10407053/diff/1/chrome/tools/build/win/create_... chrome/tools/build/win/create_installer_archive.py:378: crt_dlls = glob.glob("%s/msvc*0d.dll" % sys_dll_dir) On 2012/05/21 21:28:39, Dominic Mazzoni wrote: > On 2012/05/20 14:42:54, gab wrote: > > I'd do this inside the "if build_dir.endswith('Debug/'):" of the previous > > section (`if not crt_dlls` only of course), this duplicates "if not crt_dlls" > > which is a fast check, but avoids duplicating ".endswith()" check which is > > slower. > > I'd argue that the "if os.access" is by far the most expensive check. Since > having VS installed is the more common case, I think it makes sense to defer > that computation to the case where the DLLs weren't found in the obvious place. > > Instead, I moved the build_dir.endswith computation to the top so that it's only > done once. Does that work? Yep. > > > Subsequently I suggest we remove the warning in the "else" section above and > > replace it by a warning "if not crt_dlls" still at the end of all of this. > > I split it into two separate warnings, take a look. > > I thought it'd be simpler to default to Release dlls if the build type couldn't > be determined. My reasoning is that if someone's copying the target to another > machine, it's far more likely they're copying a Release build. It couldn't hurt. Perfect. http://codereview.chromium.org/10407053/diff/1/chrome/tools/build/win/create_... chrome/tools/build/win/create_installer_archive.py:380: crt_dlls = glob.glob("%s/msvc*0.dll" % sys_dll_dir) On 2012/05/21 21:28:39, Dominic Mazzoni wrote: > On 2012/05/20 15:05:01, gab wrote: > > .format() has been preferred over % for string formatting in this function. > > > > This has already been discussed here: > > > https://chromiumcodereview.appspot.com/9701050/diff/15002/chrome/tools/build/... > > Actually in this case a simple string append might be the most readable. Does > that look okay? If not I'm happy to use the new .format() awesomeness. Works for me. http://codereview.chromium.org/10407053/diff/1003/chrome/tools/build/win/crea... File chrome/tools/build/win/create_installer_archive.py (right): http://codereview.chromium.org/10407053/diff/1003/chrome/tools/build/win/crea... chrome/tools/build/win/create_installer_archive.py:342: is_debug = build_dir.endswith('Debug/') On 2012/05/22 02:39:59, robertshield wrote: > is the trailing slash guaranteed to be present? Yes, see [1] below. > If not, can you use os.path.normpath()? Also, os.path.basename() would be slightly better than > endswith() (though that requires normpath() to be called first). I added [1] to make this "work", but I agree now that it's better to use os.path methods. If you could change this, that'd be better I think. Maybe replace [1] by os.path.normpath and then simply use os.path.basename here. http://codereview.chromium.org/10407053/diff/1003/chrome/tools/build/win/crea... chrome/tools/build/win/create_installer_archive.py:360: # On a 64-bit system, 32-bit dlls are in SysWOW64 (don't ask). On 2012/05/22 02:39:59, robertshield wrote: > indeed :-) Aaaah! Thanks for clarifying :)! http://codereview.chromium.org/10407053/diff/1003/chrome/tools/build/win/crea... chrome/tools/build/win/create_installer_archive.py:394: CopyVisualStudioRuntimeDLLs(build_dir) optional: Add a comment as to why this needs to be done here. Something like: // Copy the VS CRT DLLs to |build_dir|. This must be done before the // general copy step below to ensure the CRT DLLs are added to the // archive and marked as a dependency in the exe manifests generated // below. http://codereview.chromium.org/10407053/diff/1003/chrome/tools/build/win/crea... chrome/tools/build/win/create_installer_archive.py:564: options.build_dir += '/' [1]
Using normpath and basepath sounds great. Everything should be addressed, ready for a final look...
lgtm (after last nit is addressed). Cheers, Gab http://codereview.chromium.org/10407053/diff/1004/chrome/tools/build/win/crea... File chrome/tools/build/win/create_installer_archive.py (right): http://codereview.chromium.org/10407053/diff/1004/chrome/tools/build/win/crea... chrome/tools/build/win/create_installer_archive.py:343: if not is_debug and not os.path.basename(build_dir) == 'Release': 'not A == B' => 'A != B'
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/dmazzoni@chromium.org/10407053/10002
Try job failure for 10407053-10002 (retry) on win_rel for step "compile" (clobber build). It's a second try, previously, step "compile" failed. http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win_rel&nu...
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/dmazzoni@chromium.org/10407053/10002 |
