Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
8059024:
Mostly automatic incremental link enabling (Closed)
|
Created:
9 years, 2 months ago by scottmg Modified:
8 years, 8 months ago CC:
chromium-reviews, pam+watch_chromium.org Visibility:
Public. |
DescriptionMostly automatic incremental link enabling
Patch and tool to make turning on incremental linking on Windows easy.
BUG=94837
TEST=No link errors
Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=103474
Patch Set 1 #
Total comments: 3
Patch Set 2 : { wrong line, reinterpret instead of C-style, banish capitals, del owners #
Total comments: 71
Patch Set 3 : fixes per review #
Total comments: 36
Patch Set 4 : further review changes #Patch Set 5 : remove GetStartupInfo call #
Total comments: 10
Patch Set 6 : review fixes #Patch Set 7 : forgot one change #Patch Set 8 : and another #
Total comments: 1
Patch Set 9 : make sure tmp file is unlink'd #
Total comments: 1
Messages
Total messages: 17 (0 generated)
Documented here: http://code.google.com/p/chromium/wiki/WindowsIncrementalLinking Briefly, add install script in tools/win/supalink that the user runs. That installs the link.exe wrapper, and writes a registry key that gyp uses to determine if it should turn ULDI on. This is in preference to overwriting the user's link.exe automatically. Either deleting the registry key, or restoring the original linker turns it off.
bunch of nits that are repeated btw are you really worried about needing an OWNERS file there? i worry when i see a file with only one OWNERS http://codereview.chromium.org/8059024/diff/1/tools/win/supalink/supalink.cpp File tools/win/supalink/supalink.cpp (right): http://codereview.chromium.org/8059024/diff/1/tools/win/supalink/supalink.cpp... tools/win/supalink/supalink.cpp:15: { nit: brace bracket after function, see style guide http://codereview.chromium.org/8059024/diff/1/tools/win/supalink/supalink.cpp... tools/win/supalink/supalink.cpp:22: LPTSTR msgBuf = NULL; msg_buf (see style guide) http://codereview.chromium.org/8059024/diff/1/tools/win/supalink/supalink.cpp... tools/win/supalink/supalink.cpp:118: wchar_t* data = (wchar_t*)malloc(len); use reinterpret_cast (see style guide)
On 2011/09/27 22:10:34, John Abd-El-Malek wrote: > bunch of nits that are repeated thanks > btw are you really worried about needing an OWNERS file there? i worry when i > see a file with only one OWNERS Deleted. I thought presubmit had complained, but maybe that was in a different location. I think it follows google-style more closely now. No mixedCase, braces on previous line, reinterpret_cast.
http://codereview.chromium.org/8059024/diff/4001/tools/win/supalink/check_ins... File tools/win/supalink/check_installed.py (right): http://codereview.chromium.org/8059024/diff/4001/tools/win/supalink/check_ins... tools/win/supalink/check_installed.py:1: import sys Copyright header http://codereview.chromium.org/8059024/diff/4001/tools/win/supalink/check_ins... tools/win/supalink/check_installed.py:1: import sys shebang http://codereview.chromium.org/8059024/diff/4001/tools/win/supalink/check_ins... tools/win/supalink/check_installed.py:1: import sys chmod +x http://codereview.chromium.org/8059024/diff/4001/tools/win/supalink/check_ins... tools/win/supalink/check_installed.py:6: return return 0 http://codereview.chromium.org/8059024/diff/4001/tools/win/supalink/check_ins... tools/win/supalink/check_installed.py:9: import os file level import for 'os'. http://codereview.chromium.org/8059024/diff/4001/tools/win/supalink/check_ins... tools/win/supalink/check_installed.py:18: return return 0 http://codereview.chromium.org/8059024/diff/4001/tools/win/supalink/check_ins... tools/win/supalink/check_installed.py:23: return 0 http://codereview.chromium.org/8059024/diff/4001/tools/win/supalink/check_ins... tools/win/supalink/check_installed.py:24: if __name__ == '__main__': 2 lines between file level "symbols" http://codereview.chromium.org/8059024/diff/4001/tools/win/supalink/check_ins... tools/win/supalink/check_installed.py:26: sys.exit(0) sys.exit(main()) http://codereview.chromium.org/8059024/diff/4001/tools/win/supalink/install_s... File tools/win/supalink/install_supalink.py (right): http://codereview.chromium.org/8059024/diff/4001/tools/win/supalink/install_s... tools/win/supalink/install_supalink.py:1: # Copyright (c) 2011 The Chromium Authors. All rights reserved. shebang http://codereview.chromium.org/8059024/diff/4001/tools/win/supalink/install_s... tools/win/supalink/install_supalink.py:10: VSVARS_PATH = 'C:\\Program Files (x86)\\Microsoft Visual Studio 9.0\ use () instead of \ http://codereview.chromium.org/8059024/diff/4001/tools/win/supalink/install_s... tools/win/supalink/install_supalink.py:12: two lines between file level symbols http://codereview.chromium.org/8059024/diff/4001/tools/win/supalink/install_s... tools/win/supalink/install_supalink.py:19: p = subprocess.Popen(['temp.bat'], shell=True, stdout=subprocess.PIPE) I'd prefer an option to not have to write a tempfile, namely: ['cmd.exe', '/c', '%s && %s' % (VSVARS_PATH, cmd)] http://codereview.chromium.org/8059024/diff/4001/tools/win/supalink/install_s... tools/win/supalink/install_supalink.py:22: out = out.replace('\r', '') universal_newlines=True? http://codereview.chromium.org/8059024/diff/4001/tools/win/supalink/install_s... tools/win/supalink/install_supalink.py:33: def main(): vcdir = os.environ.get('VCINSTALLDIR') if not vcdir: http://codereview.chromium.org/8059024/diff/4001/tools/win/supalink/install_s... tools/win/supalink/install_supalink.py:36: os.environ['PATH'] += ';' + os.path.join(vcdir, 'bin') + \ use () http://codereview.chromium.org/8059024/diff/4001/tools/win/supalink/install_s... tools/win/supalink/install_supalink.py:48: raise SystemExit('link.exe not found at %s' % link) print 'link.exe not found at %s' % link return 1 http://codereview.chromium.org/8059024/diff/4001/tools/win/supalink/install_s... tools/win/supalink/install_supalink.py:57: raise SystemExit('Wasn\'t able to back up %s to %s.\ same http://codereview.chromium.org/8059024/diff/4001/tools/win/supalink/install_s... tools/win/supalink/install_supalink.py:65: rc, out = run_with_vsvars('cl /nologo /Ox /Zi /W4 /WX\ same http://codereview.chromium.org/8059024/diff/4001/tools/win/supalink/install_s... tools/win/supalink/install_supalink.py:68: if rc != 0: if rc: http://codereview.chromium.org/8059024/diff/4001/tools/win/supalink/install_s... tools/win/supalink/install_supalink.py:70: raise SystemExit('Failed to build supalink.exe') same http://codereview.chromium.org/8059024/diff/4001/tools/win/supalink/install_s... tools/win/supalink/install_supalink.py:79: raise SystemExit('Wasn\'t able to copy supalink.exe over %s.\ same http://codereview.chromium.org/8059024/diff/4001/tools/win/supalink/install_s... tools/win/supalink/install_supalink.py:83: 'Software\\Chromium\\supalink_installed', weird alignment. http://codereview.chromium.org/8059024/diff/4001/tools/win/supalink/install_s... tools/win/supalink/install_supalink.py:87: print 'Linker shim installed. Regenerate via gyp: "gclient runhooks".' return 0 http://codereview.chromium.org/8059024/diff/4001/tools/win/supalink/install_s... tools/win/supalink/install_supalink.py:88: two lines between file level symbols http://codereview.chromium.org/8059024/diff/4001/tools/win/supalink/install_s... tools/win/supalink/install_supalink.py:90: main() sys.exit(main()) http://codereview.chromium.org/8059024/diff/4001/tools/win/supalink/supalink.cpp File tools/win/supalink/supalink.cpp (right): http://codereview.chromium.org/8059024/diff/4001/tools/win/supalink/supalink.... tools/win/supalink/supalink.cpp:15: fprintf(stdout, "supalink fatal error: %s\n", msg); why not wprintf() then? http://codereview.chromium.org/8059024/diff/4001/tools/win/supalink/supalink.... tools/win/supalink/supalink.cpp:19: static string ErrorMessageToString(DWORD err) { wstring http://codereview.chromium.org/8059024/diff/4001/tools/win/supalink/supalink.... tools/win/supalink/supalink.cpp:20: LPTSTR msg_buf = NULL; wchar_t* http://codereview.chromium.org/8059024/diff/4001/tools/win/supalink/supalink.... tools/win/supalink/supalink.cpp:25: (LPTSTR)&msg_buf, reinterpret_cast<>() http://codereview.chromium.org/8059024/diff/4001/tools/win/supalink/supalink.... tools/win/supalink/supalink.cpp:34: static void Fallback(const char* msg = 0) { wchar_t http://codereview.chromium.org/8059024/diff/4001/tools/win/supalink/supalink.... tools/win/supalink/supalink.cpp:36: fprintf(stdout, same http://codereview.chromium.org/8059024/diff/4001/tools/win/supalink/supalink.... tools/win/supalink/supalink.cpp:49: const char* search_for[] = { Move to const global http://codereview.chromium.org/8059024/diff/4001/tools/win/supalink/supalink.... tools/win/supalink/supalink.cpp:85: (LPSTR)cmd.c_str(), same http://codereview.chromium.org/8059024/diff/4001/tools/win/supalink/supalink.... tools/win/supalink/supalink.cpp:103: wstring SlurpFile(const char* path) { so I'll stop here but make the project unicode please.
Thanks! Is Unicode build just general policy or is there something that MBCS code would have broken on? http://codereview.chromium.org/8059024/diff/4001/tools/win/supalink/check_ins... File tools/win/supalink/check_installed.py (right): http://codereview.chromium.org/8059024/diff/4001/tools/win/supalink/check_ins... tools/win/supalink/check_installed.py:1: import sys On 2011/09/27 23:21:33, Marc-Antoine Ruel wrote: > Copyright header Done. http://codereview.chromium.org/8059024/diff/4001/tools/win/supalink/check_ins... tools/win/supalink/check_installed.py:1: import sys On 2011/09/27 23:21:33, Marc-Antoine Ruel wrote: > shebang Done. http://codereview.chromium.org/8059024/diff/4001/tools/win/supalink/check_ins... tools/win/supalink/check_installed.py:1: import sys On 2011/09/27 23:21:33, Marc-Antoine Ruel wrote: > chmod +x Is this possible from a Windows git cl? http://codereview.chromium.org/8059024/diff/4001/tools/win/supalink/check_ins... tools/win/supalink/check_installed.py:6: return On 2011/09/27 23:21:33, Marc-Antoine Ruel wrote: > return 0 Done. http://codereview.chromium.org/8059024/diff/4001/tools/win/supalink/check_ins... tools/win/supalink/check_installed.py:9: import os On 2011/09/27 23:21:33, Marc-Antoine Ruel wrote: > file level import for 'os'. Done. http://codereview.chromium.org/8059024/diff/4001/tools/win/supalink/check_ins... tools/win/supalink/check_installed.py:18: return On 2011/09/27 23:21:33, Marc-Antoine Ruel wrote: > return 0 Done. http://codereview.chromium.org/8059024/diff/4001/tools/win/supalink/check_ins... tools/win/supalink/check_installed.py:23: On 2011/09/27 23:21:33, Marc-Antoine Ruel wrote: > return 0 Done. http://codereview.chromium.org/8059024/diff/4001/tools/win/supalink/check_ins... tools/win/supalink/check_installed.py:24: if __name__ == '__main__': On 2011/09/27 23:21:33, Marc-Antoine Ruel wrote: > 2 lines between file level "symbols" Done. http://codereview.chromium.org/8059024/diff/4001/tools/win/supalink/check_ins... tools/win/supalink/check_installed.py:26: sys.exit(0) On 2011/09/27 23:21:33, Marc-Antoine Ruel wrote: > sys.exit(main()) Done. http://codereview.chromium.org/8059024/diff/4001/tools/win/supalink/install_s... File tools/win/supalink/install_supalink.py (right): http://codereview.chromium.org/8059024/diff/4001/tools/win/supalink/install_s... tools/win/supalink/install_supalink.py:1: # Copyright (c) 2011 The Chromium Authors. All rights reserved. On 2011/09/27 23:21:33, Marc-Antoine Ruel wrote: > shebang Done. http://codereview.chromium.org/8059024/diff/4001/tools/win/supalink/install_s... tools/win/supalink/install_supalink.py:10: VSVARS_PATH = 'C:\\Program Files (x86)\\Microsoft Visual Studio 9.0\ On 2011/09/27 23:21:33, Marc-Antoine Ruel wrote: > use () instead of \ Done. http://codereview.chromium.org/8059024/diff/4001/tools/win/supalink/install_s... tools/win/supalink/install_supalink.py:12: On 2011/09/27 23:21:33, Marc-Antoine Ruel wrote: > two lines between file level symbols Done. http://codereview.chromium.org/8059024/diff/4001/tools/win/supalink/install_s... tools/win/supalink/install_supalink.py:19: p = subprocess.Popen(['temp.bat'], shell=True, stdout=subprocess.PIPE) On 2011/09/27 23:21:33, Marc-Antoine Ruel wrote: > I'd prefer an option to not have to write a tempfile, namely: > > ['cmd.exe', '/c', '%s && %s' % (VSVARS_PATH, cmd)] I couldn't figure out the quoting required to get that to work inside a subprocess.Popen. As above doesn't work because VSVARS_PATH contains spaces. And '"%s" && %s' and 'call "%s" && %s' don't work because subprocess seems to re-escape the "s. http://codereview.chromium.org/8059024/diff/4001/tools/win/supalink/install_s... tools/win/supalink/install_supalink.py:22: out = out.replace('\r', '') On 2011/09/27 23:21:33, Marc-Antoine Ruel wrote: > universal_newlines=True? Done. http://codereview.chromium.org/8059024/diff/4001/tools/win/supalink/install_s... tools/win/supalink/install_supalink.py:33: def main(): On 2011/09/27 23:21:33, Marc-Antoine Ruel wrote: > vcdir = os.environ.get('VCINSTALLDIR') > if not vcdir: Done. http://codereview.chromium.org/8059024/diff/4001/tools/win/supalink/install_s... tools/win/supalink/install_supalink.py:36: os.environ['PATH'] += ';' + os.path.join(vcdir, 'bin') + \ On 2011/09/27 23:21:33, Marc-Antoine Ruel wrote: > use () Done. http://codereview.chromium.org/8059024/diff/4001/tools/win/supalink/install_s... tools/win/supalink/install_supalink.py:48: raise SystemExit('link.exe not found at %s' % link) On 2011/09/27 23:21:33, Marc-Antoine Ruel wrote: > print 'link.exe not found at %s' % link > return 1 Done. http://codereview.chromium.org/8059024/diff/4001/tools/win/supalink/install_s... tools/win/supalink/install_supalink.py:57: raise SystemExit('Wasn\'t able to back up %s to %s.\ On 2011/09/27 23:21:33, Marc-Antoine Ruel wrote: > same Done. http://codereview.chromium.org/8059024/diff/4001/tools/win/supalink/install_s... tools/win/supalink/install_supalink.py:65: rc, out = run_with_vsvars('cl /nologo /Ox /Zi /W4 /WX\ On 2011/09/27 23:21:33, Marc-Antoine Ruel wrote: > same Done. http://codereview.chromium.org/8059024/diff/4001/tools/win/supalink/install_s... tools/win/supalink/install_supalink.py:68: if rc != 0: On 2011/09/27 23:21:33, Marc-Antoine Ruel wrote: > if rc: Done. http://codereview.chromium.org/8059024/diff/4001/tools/win/supalink/install_s... tools/win/supalink/install_supalink.py:70: raise SystemExit('Failed to build supalink.exe') On 2011/09/27 23:21:33, Marc-Antoine Ruel wrote: > same Done. http://codereview.chromium.org/8059024/diff/4001/tools/win/supalink/install_s... tools/win/supalink/install_supalink.py:79: raise SystemExit('Wasn\'t able to copy supalink.exe over %s.\ On 2011/09/27 23:21:33, Marc-Antoine Ruel wrote: > same Done. http://codereview.chromium.org/8059024/diff/4001/tools/win/supalink/install_s... tools/win/supalink/install_supalink.py:83: 'Software\\Chromium\\supalink_installed', On 2011/09/27 23:21:33, Marc-Antoine Ruel wrote: > weird alignment. Done. http://codereview.chromium.org/8059024/diff/4001/tools/win/supalink/install_s... tools/win/supalink/install_supalink.py:87: print 'Linker shim installed. Regenerate via gyp: "gclient runhooks".' On 2011/09/27 23:21:33, Marc-Antoine Ruel wrote: > return 0 Done. http://codereview.chromium.org/8059024/diff/4001/tools/win/supalink/install_s... tools/win/supalink/install_supalink.py:88: On 2011/09/27 23:21:33, Marc-Antoine Ruel wrote: > two lines between file level symbols Done. http://codereview.chromium.org/8059024/diff/4001/tools/win/supalink/install_s... tools/win/supalink/install_supalink.py:90: main() On 2011/09/27 23:21:33, Marc-Antoine Ruel wrote: > sys.exit(main()) Done. http://codereview.chromium.org/8059024/diff/4001/tools/win/supalink/supalink.cpp File tools/win/supalink/supalink.cpp (right): http://codereview.chromium.org/8059024/diff/4001/tools/win/supalink/supalink.... tools/win/supalink/supalink.cpp:15: fprintf(stdout, "supalink fatal error: %s\n", msg); On 2011/09/27 23:21:33, Marc-Antoine Ruel wrote: > why not wprintf() then? Done. http://codereview.chromium.org/8059024/diff/4001/tools/win/supalink/supalink.... tools/win/supalink/supalink.cpp:19: static string ErrorMessageToString(DWORD err) { On 2011/09/27 23:21:33, Marc-Antoine Ruel wrote: > wstring Done. http://codereview.chromium.org/8059024/diff/4001/tools/win/supalink/supalink.... tools/win/supalink/supalink.cpp:20: LPTSTR msg_buf = NULL; On 2011/09/27 23:21:33, Marc-Antoine Ruel wrote: > wchar_t* Done. http://codereview.chromium.org/8059024/diff/4001/tools/win/supalink/supalink.... tools/win/supalink/supalink.cpp:25: (LPTSTR)&msg_buf, On 2011/09/27 23:21:33, Marc-Antoine Ruel wrote: > reinterpret_cast<>() Done. http://codereview.chromium.org/8059024/diff/4001/tools/win/supalink/supalink.... tools/win/supalink/supalink.cpp:34: static void Fallback(const char* msg = 0) { On 2011/09/27 23:21:33, Marc-Antoine Ruel wrote: > wchar_t Done. http://codereview.chromium.org/8059024/diff/4001/tools/win/supalink/supalink.... tools/win/supalink/supalink.cpp:36: fprintf(stdout, On 2011/09/27 23:21:33, Marc-Antoine Ruel wrote: > same Done. http://codereview.chromium.org/8059024/diff/4001/tools/win/supalink/supalink.... tools/win/supalink/supalink.cpp:49: const char* search_for[] = { On 2011/09/27 23:21:33, Marc-Antoine Ruel wrote: > Move to const global Done. http://codereview.chromium.org/8059024/diff/4001/tools/win/supalink/supalink.... tools/win/supalink/supalink.cpp:85: (LPSTR)cmd.c_str(), On 2011/09/27 23:21:33, Marc-Antoine Ruel wrote: > same Done. http://codereview.chromium.org/8059024/diff/4001/tools/win/supalink/supalink.... tools/win/supalink/supalink.cpp:103: wstring SlurpFile(const char* path) { On 2011/09/27 23:21:33, Marc-Antoine Ruel wrote: > so I'll stop here but make the project unicode please. Done.
lgtm
http://codereview.chromium.org/8059024/diff/4001/tools/win/supalink/check_ins... File tools/win/supalink/check_installed.py (right): http://codereview.chromium.org/8059024/diff/4001/tools/win/supalink/check_ins... tools/win/supalink/check_installed.py:1: import sys On 2011/09/28 00:19:38, scottmg wrote: > On 2011/09/27 23:21:33, Marc-Antoine Ruel wrote: > > chmod +x > Is this possible from a Windows git cl? Ah no, don't bother. :) http://codereview.chromium.org/8059024/diff/4002/tools/win/supalink/install_s... File tools/win/supalink/install_supalink.py (right): http://codereview.chromium.org/8059024/diff/4002/tools/win/supalink/install_s... tools/win/supalink/install_supalink.py:17: f = open('temp.bat', 'w') I'd prefer to use tempfile instead of creating a file in the current directory. http://codereview.chromium.org/8059024/diff/4002/tools/win/supalink/install_s... tools/win/supalink/install_supalink.py:31: for line in out.split('\n'): splitlines() http://codereview.chromium.org/8059024/diff/4002/tools/win/supalink/install_s... tools/win/supalink/install_supalink.py:34: raise SystemExit('Couldn\'t get VCINSTALLDIR. Run vsvars32.bat?') I'd remove this line and do the check at line 40. http://codereview.chromium.org/8059024/diff/4002/tools/win/supalink/install_s... tools/win/supalink/install_supalink.py:38: vcdir = os.environ['VCINSTALLDIR'] os.environ.get('VCINSTALLDIR') otherwise it'll throw an exception. http://codereview.chromium.org/8059024/diff/4002/tools/win/supalink/install_s... tools/win/supalink/install_supalink.py:40: vcdir = get_vc_dir() if not vcdir: print 'Couldn\'t get ...' return 1 http://codereview.chromium.org/8059024/diff/4002/tools/win/supalink/install_s... tools/win/supalink/install_supalink.py:62: 'Not running with Administrator privileges?' append ) otherwise it'll fail. http://codereview.chromium.org/8059024/diff/4002/tools/win/supalink/install_s... tools/win/supalink/install_supalink.py:67: if (not os.path.exists('supalink.exe') 'or' at the end of the line http://codereview.chromium.org/8059024/diff/4002/tools/win/supalink/install_s... tools/win/supalink/install_supalink.py:94: print 'Linker shim installed. Regenerate via gyp: "gclient runhooks".' return 0 http://codereview.chromium.org/8059024/diff/4002/tools/win/supalink/install_s... tools/win/supalink/install_supalink.py:98: main() sys.exit(main()) http://codereview.chromium.org/8059024/diff/4002/tools/win/supalink/supalink.cpp File tools/win/supalink/supalink.cpp (right): http://codereview.chromium.org/8059024/diff/4002/tools/win/supalink/supalink.... tools/win/supalink/supalink.cpp:21: FormatMessage(FORMAT_MESSAGE_ALLOCATE_BUFFER | FORMAT_MESSAGE_FROM_SYSTEM, You should still check the error code. http://codereview.chromium.org/8059024/diff/4002/tools/win/supalink/supalink.... tools/win/supalink/supalink.cpp:33: static const wchar_t* g_search_for[] = { that's not const. To be const, it must be: static const wchar_t* const g_search_for[] = { http://codereview.chromium.org/8059024/diff/4002/tools/win/supalink/supalink.... tools/win/supalink/supalink.cpp:46: static void Fallback(const wchar_t* msg = 0) { NULL http://codereview.chromium.org/8059024/diff/4002/tools/win/supalink/supalink.... tools/win/supalink/supalink.cpp:54: STARTUPINFO startup_info = { sizeof(STARTUPINFO) }; then GetStartupInfo() to initialize it correctly. http://codereview.chromium.org/8059024/diff/4002/tools/win/supalink/supalink.... tools/win/supalink/supalink.cpp:58: wstring orig_cmd(GetCommandLine()); I feel like it's be much easier to search on the lower cased version of the line and replace the non modified string. But what about using CommandLineToArgvW()? http://codereview.chromium.org/8059024/diff/4002/tools/win/supalink/supalink.... tools/win/supalink/supalink.cpp:98: CloseHandle(process_info.hThread); you can close it before line 95
http://codereview.chromium.org/8059024/diff/4002/tools/win/supalink/install_s... File tools/win/supalink/install_supalink.py (right): http://codereview.chromium.org/8059024/diff/4002/tools/win/supalink/install_s... tools/win/supalink/install_supalink.py:17: f = open('temp.bat', 'w') On 2011/09/28 00:33:27, Marc-Antoine Ruel wrote: > I'd prefer to use tempfile instead of creating a file in the current directory. Done. http://codereview.chromium.org/8059024/diff/4002/tools/win/supalink/install_s... tools/win/supalink/install_supalink.py:31: for line in out.split('\n'): On 2011/09/28 00:33:27, Marc-Antoine Ruel wrote: > splitlines() Done. http://codereview.chromium.org/8059024/diff/4002/tools/win/supalink/install_s... tools/win/supalink/install_supalink.py:34: raise SystemExit('Couldn\'t get VCINSTALLDIR. Run vsvars32.bat?') On 2011/09/28 00:33:27, Marc-Antoine Ruel wrote: > I'd remove this line and do the check at line 40. Done. http://codereview.chromium.org/8059024/diff/4002/tools/win/supalink/install_s... tools/win/supalink/install_supalink.py:38: vcdir = os.environ['VCINSTALLDIR'] On 2011/09/28 00:33:27, Marc-Antoine Ruel wrote: > os.environ.get('VCINSTALLDIR') > > otherwise it'll throw an exception. Done. http://codereview.chromium.org/8059024/diff/4002/tools/win/supalink/install_s... tools/win/supalink/install_supalink.py:62: 'Not running with Administrator privileges?' On 2011/09/28 00:33:27, Marc-Antoine Ruel wrote: > append ) otherwise it'll fail. Not sure what you mean? http://codereview.chromium.org/8059024/diff/4002/tools/win/supalink/install_s... tools/win/supalink/install_supalink.py:67: if (not os.path.exists('supalink.exe') On 2011/09/28 00:33:27, Marc-Antoine Ruel wrote: > 'or' at the end of the line Done. http://codereview.chromium.org/8059024/diff/4002/tools/win/supalink/install_s... tools/win/supalink/install_supalink.py:94: print 'Linker shim installed. Regenerate via gyp: "gclient runhooks".' On 2011/09/28 00:33:27, Marc-Antoine Ruel wrote: > return 0 Done. http://codereview.chromium.org/8059024/diff/4002/tools/win/supalink/install_s... tools/win/supalink/install_supalink.py:98: main() On 2011/09/28 00:33:27, Marc-Antoine Ruel wrote: > sys.exit(main()) Done. http://codereview.chromium.org/8059024/diff/4002/tools/win/supalink/supalink.cpp File tools/win/supalink/supalink.cpp (right): http://codereview.chromium.org/8059024/diff/4002/tools/win/supalink/supalink.... tools/win/supalink/supalink.cpp:21: FormatMessage(FORMAT_MESSAGE_ALLOCATE_BUFFER | FORMAT_MESSAGE_FROM_SYSTEM, On 2011/09/28 00:33:27, Marc-Antoine Ruel wrote: > You should still check the error code. Done. http://codereview.chromium.org/8059024/diff/4002/tools/win/supalink/supalink.... tools/win/supalink/supalink.cpp:33: static const wchar_t* g_search_for[] = { On 2011/09/28 00:33:27, Marc-Antoine Ruel wrote: > that's not const. To be const, it must be: > > static const wchar_t* const g_search_for[] = { Done. http://codereview.chromium.org/8059024/diff/4002/tools/win/supalink/supalink.... tools/win/supalink/supalink.cpp:46: static void Fallback(const wchar_t* msg = 0) { On 2011/09/28 00:33:27, Marc-Antoine Ruel wrote: > NULL Done. http://codereview.chromium.org/8059024/diff/4002/tools/win/supalink/supalink.... tools/win/supalink/supalink.cpp:54: STARTUPINFO startup_info = { sizeof(STARTUPINFO) }; On 2011/09/28 00:33:27, Marc-Antoine Ruel wrote: > then GetStartupInfo() to initialize it correctly. Done. Why though? The 2 handles are initialized by CreateProcess and that's all that's being used. Er... wait. Undo that. GetStartupInfo gets STARTUPINFO for this process, why do I want that here? http://codereview.chromium.org/8059024/diff/4002/tools/win/supalink/supalink.... tools/win/supalink/supalink.cpp:58: wstring orig_cmd(GetCommandLine()); On 2011/09/28 00:33:27, Marc-Antoine Ruel wrote: > I feel like it's be much easier to search on the lower cased version of the line > and replace the non modified string. Yes, maybe. I would prefer not to rewrite it unless you feel strongly. > But what about using CommandLineToArgvW()? I didn't think that was a good idea because there's no guarantee that lib/link/dumpbin/etc. parse their command lines in the same way as CRT. Even if they do it would also be complex to try to emulate the re-addition of quotes when calling onwards to the subprocess. http://codereview.chromium.org/8059024/diff/4002/tools/win/supalink/supalink.... tools/win/supalink/supalink.cpp:98: CloseHandle(process_info.hThread); On 2011/09/28 00:33:27, Marc-Antoine Ruel wrote: > you can close it before line 95 Done.
> Is Unicode build just general policy or is there something that MBCS code would > have broken on? General policy and it would break linking an executable in a path that cannot be easily represented on the current code page. http://codereview.chromium.org/8059024/diff/4002/tools/win/supalink/install_s... File tools/win/supalink/install_supalink.py (right): http://codereview.chromium.org/8059024/diff/4002/tools/win/supalink/install_s... tools/win/supalink/install_supalink.py:62: 'Not running with Administrator privileges?' On 2011/09/28 15:59:42, scottmg wrote: > On 2011/09/28 00:33:27, Marc-Antoine Ruel wrote: > > append ) otherwise it'll fail. > > Not sure what you mean? % has a higher precedence than string concatenation. So you need: print ('Wasn\'t able to back up %s to %s. ' 'Not running with Administrator privileges?') % (link, link_backup) http://codereview.chromium.org/8059024/diff/4002/tools/win/supalink/supalink.cpp File tools/win/supalink/supalink.cpp (right): http://codereview.chromium.org/8059024/diff/4002/tools/win/supalink/supalink.... tools/win/supalink/supalink.cpp:54: STARTUPINFO startup_info = { sizeof(STARTUPINFO) }; On 2011/09/28 15:59:42, scottmg wrote: > On 2011/09/28 00:33:27, Marc-Antoine Ruel wrote: > > then GetStartupInfo() to initialize it correctly. > > Done. Why though? The 2 handles are initialized by CreateProcess and that's all > that's being used. > > Er... wait. Undo that. GetStartupInfo gets STARTUPINFO for this process, why do > I want that here? So the child process is configured like if the parent process had started it. For example, if someone runs start /min supalink.exe foo bar it'd be nice if supalink.exe didn't screw around. http://codereview.chromium.org/8059024/diff/4002/tools/win/supalink/supalink.... tools/win/supalink/supalink.cpp:58: wstring orig_cmd(GetCommandLine()); On 2011/09/28 15:59:42, scottmg wrote: > On 2011/09/28 00:33:27, Marc-Antoine Ruel wrote: > > I feel like it's be much easier to search on the lower cased version of the > line > > and replace the non modified string. > > Yes, maybe. I would prefer not to rewrite it unless you feel strongly. Link.exe, yes. > > But what about using CommandLineToArgvW()? > > I didn't think that was a good idea because there's no guarantee that > lib/link/dumpbin/etc. parse their command lines in the same way as CRT. Yeah, link.exe uses the msvcr100.dll's version. Fine enough. http://codereview.chromium.org/8059024/diff/10002/tools/win/supalink/install_... File tools/win/supalink/install_supalink.py (right): http://codereview.chromium.org/8059024/diff/10002/tools/win/supalink/install_... tools/win/supalink/install_supalink.py:19: (fd, filename) = tempfile.mkstemp('.bat', text=True) with tempfile.NamedTemporaryFile('w', suffix='.bat') as f: f.write(...) f.flush() p = subprocess.Popen([f.name],... return p.communicate()[0] http://codereview.chromium.org/8059024/diff/10002/tools/win/supalink/install_... tools/win/supalink/install_supalink.py:41: vcdir = os.environ.get('VCINSTALLDIR', None) ", None" is not needed. http://codereview.chromium.org/8059024/diff/10002/tools/win/supalink/install_... tools/win/supalink/install_supalink.py:48: ';' + os.path.join(vcdir, '../Common7/IDE')) \\ instead of / ? http://codereview.chromium.org/8059024/diff/10002/tools/win/supalink/install_... tools/win/supalink/install_supalink.py:77: rc, out = run_with_vsvars('cl /nologo /Ox /Zi /W4 /WX /D_UNICODE /DUNICODE' Sometimes you align +4 sometimes you align at (, it'd be nice to be consistent. http://codereview.chromium.org/8059024/diff/10002/tools/win/supalink/supalink... File tools/win/supalink/supalink.cpp (right): http://codereview.chromium.org/8059024/diff/10002/tools/win/supalink/supalink... tools/win/supalink/supalink.cpp:125: Fatal(L"failed during response rewrite"); alignment
Thanks, hopefully getting closer. http://codereview.chromium.org/8059024/diff/4002/tools/win/supalink/install_s... File tools/win/supalink/install_supalink.py (right): http://codereview.chromium.org/8059024/diff/4002/tools/win/supalink/install_s... tools/win/supalink/install_supalink.py:40: vcdir = get_vc_dir() On 2011/09/28 00:33:27, Marc-Antoine Ruel wrote: > if not vcdir: > print 'Couldn\'t get ...' > return 1 Done. http://codereview.chromium.org/8059024/diff/4002/tools/win/supalink/install_s... tools/win/supalink/install_supalink.py:62: 'Not running with Administrator privileges?' On 2011/09/28 18:17:14, Marc-Antoine Ruel wrote: > On 2011/09/28 15:59:42, scottmg wrote: > > On 2011/09/28 00:33:27, Marc-Antoine Ruel wrote: > > > append ) otherwise it'll fail. > > > > Not sure what you mean? > > % has a higher precedence than string concatenation. So you need: > print ('Wasn\'t able to back up %s to %s. ' > 'Not running with Administrator privileges?') % (link, link_backup) Done. As adjacent string literals behaviour is defined at the lexer level, I don't think it's relevant here though. http://codereview.chromium.org/8059024/diff/4002/tools/win/supalink/supalink.cpp File tools/win/supalink/supalink.cpp (right): http://codereview.chromium.org/8059024/diff/4002/tools/win/supalink/supalink.... tools/win/supalink/supalink.cpp:54: STARTUPINFO startup_info = { sizeof(STARTUPINFO) }; On 2011/09/28 18:17:14, Marc-Antoine Ruel wrote: > On 2011/09/28 15:59:42, scottmg wrote: > > On 2011/09/28 00:33:27, Marc-Antoine Ruel wrote: > > > then GetStartupInfo() to initialize it correctly. > > > > Done. Why though? The 2 handles are initialized by CreateProcess and that's > all > > that's being used. > > > > Er... wait. Undo that. GetStartupInfo gets STARTUPINFO for this process, why > do > > I want that here? > > So the child process is configured like if the parent process had started it. > For example, if someone runs > start /min supalink.exe foo bar > it'd be nice if supalink.exe didn't screw around. Ah, got it, thank you. Done. http://codereview.chromium.org/8059024/diff/4002/tools/win/supalink/supalink.... tools/win/supalink/supalink.cpp:58: wstring orig_cmd(GetCommandLine()); On 2011/09/28 18:17:14, Marc-Antoine Ruel wrote: > On 2011/09/28 15:59:42, scottmg wrote: > > On 2011/09/28 00:33:27, Marc-Antoine Ruel wrote: > > > I feel like it's be much easier to search on the lower cased version of the > > line > > > and replace the non modified string. > > > > Yes, maybe. I would prefer not to rewrite it unless you feel strongly. > > Link.exe, yes. true, Done. > > > > > But what about using CommandLineToArgvW()? > > > > I didn't think that was a good idea because there's no guarantee that > > lib/link/dumpbin/etc. parse their command lines in the same way as CRT. > > Yeah, link.exe uses the msvcr100.dll's version. Fine enough. OK. I still believe it's less error prone not to use CommandLineToArgvW because after replacing argv[0] it's difficult to rejoin it with argv[1..]. http://codereview.chromium.org/8059024/diff/10002/tools/win/supalink/install_... File tools/win/supalink/install_supalink.py (right): http://codereview.chromium.org/8059024/diff/10002/tools/win/supalink/install_... tools/win/supalink/install_supalink.py:19: (fd, filename) = tempfile.mkstemp('.bat', text=True) On 2011/09/28 18:17:14, Marc-Antoine Ruel wrote: > with tempfile.NamedTemporaryFile('w', suffix='.bat') as f: > f.write(...) > f.flush() > p = subprocess.Popen([f.name],... > return p.communicate()[0] That idiom doesn't work on Windows. tempfile.NamedTemporaryFile docs says: Whether the name can be used to open the file a second time, while the named temporary file is still open, varies across platforms (it can be so used on Unix; it cannot on Windows NT or later)... I confirmed that cmd cannot run the batch file when it's still held open by the parent process. http://codereview.chromium.org/8059024/diff/10002/tools/win/supalink/install_... tools/win/supalink/install_supalink.py:41: vcdir = os.environ.get('VCINSTALLDIR', None) On 2011/09/28 18:17:14, Marc-Antoine Ruel wrote: > ", None" is not needed. Done. http://codereview.chromium.org/8059024/diff/10002/tools/win/supalink/install_... tools/win/supalink/install_supalink.py:48: ';' + os.path.join(vcdir, '../Common7/IDE')) On 2011/09/28 18:17:14, Marc-Antoine Ruel wrote: > \\ instead of / ? Done. http://codereview.chromium.org/8059024/diff/10002/tools/win/supalink/install_... tools/win/supalink/install_supalink.py:77: rc, out = run_with_vsvars('cl /nologo /Ox /Zi /W4 /WX /D_UNICODE /DUNICODE' On 2011/09/28 18:17:14, Marc-Antoine Ruel wrote: > Sometimes you align +4 sometimes you align at (, it'd be nice to be consistent. I've tried to make them all at (. http://codereview.chromium.org/8059024/diff/10002/tools/win/supalink/supalink... File tools/win/supalink/supalink.cpp (right): http://codereview.chromium.org/8059024/diff/10002/tools/win/supalink/supalink... tools/win/supalink/supalink.cpp:125: Fatal(L"failed during response rewrite"); On 2011/09/28 18:17:14, Marc-Antoine Ruel wrote: > alignment Done.
On 2011/09/28 21:47:56, scottmg wrote: > Thanks, hopefully getting closer. Pray I don't review you further. > > Sometimes you align +4 sometimes you align at (, it'd be nice to be > consistent. > > I've tried to make them all at (. FYI, the reason I always align at +4 is that it makes refactoring much easier. It's always annoying when the called function name changes and all the arguments needs to be re-aligned. Also, aligning at ( leaves you much less space. lgtm http://codereview.chromium.org/8059024/diff/11002/tools/win/supalink/install_... File tools/win/supalink/install_supalink.py (right): http://codereview.chromium.org/8059024/diff/11002/tools/win/supalink/install_... tools/win/supalink/install_supalink.py:25: p = subprocess.Popen([filename], shell=True, stdout=subprocess.PIPE, try: p = subprocess.Popen([filename], shell=True, stdout=subprocess.PIPE, universal_newlines=True) out, err = p.communicate() return p.returncode, out finally: os.unlink(filename)
On 2011/09/30 13:58:11, Marc-Antoine Ruel wrote: > On 2011/09/28 21:47:56, scottmg wrote: > > Thanks, hopefully getting closer. > > Pray I don't review you further. > :) > > > > Sometimes you align +4 sometimes you align at (, it'd be nice to be > > consistent. > > > > I've tried to make them all at (. > > FYI, the reason I always align at +4 is that it makes refactoring much easier. > It's always annoying when the called function name changes and all the arguments > needs to be re-aligned. Also, aligning at ( leaves you much less space. Agreed, ( is very tiring. I thought that ( was The Rule, but on checking I realize that's for C++ only, and Python is Either-Is-OK. Will use +4 from now on. > lgtm Hurray! > > http://codereview.chromium.org/8059024/diff/11002/tools/win/supalink/install_... > File tools/win/supalink/install_supalink.py (right): > > http://codereview.chromium.org/8059024/diff/11002/tools/win/supalink/install_... > tools/win/supalink/install_supalink.py:25: p = subprocess.Popen([filename], > shell=True, stdout=subprocess.PIPE, > try: > p = subprocess.Popen([filename], shell=True, stdout=subprocess.PIPE, > universal_newlines=True) > out, err = p.communicate() > return p.returncode, out > finally: > os.unlink(filename) Done.
CQ is trying tha patch. Follow status at https://chromium-status.appspot.com/cq/scottmg@chromium.org/8059024/15001
Change committed as 103474
http://codereview.chromium.org/8059024/diff/15001/build/common.gypi File build/common.gypi (right): http://codereview.chromium.org/8059024/diff/15001/build/common.gypi#newcode416 build/common.gypi:416: 'incremental_chrome_dll%': '<!(python <(DEPTH)/tools/win/supalink/check_installed.py)', rlz has a mode where it can be built stand-alone. It maps in build/ and a few other chrome directories and then runs gyp with this file injected. Due to supalink, this fails with newer chrome revisions. I guess I'll just add tools/win/supalink to the list of directories that rlz maps, does that sound reasonable? Or do you expect this file to move around soonish?
On 2012/03/29 22:47:32, Nico wrote: > http://codereview.chromium.org/8059024/diff/15001/build/common.gypi > File build/common.gypi (right): > > http://codereview.chromium.org/8059024/diff/15001/build/common.gypi#newcode416 > build/common.gypi:416: 'incremental_chrome_dll%': '<!(python > <(DEPTH)/tools/win/supalink/check_installed.py)', > rlz has a mode where it can be built stand-alone. It maps in build/ and a few > other chrome directories and then runs gyp with this file injected. Due to > supalink, this fails with newer chrome revisions. I guess I'll just add > tools/win/supalink to the list of directories that rlz maps, does that sound > reasonable? Or do you expect this file to move around soonish? I wasn't planning to move this file. (Or are you suggesting it ought to move? Is build/ not supposed to use tools/ or something?)
> I wasn't planning to move this file. (Or are you suggesting it ought to move? Is > build/ not supposed to use tools/ or something?) It's fine where it is, just wanted to prevent a situation where I change something in rlz and then this moves two weeks later. If you don't plan moving this, all's well :-) |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
