Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2946063002:
Python script to compile and link the integration tests using llvm's clang-cl (Closed)
DescriptionPython script to compile and link the integration tests using llvm's clang-cl
BUG=
Review-Url: https://codereview.chromium.org/2946063002
Committed: https://github.com/google/syzygy/commit/d98b9eeb23efbbfa81fcca245c7ef68d0ec5028c
Patch Set 1 #
Total comments: 21
Patch Set 2 : Merge branch 'syzygy_asan_rtl' into compilation_script #Patch Set 3 : Modification made based on Seb's reviews #
Total comments: 24
Patch Set 4 : Response to reviewer comments. #Patch Set 5 : Response to reviewer comments. #
Total comments: 36
Patch Set 6 : Remove gyp file from banch #Patch Set 7 : Response to reviews #
Total comments: 6
Patch Set 8 : Added -m32 flag to the compialtion method and changed the extenstion of the object files from .o to… #Patch Set 9 : Merge branch 'compilation_script' into gyp_file #Patch Set 10 : Remove the files needed for the gyp file. #Patch Set 11 : Fixed indentation #Patch Set 12 : Response to last set of comments #Patch Set 13 : Remove the integration_tests.gyp from this CL. #
Total comments: 9
Patch Set 14 : Response to reviewer's comments. #Patch Set 15 : Python script to compile and link the integration tests using llvm's clang-cl #
Total comments: 6
Patch Set 16 : Python script to compile and link the integration tests using llvm's clang-cl #
Total comments: 12
Patch Set 17 : Python script to compile and link the integration tests using llvm's clang-cl #
Total comments: 3
Messages
Total messages: 39 (13 generated)
Description was changed from ========== Python script to compile and link the integration tests using llvm's clang-cl Add missing methods to make compiling and linking of 32 bit integration_tests instrumented with LLVM Asan possible. BUG= ========== to ========== Python script to compile and link the integration tests using llvm's clang-cl BUG= ==========
njanevsk@google.com changed reviewers: + etienneb@chromium.org, sebmarchand@chromium.org
This is the script that compiles and links the integration_tests using clang-cl. It depends on https://codereview.chromium.org/2937353002/ I don't know if there is a way to specify that somehow. In Critique you can specify on which CLs the current CL depends.
Yes, there's a way to make it dependent on the other CL, please do it: - make sure that your branch's upstream is set to the branch of the other CL (e.g. if the other branch is called "foo" do: "git br -u foo"). - pull the changes from this branch (do a git "pull") - the "git cl" commands will know mark the other CL as a parent.
https://codereview.chromium.org/2946063002/diff/1/syzygy/integration_tests/ma... File syzygy/integration_tests/make_integration_tests_clang.py (right): https://codereview.chromium.org/2946063002/diff/1/syzygy/integration_tests/ma... syzygy/integration_tests/make_integration_tests_clang.py:15: parser.add_option('--src-dir', You don't need this, you can automatically find it (see https://github.com/google/syzygy/blob/30b171f90991d6332499da77309c8c1a6d93198...) https://codereview.chromium.org/2946063002/diff/1/syzygy/integration_tests/ma... syzygy/integration_tests/make_integration_tests_clang.py:17: parser.add_option('--release-dir', help='Syzygy build directory.') We tend to call this --build-dir https://codereview.chromium.org/2946063002/diff/1/syzygy/integration_tests/ma... syzygy/integration_tests/make_integration_tests_clang.py:18: parser.add_option('--input_files', help='files to be compiled and linked') be consistent, you're using dashes for the other flags so don't use underscores here, use a dash too. https://codereview.chromium.org/2946063002/diff/1/syzygy/integration_tests/ma... syzygy/integration_tests/make_integration_tests_clang.py:29: clang_path = options.src_dir + "/third_party/llvm-build/Release+Asserts/bin/clang-cl.exe" The 80 chars / line rule also apply to Python scrits :), you can use parenthesis for this. You could also move this path to a constant at the top of the file, e.g.: _CLANG_CL_PATH = os.path.join(_SRC_DIR, 'third_party', 'llvm-build', 'Release+Asserts', 'bin', 'clang-cl.exe') Not that in this case _SRC_DIR is also a constant, see my comment for the --src-dir flag. https://codereview.chromium.org/2946063002/diff/1/syzygy/integration_tests/ma... syzygy/integration_tests/make_integration_tests_clang.py:30: compiler_flags = '-c -fsanitize=address -mllvm -asan-instrumentation-with-call-threshold=0 -mllvm -asan-stack=0 -DUNICODE -D_UNICODE -DNOMINMAX -D_CRT_SECURE_NO_WARNINGS -I' + options.src_dir Ooch, having this as a list will make it much easier to read, maintain, e.g.: compiler_flags = [ '-c', '-fsanitize=address', ... _SRC_DIR ] Then to use this as a string you can just do ' '.join(compiler_flags) https://codereview.chromium.org/2946063002/diff/1/syzygy/integration_tests/ma... syzygy/integration_tests/make_integration_tests_clang.py:32: source_files = re.findall(r'\b(\w+.cc)\b', options.input_files) I'd prefer to have something that works for all the source file extensions (cc, cpp, Cpp...), could we just assume that all the source files are C++ source files? https://codereview.chromium.org/2946063002/diff/1/syzygy/integration_tests/ma... syzygy/integration_tests/make_integration_tests_clang.py:33: print subprocess.check_output('"' + clang_path + '" ' + compiler_flags + ' ' + ' '.join(source_files), shell = True) Same thing, use a list: compile_command = [ clang_path ] compile_command.extend(compiler_flags) compile_command.extend(source_files) ret = subprocess.call(compile_command) if ret != 0: # Log a meaningful error https://codereview.chromium.org/2946063002/diff/1/syzygy/integration_tests/ma... syzygy/integration_tests/make_integration_tests_clang.py:34: object_files = [filename[:-3] + ".o" for filename in source_files] Use os.path.splitext to get the name of the file. https://codereview.chromium.org/2946063002/diff/1/syzygy/integration_tests/ma... syzygy/integration_tests/make_integration_tests_clang.py:36: print subprocess.check_output('"' + clang_path + '" ' + '-m32' + ' ' + ' '.join(object_files) + ' -o' + ' ' + options.release_dir + '\integration_tests_dll.dll' + ' /link /dll ' + options.release_dir + '\export_dll.dll.lib ' + options.release_dir + '\syzyasan_rtl.dll.lib' + ' -defaultlib:libcmt', shell = True) Same comments as above. I'd prefer to split this into several functions: - One to do the compile step - One to do the link one.
PTAL https://codereview.chromium.org/2946063002/diff/1/syzygy/integration_tests/ma... File syzygy/integration_tests/make_integration_tests_clang.py (right): https://codereview.chromium.org/2946063002/diff/1/syzygy/integration_tests/ma... syzygy/integration_tests/make_integration_tests_clang.py:15: parser.add_option('--src-dir', On 2017/06/21 15:32:49, Sébastien Marchand wrote: > You don't need this, you can automatically find it (see > https://github.com/google/syzygy/blob/30b171f90991d6332499da77309c8c1a6d93198...) That command gives the path to the script/file. I need the path to the repository directory. GYP provides this. I don't think there will be an easier way in Python to get the src directory. https://codereview.chromium.org/2946063002/diff/1/syzygy/integration_tests/ma... syzygy/integration_tests/make_integration_tests_clang.py:17: parser.add_option('--release-dir', help='Syzygy build directory.') On 2017/06/21 15:32:49, Sébastien Marchand wrote: > We tend to call this --build-dir Done. https://codereview.chromium.org/2946063002/diff/1/syzygy/integration_tests/ma... syzygy/integration_tests/make_integration_tests_clang.py:18: parser.add_option('--input_files', help='files to be compiled and linked') On 2017/06/21 15:32:49, Sébastien Marchand wrote: > be consistent, you're using dashes for the other flags so don't use underscores > here, use a dash too. Done. https://codereview.chromium.org/2946063002/diff/1/syzygy/integration_tests/ma... syzygy/integration_tests/make_integration_tests_clang.py:29: clang_path = options.src_dir + "/third_party/llvm-build/Release+Asserts/bin/clang-cl.exe" On 2017/06/21 15:32:49, Sébastien Marchand wrote: > The 80 chars / line rule also apply to Python scrits :), you can use parenthesis > for this. > > You could also move this path to a constant at the top of the file, e.g.: > _CLANG_CL_PATH = os.path.join(_SRC_DIR, 'third_party', 'llvm-build', > 'Release+Asserts', 'bin', 'clang-cl.exe') > > Not that in this case _SRC_DIR is also a constant, see my comment for the > --src-dir flag. Done. https://codereview.chromium.org/2946063002/diff/1/syzygy/integration_tests/ma... syzygy/integration_tests/make_integration_tests_clang.py:30: compiler_flags = '-c -fsanitize=address -mllvm -asan-instrumentation-with-call-threshold=0 -mllvm -asan-stack=0 -DUNICODE -D_UNICODE -DNOMINMAX -D_CRT_SECURE_NO_WARNINGS -I' + options.src_dir On 2017/06/21 15:32:49, Sébastien Marchand wrote: > Ooch, having this as a list will make it much easier to read, maintain, e.g.: > > compiler_flags = [ > '-c', > '-fsanitize=address', > ... > _SRC_DIR > ] > > Then to use this as a string you can just do ' '.join(compiler_flags) Done. https://codereview.chromium.org/2946063002/diff/1/syzygy/integration_tests/ma... syzygy/integration_tests/make_integration_tests_clang.py:32: source_files = re.findall(r'\b(\w+.cc)\b', options.input_files) On 2017/06/21 15:32:49, Sébastien Marchand wrote: > I'd prefer to have something that works for all the source file extensions (cc, > cpp, Cpp...), could we just assume that all the source files are C++ source > files? Yes we can but in that case I will need to have at least two list variables in the gyp file. One for the source files, one for the header files, and one for the remaining. Or just one for the source files, and one for the rest. If you think is better approach I can do it that way. https://codereview.chromium.org/2946063002/diff/1/syzygy/integration_tests/ma... syzygy/integration_tests/make_integration_tests_clang.py:33: print subprocess.check_output('"' + clang_path + '" ' + compiler_flags + ' ' + ' '.join(source_files), shell = True) On 2017/06/21 15:32:49, Sébastien Marchand wrote: > Same thing, use a list: > > compile_command = [ > clang_path > ] > compile_command.extend(compiler_flags) > compile_command.extend(source_files) > > ret = subprocess.call(compile_command) > if ret != 0: > # Log a meaningful error Done. https://codereview.chromium.org/2946063002/diff/1/syzygy/integration_tests/ma... syzygy/integration_tests/make_integration_tests_clang.py:34: object_files = [filename[:-3] + ".o" for filename in source_files] On 2017/06/21 15:32:49, Sébastien Marchand wrote: > Use os.path.splitext to get the name of the file. Done. https://codereview.chromium.org/2946063002/diff/1/syzygy/integration_tests/ma... syzygy/integration_tests/make_integration_tests_clang.py:36: print subprocess.check_output('"' + clang_path + '" ' + '-m32' + ' ' + ' '.join(object_files) + ' -o' + ' ' + options.release_dir + '\integration_tests_dll.dll' + ' /link /dll ' + options.release_dir + '\export_dll.dll.lib ' + options.release_dir + '\syzyasan_rtl.dll.lib' + ' -defaultlib:libcmt', shell = True) On 2017/06/21 15:32:49, Sébastien Marchand wrote: > Same comments as above. I'd prefer to split this into several functions: > - One to do the compile step > - One to do the link one. Done.
https://codereview.chromium.org/2946063002/diff/1/syzygy/integration_tests/ma... File syzygy/integration_tests/make_integration_tests_clang.py (right): https://codereview.chromium.org/2946063002/diff/1/syzygy/integration_tests/ma... syzygy/integration_tests/make_integration_tests_clang.py:15: parser.add_option('--src-dir', On 2017/06/21 19:26:49, njanevsk wrote: > On 2017/06/21 15:32:49, Sébastien Marchand wrote: > > You don't need this, you can automatically find it (see > > > https://github.com/google/syzygy/blob/30b171f90991d6332499da77309c8c1a6d93198...) > > That command gives the path to the script/file. I need the path to the > repository directory. GYP provides this. I don't think there will be an easier > way in Python to get the src directory. Yes, look at the line following the one I linked to. _SCRIPT_DIR = os.path.dirname(os.path.realpath(__file__)) _SRC_DIR = os.path.abspath(os.path.join(_SCRIPT_DIR, os.pardir)) https://codereview.chromium.org/2946063002/diff/1/syzygy/integration_tests/ma... syzygy/integration_tests/make_integration_tests_clang.py:29: clang_path = options.src_dir + "/third_party/llvm-build/Release+Asserts/bin/clang-cl.exe" On 2017/06/21 15:32:49, Sébastien Marchand wrote: > The 80 chars / line rule also apply to Python scrits :), you can use parenthesis > for this. > > You could also move this path to a constant at the top of the file, e.g.: > _CLANG_CL_PATH = os.path.join(_SRC_DIR, 'third_party', 'llvm-build', > 'Release+Asserts', 'bin', 'clang-cl.exe') > > Not that in this case _SRC_DIR is also a constant, see my comment for the > --src-dir flag. You haven't addressed the second part of this comment. https://codereview.chromium.org/2946063002/diff/1/syzygy/integration_tests/ma... syzygy/integration_tests/make_integration_tests_clang.py:32: source_files = re.findall(r'\b(\w+.cc)\b', options.input_files) On 2017/06/21 19:26:49, njanevsk wrote: > On 2017/06/21 15:32:49, Sébastien Marchand wrote: > > I'd prefer to have something that works for all the source file extensions > (cc, > > cpp, Cpp...), could we just assume that all the source files are C++ source > > files? > > Yes we can but in that case I will need to have at least two list variables in > the gyp file. One for the source files, one for the header files, and one for > the remaining. Or just one for the source files, and one for the rest. > > If you think is better approach I can do it that way. I think that I'll keep this as 2 lists. A lot of the integration_test files aren't needed for the Clang version of integration_tests.dll, e,g, the "bb_entry*" one and the "coverage*" ones. https://codereview.chromium.org/2946063002/diff/40001/syzygy/integration_test... File syzygy/integration_tests/make_integration_tests_clang.py (right): https://codereview.chromium.org/2946063002/diff/40001/syzygy/integration_test... syzygy/integration_tests/make_integration_tests_clang.py:12: def compile(clang_path, source_files, src_dir): Add a comment to describe what this function do. https://codereview.chromium.org/2946063002/diff/40001/syzygy/integration_test... syzygy/integration_tests/make_integration_tests_clang.py:14: '-c', Fix the indentation. https://codereview.chromium.org/2946063002/diff/40001/syzygy/integration_test... syzygy/integration_tests/make_integration_tests_clang.py:24: '-I' + src_dir s/+/,/ and move src_dir to the next line. https://codereview.chromium.org/2946063002/diff/40001/syzygy/integration_test... syzygy/integration_tests/make_integration_tests_clang.py:32: print "Failed compiling the integration_tests using clang-cl" This isn't indented properly. https://codereview.chromium.org/2946063002/diff/40001/syzygy/integration_test... syzygy/integration_tests/make_integration_tests_clang.py:35: object_files = [os.path.splitext(filename)[0] + ".o" for filename in source_files] > 80 chars. https://codereview.chromium.org/2946063002/diff/40001/syzygy/integration_test... syzygy/integration_tests/make_integration_tests_clang.py:39: build_dir + '\integration_tests_dll.dll' Fix indentation, look at the other Python script for reference. https://codereview.chromium.org/2946063002/diff/40001/syzygy/integration_test... syzygy/integration_tests/make_integration_tests_clang.py:51: linker_command.extend(['-m32']) linker_command = [ clang_path, '-m32', ] https://codereview.chromium.org/2946063002/diff/40001/syzygy/integration_test... syzygy/integration_tests/make_integration_tests_clang.py:58: print "Failed to link the integration_tests using clang-cl" Fix indent. https://codereview.chromium.org/2946063002/diff/40001/syzygy/integration_test... syzygy/integration_tests/make_integration_tests_clang.py:76: clang_path = options.src_dir + \ Use parenthesis instead of \, better than that, move this to a constant as suggested in the previous round of comments. https://codereview.chromium.org/2946063002/diff/40001/syzygy/integration_test... syzygy/integration_tests/make_integration_tests_clang.py:80: compile(clang_path, source_files, options.src_dir) Check the return value, don't try to link if the compile step failed. https://codereview.chromium.org/2946063002/diff/40001/syzygy/integration_test... syzygy/integration_tests/make_integration_tests_clang.py:81: link(clang_path, source_files, options.build_dir) Ditto.
PTAL! Somehow I added the integration_tests.gyp file to this cl. I couldn't figure out how to revert it. Is it OK to leave it in this CL or do you know how to remove it from the CL? https://codereview.chromium.org/2946063002/diff/40001/syzygy/integration_test... File syzygy/integration_tests/make_integration_tests_clang.py (right): https://codereview.chromium.org/2946063002/diff/40001/syzygy/integration_test... syzygy/integration_tests/make_integration_tests_clang.py:12: def compile(clang_path, source_files, src_dir): On 2017/06/21 19:51:37, Sébastien Marchand wrote: > Add a comment to describe what this function do. Done. https://codereview.chromium.org/2946063002/diff/40001/syzygy/integration_test... syzygy/integration_tests/make_integration_tests_clang.py:14: '-c', On 2017/06/21 19:51:36, Sébastien Marchand wrote: > Fix the indentation. Done. https://codereview.chromium.org/2946063002/diff/40001/syzygy/integration_test... syzygy/integration_tests/make_integration_tests_clang.py:24: '-I' + src_dir On 2017/06/21 19:51:36, Sébastien Marchand wrote: > s/+/,/ and move src_dir to the next line. Why? Cause this is a single flag. include the src directory when searching for headers. https://codereview.chromium.org/2946063002/diff/40001/syzygy/integration_test... syzygy/integration_tests/make_integration_tests_clang.py:32: print "Failed compiling the integration_tests using clang-cl" On 2017/06/21 19:51:37, Sébastien Marchand wrote: > This isn't indented properly. Done. https://codereview.chromium.org/2946063002/diff/40001/syzygy/integration_test... syzygy/integration_tests/make_integration_tests_clang.py:35: object_files = [os.path.splitext(filename)[0] + ".o" for filename in source_files] On 2017/06/21 19:51:36, Sébastien Marchand wrote: > > 80 chars. Done. https://codereview.chromium.org/2946063002/diff/40001/syzygy/integration_test... syzygy/integration_tests/make_integration_tests_clang.py:39: build_dir + '\integration_tests_dll.dll' On 2017/06/21 19:51:36, Sébastien Marchand wrote: > Fix indentation, look at the other Python script for reference. Done. https://codereview.chromium.org/2946063002/diff/40001/syzygy/integration_test... syzygy/integration_tests/make_integration_tests_clang.py:51: linker_command.extend(['-m32']) On 2017/06/21 19:51:36, Sébastien Marchand wrote: > linker_command = [ > clang_path, > '-m32', > ] Done. https://codereview.chromium.org/2946063002/diff/40001/syzygy/integration_test... syzygy/integration_tests/make_integration_tests_clang.py:58: print "Failed to link the integration_tests using clang-cl" On 2017/06/21 19:51:36, Sébastien Marchand wrote: > Fix indent. Done. https://codereview.chromium.org/2946063002/diff/40001/syzygy/integration_test... syzygy/integration_tests/make_integration_tests_clang.py:76: clang_path = options.src_dir + \ On 2017/06/21 19:51:37, Sébastien Marchand wrote: > Use parenthesis instead of \, better than that, move this to a constant as > suggested in the previous round of comments. Done. https://codereview.chromium.org/2946063002/diff/40001/syzygy/integration_test... syzygy/integration_tests/make_integration_tests_clang.py:80: compile(clang_path, source_files, options.src_dir) On 2017/06/21 19:51:36, Sébastien Marchand wrote: > Check the return value, don't try to link if the compile step failed. Done. https://codereview.chromium.org/2946063002/diff/40001/syzygy/integration_test... syzygy/integration_tests/make_integration_tests_clang.py:81: link(clang_path, source_files, options.build_dir) On 2017/06/21 19:51:36, Sébastien Marchand wrote: > Ditto. There is no need to check the return value here. Instead, I do this step only if it compiled successfully. Otherwise I print an error message.
You can easily split this in 2 CLs: - Switch to your branch for the Gyp change; - Do "git co branch_with_script syzygy/integration_tests/integration_tests.gyp", this will fetch this copy of integration_tests.gyp and copy it to your branch. - Do a Git commit. - Switch back to the branch for this CL, do a "git co origin/master syzygy/integration_tests/integration_tests.gyp" This isn't critical but it's good if you know how to do this.
Thanks for the command. It worked like a charm. PTAL.
Another handful of comments :) https://codereview.chromium.org/2946063002/diff/40001/syzygy/integration_test... File syzygy/integration_tests/make_integration_tests_clang.py (right): https://codereview.chromium.org/2946063002/diff/40001/syzygy/integration_test... syzygy/integration_tests/make_integration_tests_clang.py:24: '-I' + src_dir On 2017/06/22 18:43:49, njanevsk wrote: > On 2017/06/21 19:51:36, Sébastien Marchand wrote: > > s/+/,/ and move src_dir to the next line. > > Why? Cause this is a single flag. include the src directory when searching for > headers. Yes but this will also work with a comma, and here you're technically creating a temporary string, so this is less efficient than doing "'I', src_dir," https://codereview.chromium.org/2946063002/diff/40001/syzygy/integration_test... syzygy/integration_tests/make_integration_tests_clang.py:81: link(clang_path, source_files, options.build_dir) On 2017/06/22 18:43:49, njanevsk wrote: > On 2017/06/21 19:51:36, Sébastien Marchand wrote: > > Ditto. > > There is no need to check the return value here. Instead, I do this step only if > it compiled successfully. Otherwise I print an error message. We still want the script to return something != 0 on error. https://codereview.chromium.org/2946063002/diff/80001/syzygy/integration_test... File syzygy/integration_tests/make_integration_tests_clang.py (right): https://codereview.chromium.org/2946063002/diff/80001/syzygy/integration_test... syzygy/integration_tests/make_integration_tests_clang.py:11: Add an extra BL here. https://codereview.chromium.org/2946063002/diff/80001/syzygy/integration_test... syzygy/integration_tests/make_integration_tests_clang.py:13: _SRC_DIR = os.path.abspath(os.path.join(os.path.join(_SCRIPT_DIR, os.pardir), you can do os.path.abspath(os.path.join(_SCRIPT_DIR, os.pardir, os.pardir)) , no need for the nested join as it can take any number of args. https://codereview.chromium.org/2946063002/diff/80001/syzygy/integration_test... syzygy/integration_tests/make_integration_tests_clang.py:14: os.pardir)) Indent at +4. https://codereview.chromium.org/2946063002/diff/80001/syzygy/integration_test... syzygy/integration_tests/make_integration_tests_clang.py:15: _CLANG_PATH = (_SRC_DIR, I'd call this _CLANG_CL_PATH https://codereview.chromium.org/2946063002/diff/80001/syzygy/integration_test... syzygy/integration_tests/make_integration_tests_clang.py:17: Add an extra BL here. https://codereview.chromium.org/2946063002/diff/80001/syzygy/integration_test... syzygy/integration_tests/make_integration_tests_clang.py:19: """ Only compiles the integration tests but does not link. Remove the space before "Only", also I'll just say "Compiles with Clang", this isn't specific to the integration_tests. Maybe call this compile_with_asan as the Asan flags are used? https://codereview.chromium.org/2946063002/diff/80001/syzygy/integration_test... syzygy/integration_tests/make_integration_tests_clang.py:20: Remove one of these 2 BLs. https://codereview.chromium.org/2946063002/diff/80001/syzygy/integration_test... syzygy/integration_tests/make_integration_tests_clang.py:30: compiler_flags = ['-c', Nit, I prefer this form: foo = [ 'bar', 'baz', ] i.e., no value one the first line, and the closing bracket on its own line. https://codereview.chromium.org/2946063002/diff/80001/syzygy/integration_test... syzygy/integration_tests/make_integration_tests_clang.py:53: Remove one of these BLs. https://codereview.chromium.org/2946063002/diff/80001/syzygy/integration_test... syzygy/integration_tests/make_integration_tests_clang.py:64: ".o" for filename in source_files]) Indent +2. When you continue a line over the next one you should have a 4 spaces indentation. https://codereview.chromium.org/2946063002/diff/80001/syzygy/integration_test... syzygy/integration_tests/make_integration_tests_clang.py:67: (build_dir, Why the parenthesis? Should you use os.path.join here? https://codereview.chromium.org/2946063002/diff/80001/syzygy/integration_test... syzygy/integration_tests/make_integration_tests_clang.py:97: source_files = re.findall(r'\b(\w+.cc)\b', options.input_files) using a Regex is overkill, you can use glob: https://docs.python.org/2/library/glob.html https://codereview.chromium.org/2946063002/diff/80001/syzygy/integration_test... syzygy/integration_tests/make_integration_tests_clang.py:103: print "Skipping link step." You should also return something !0 on failure so Gyp will know if it has failed or not (in this case it'll consider that it always works even if it fails because the return value is always 0).
https://codereview.chromium.org/2946063002/diff/80001/syzygy/integration_test... File syzygy/integration_tests/make_integration_tests_clang.py (right): https://codereview.chromium.org/2946063002/diff/80001/syzygy/integration_test... syzygy/integration_tests/make_integration_tests_clang.py:4: # found in the LICENSE file. Add a comment to describe what this script does, e.g.: https://github.com/google/syzygy/blob/master/syzygy/scripts/asan/minidump_sym... https://codereview.chromium.org/2946063002/diff/80001/syzygy/integration_test... syzygy/integration_tests/make_integration_tests_clang.py:5: Add a BL here. Basically each "section" is separated by 2 BLs. https://codereview.chromium.org/2946063002/diff/80001/syzygy/integration_test... syzygy/integration_tests/make_integration_tests_clang.py:50: 2 BLs between each function :) https://codereview.chromium.org/2946063002/diff/80001/syzygy/integration_test... syzygy/integration_tests/make_integration_tests_clang.py:87: parser.add_option('--build-dir', help='Syzygy build directory.') help="Path to the build directory"? There's a directory called "build" in the Syzygy repo. https://codereview.chromium.org/2946063002/diff/80001/syzygy/integration_test... syzygy/integration_tests/make_integration_tests_clang.py:97: source_files = re.findall(r'\b(\w+.cc)\b', options.input_files) On 2017/06/22 19:34:17, Sébastien Marchand wrote: > using a Regex is overkill, you can use glob: > https://docs.python.org/2/library/glob.html And as discussed in a previous patchset we should have 2 lists in the gyp file: - the source files that should be compiled with Clang. - the other ones.
PTAL https://codereview.chromium.org/2946063002/diff/80001/syzygy/integration_test... File syzygy/integration_tests/make_integration_tests_clang.py (right): https://codereview.chromium.org/2946063002/diff/80001/syzygy/integration_test... syzygy/integration_tests/make_integration_tests_clang.py:4: # found in the LICENSE file. On 2017/06/22 19:44:48, Sébastien Marchand wrote: > Add a comment to describe what this script does, e.g.: > https://github.com/google/syzygy/blob/master/syzygy/scripts/asan/minidump_sym... Done. https://codereview.chromium.org/2946063002/diff/80001/syzygy/integration_test... syzygy/integration_tests/make_integration_tests_clang.py:5: On 2017/06/22 19:44:48, Sébastien Marchand wrote: > Add a BL here. > > Basically each "section" is separated by 2 BLs. Done. https://codereview.chromium.org/2946063002/diff/80001/syzygy/integration_test... syzygy/integration_tests/make_integration_tests_clang.py:11: On 2017/06/22 19:34:17, Sébastien Marchand wrote: > Add an extra BL here. Done. https://codereview.chromium.org/2946063002/diff/80001/syzygy/integration_test... syzygy/integration_tests/make_integration_tests_clang.py:13: _SRC_DIR = os.path.abspath(os.path.join(os.path.join(_SCRIPT_DIR, os.pardir), On 2017/06/22 19:34:17, Sébastien Marchand wrote: > you can do os.path.abspath(os.path.join(_SCRIPT_DIR, os.pardir, os.pardir)) , no > need for the nested join as it can take any number of args. Done. https://codereview.chromium.org/2946063002/diff/80001/syzygy/integration_test... syzygy/integration_tests/make_integration_tests_clang.py:14: os.pardir)) On 2017/06/22 19:34:17, Sébastien Marchand wrote: > Indent at +4. Done. https://codereview.chromium.org/2946063002/diff/80001/syzygy/integration_test... syzygy/integration_tests/make_integration_tests_clang.py:15: _CLANG_PATH = (_SRC_DIR, On 2017/06/22 19:34:16, Sébastien Marchand wrote: > I'd call this _CLANG_CL_PATH Done. https://codereview.chromium.org/2946063002/diff/80001/syzygy/integration_test... syzygy/integration_tests/make_integration_tests_clang.py:17: On 2017/06/22 19:34:17, Sébastien Marchand wrote: > Add an extra BL here. Done. https://codereview.chromium.org/2946063002/diff/80001/syzygy/integration_test... syzygy/integration_tests/make_integration_tests_clang.py:19: """ Only compiles the integration tests but does not link. On 2017/06/22 19:34:17, Sébastien Marchand wrote: > Remove the space before "Only", also I'll just say "Compiles with Clang", this > isn't specific to the integration_tests. > > Maybe call this compile_with_asan as the Asan flags are used? Done. https://codereview.chromium.org/2946063002/diff/80001/syzygy/integration_test... syzygy/integration_tests/make_integration_tests_clang.py:20: On 2017/06/22 19:34:17, Sébastien Marchand wrote: > Remove one of these 2 BLs. Done. https://codereview.chromium.org/2946063002/diff/80001/syzygy/integration_test... syzygy/integration_tests/make_integration_tests_clang.py:30: compiler_flags = ['-c', On 2017/06/22 19:34:17, Sébastien Marchand wrote: > Nit, I prefer this form: > foo = [ > 'bar', > 'baz', > ] > > i.e., no value one the first line, and the closing bracket on its own line. Done. https://codereview.chromium.org/2946063002/diff/80001/syzygy/integration_test... syzygy/integration_tests/make_integration_tests_clang.py:50: On 2017/06/22 19:44:48, Sébastien Marchand wrote: > 2 BLs between each function :) Done. https://codereview.chromium.org/2946063002/diff/80001/syzygy/integration_test... syzygy/integration_tests/make_integration_tests_clang.py:53: On 2017/06/22 19:34:17, Sébastien Marchand wrote: > Remove one of these BLs. Done. https://codereview.chromium.org/2946063002/diff/80001/syzygy/integration_test... syzygy/integration_tests/make_integration_tests_clang.py:64: ".o" for filename in source_files]) On 2017/06/22 19:34:17, Sébastien Marchand wrote: > Indent +2. When you continue a line over the next one you should have a 4 spaces > indentation. Done. https://codereview.chromium.org/2946063002/diff/80001/syzygy/integration_test... syzygy/integration_tests/make_integration_tests_clang.py:67: (build_dir, On 2017/06/22 19:34:17, Sébastien Marchand wrote: > Why the parenthesis? Should you use os.path.join here? To join the two. I tried os.path.join but I get an error when running the script. I think it is not joining them correctly cause build_dir has ../.. in it. https://codereview.chromium.org/2946063002/diff/80001/syzygy/integration_test... syzygy/integration_tests/make_integration_tests_clang.py:87: parser.add_option('--build-dir', help='Syzygy build directory.') On 2017/06/22 19:44:48, Sébastien Marchand wrote: > help="Path to the build directory"? There's a directory called "build" in the > Syzygy repo. To the Release directory not the build directory. Updated https://codereview.chromium.org/2946063002/diff/80001/syzygy/integration_test... syzygy/integration_tests/make_integration_tests_clang.py:103: print "Skipping link step." On 2017/06/22 19:34:17, Sébastien Marchand wrote: > You should also return something !0 on failure so Gyp will know if it has failed > or not (in this case it'll consider that it always works even if it fails > because the return value is always 0). Done.
Almost there! https://codereview.chromium.org/2946063002/diff/80001/syzygy/integration_test... File syzygy/integration_tests/make_integration_tests_clang.py (right): https://codereview.chromium.org/2946063002/diff/80001/syzygy/integration_test... syzygy/integration_tests/make_integration_tests_clang.py:67: (build_dir, On 2017/06/22 21:16:33, njanevsk wrote: > On 2017/06/22 19:34:17, Sébastien Marchand wrote: > > Why the parenthesis? Should you use os.path.join here? > > To join the two. I tried os.path.join but I get an error when running the > script. I think it is not joining them correctly cause build_dir has ../.. in > it. os.path.join handles '..', you should use it. https://codereview.chromium.org/2946063002/diff/80001/syzygy/integration_test... syzygy/integration_tests/make_integration_tests_clang.py:87: parser.add_option('--build-dir', help='Syzygy build directory.') On 2017/06/22 21:16:32, njanevsk wrote: > On 2017/06/22 19:44:48, Sébastien Marchand wrote: > > help="Path to the build directory"? There's a directory called "build" in the > > Syzygy repo. > > To the Release directory not the build directory. Updated You also want this to work in debug :), let's just call this --output-dir, "Output directory where the compiled binary should be written." ? https://codereview.chromium.org/2946063002/diff/120001/syzygy/integration_tes... File syzygy/integration_tests/make_integration_tests_clang.py (right): https://codereview.chromium.org/2946063002/diff/120001/syzygy/integration_tes... syzygy/integration_tests/make_integration_tests_clang.py:5: """ A script to compile the integration tests with llvm's clang, Remove the space before the 'A' https://codereview.chromium.org/2946063002/diff/120001/syzygy/integration_tes... syzygy/integration_tests/make_integration_tests_clang.py:53: ret = subprocess.call(compile_command) The object files seems to be produced next to their corresponding .cc file? Please don't do this, they should probably go in os.path.join(build_dir, 'obj', os.path.rel_path(file, _SRC_DIR), 'integration_tests_dll_clang.%s.obj' % os.path.basename(file)). E.g. in Relase syzygy/integration_tests/asan_interceptors_tests.cc will be compiled to out/Release/obj/syzygy/integration_tests/integration_tests_dll_clang.asan_interceptors_tests.obj https://codereview.chromium.org/2946063002/diff/120001/syzygy/integration_tes... syzygy/integration_tests/make_integration_tests_clang.py:74: compiler_controlled_linker_flags = [ this name is confusing, why not just call it linker_flags? https://codereview.chromium.org/2946063002/diff/120001/syzygy/integration_tes... syzygy/integration_tests/make_integration_tests_clang.py:109: parser.error('--input-files is required') Could you add a "target-name" flag and use it everywhere instead of hardcoding "integration_tests_clang_dll"? https://codereview.chromium.org/2946063002/diff/120001/syzygy/integration_tes... syzygy/integration_tests/make_integration_tests_clang.py:117: print "Compilation of integration_tests_clang_dll failed." Add "ERROR" at the beginning of this line, so it'll be easier to spot it in the logs. https://codereview.chromium.org/2946063002/diff/120001/syzygy/integration_tes... syzygy/integration_tests/make_integration_tests_clang.py:118: print "Skipping link step." Print these 2 messages on the same line.
Patchset #13 (id:240001) has been deleted
Thanks for your helping me understand splitting branches in git. PTAL!
Thanks for your helping me understand splitting branches in git. PTAL!
Almost there! https://codereview.chromium.org/2946063002/diff/260001/syzygy/integration_tes... File syzygy/integration_tests/make_integration_tests_clang.py (right): https://codereview.chromium.org/2946063002/diff/260001/syzygy/integration_tes... syzygy/integration_tests/make_integration_tests_clang.py:20: "third_party", "llvm-build", "Release+Asserts", "bin", "clang-cl.exe") Use ' rather than ". Both works but we tend to prefer '. https://codereview.chromium.org/2946063002/diff/260001/syzygy/integration_tes... syzygy/integration_tests/make_integration_tests_clang.py:58: command_len = len(compile_command) Add a new variable called compile_command_base (or something similar) and in the loop create and use compile_command based on compile_command_base. https://codereview.chromium.org/2946063002/diff/260001/syzygy/integration_tes... syzygy/integration_tests/make_integration_tests_clang.py:65: print ("ERROR: Failed compiling %s using clang-cl" % target_name) The parenthesis aren't needed, use ' rather than ". https://codereview.chromium.org/2946063002/diff/260001/syzygy/integration_tes... syzygy/integration_tests/make_integration_tests_clang.py:100: print ("ERROR: Failed to link %s using clang-cl" % target_name) Ditto, no parenthesis and s/"/'/ https://codereview.chromium.org/2946063002/diff/260001/syzygy/integration_tes... syzygy/integration_tests/make_integration_tests_clang.py:108: parser.add_option('--input-files', help='files to be compiled and linked') Use full sentences here, starting with an uppercase letter and finishing with a period. https://codereview.chromium.org/2946063002/diff/260001/syzygy/integration_tes... syzygy/integration_tests/make_integration_tests_clang.py:109: parser.add_option('--target-name', help='name of the target to be compiled') Ditto. https://codereview.chromium.org/2946063002/diff/260001/syzygy/integration_tes... syzygy/integration_tests/make_integration_tests_clang.py:114: parser.error('--output-dir is required') Ditto, add a period (here and below). https://codereview.chromium.org/2946063002/diff/260001/syzygy/integration_tes... syzygy/integration_tests/make_integration_tests_clang.py:125: target_name + '.%s.obj' % Don't mix + and %, you could do: "%s.%s.obj' % (target_name, os.path.splitext(os.path.basename(source_file))[0]) https://codereview.chromium.org/2946063002/diff/260001/syzygy/integration_tes... syzygy/integration_tests/make_integration_tests_clang.py:143: print ("ERROR: Compilation of %s failed, skipping link step." Use ' rather than ".
The CQ bit was checked by njanevsk@google.com to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was checked by njanevsk@google.com to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was unchecked by njanevsk@google.com
The dry run passed. PTAL!
The dry run passed. PTAL!
Looking pretty good. Chris, do you want to take a quick look at this? https://codereview.chromium.org/2946063002/diff/300001/syzygy/integration_tes... File syzygy/integration_tests/make_integration_tests_clang.py (right): https://codereview.chromium.org/2946063002/diff/300001/syzygy/integration_tes... syzygy/integration_tests/make_integration_tests_clang.py:57: compile_command = compile_command_base You could use the "compile_command = [...]" syntax here. https://codereview.chromium.org/2946063002/diff/300001/syzygy/integration_tes... syzygy/integration_tests/make_integration_tests_clang.py:64: print 'ERROR: Failed compiling %s using clang-cl' % target_name Missing period at the end of this message. https://codereview.chromium.org/2946063002/diff/300001/syzygy/integration_tes... syzygy/integration_tests/make_integration_tests_clang.py:98: print 'ERROR: Failed to link %s using clang-cl' % target_name Missing period at the end of this message.
PTAL https://codereview.chromium.org/2946063002/diff/300001/syzygy/integration_tes... File syzygy/integration_tests/make_integration_tests_clang.py (right): https://codereview.chromium.org/2946063002/diff/300001/syzygy/integration_tes... syzygy/integration_tests/make_integration_tests_clang.py:57: compile_command = compile_command_base On 2017/07/06 19:29:57, Sébastien Marchand wrote: > You could use the "compile_command = [...]" syntax here. Done. https://codereview.chromium.org/2946063002/diff/300001/syzygy/integration_tes... syzygy/integration_tests/make_integration_tests_clang.py:64: print 'ERROR: Failed compiling %s using clang-cl' % target_name On 2017/07/06 19:29:57, Sébastien Marchand wrote: > Missing period at the end of this message. Done. https://codereview.chromium.org/2946063002/diff/300001/syzygy/integration_tes... syzygy/integration_tests/make_integration_tests_clang.py:98: print 'ERROR: Failed to link %s using clang-cl' % target_name On 2017/07/06 19:29:57, Sébastien Marchand wrote: > Missing period at the end of this message. Done.
A few more minor nits, looking good otherwise. https://codereview.chromium.org/2946063002/diff/320001/syzygy/integration_tes... File syzygy/integration_tests/make_integration_tests_clang.py (right): https://codereview.chromium.org/2946063002/diff/320001/syzygy/integration_tes... syzygy/integration_tests/make_integration_tests_clang.py:21: Add another BL here. https://codereview.chromium.org/2946063002/diff/320001/syzygy/integration_tes... syzygy/integration_tests/make_integration_tests_clang.py:24: """Only compiles with clang but does not link. Drop the "Only", here and below (and update the sentence below a little bit, "Compiles the files and instrument them with LLVM's ASAN. The linking is done..." https://codereview.chromium.org/2946063002/diff/320001/syzygy/integration_tes... syzygy/integration_tests/make_integration_tests_clang.py:26: Only compiles the files and isntruments them with LLVM's Asan but does not instruments, not isntruments :) https://codereview.chromium.org/2946063002/diff/320001/syzygy/integration_tes... syzygy/integration_tests/make_integration_tests_clang.py:32: src_dir: The repository where the syzgy src is located. s/syzgy/syzygy/ https://codereview.chromium.org/2946063002/diff/320001/syzygy/integration_tes... syzygy/integration_tests/make_integration_tests_clang.py:64: Add a BL https://codereview.chromium.org/2946063002/diff/320001/syzygy/integration_tes... syzygy/integration_tests/make_integration_tests_clang.py:67: Remove one of these BLs
PTAL https://codereview.chromium.org/2946063002/diff/320001/syzygy/integration_tes... File syzygy/integration_tests/make_integration_tests_clang.py (right): https://codereview.chromium.org/2946063002/diff/320001/syzygy/integration_tes... syzygy/integration_tests/make_integration_tests_clang.py:21: On 2017/07/06 20:28:58, Sébastien Marchand wrote: > Add another BL here. Done. https://codereview.chromium.org/2946063002/diff/320001/syzygy/integration_tes... syzygy/integration_tests/make_integration_tests_clang.py:24: """Only compiles with clang but does not link. On 2017/07/06 20:28:57, Sébastien Marchand wrote: > Drop the "Only", here and below (and update the sentence below a little bit, > "Compiles the files and instrument them with LLVM's ASAN. The linking is > done..." Done. https://codereview.chromium.org/2946063002/diff/320001/syzygy/integration_tes... syzygy/integration_tests/make_integration_tests_clang.py:26: Only compiles the files and isntruments them with LLVM's Asan but does not On 2017/07/06 20:28:57, Sébastien Marchand wrote: > instruments, not isntruments :) Done. https://codereview.chromium.org/2946063002/diff/320001/syzygy/integration_tes... syzygy/integration_tests/make_integration_tests_clang.py:32: src_dir: The repository where the syzgy src is located. On 2017/07/06 20:28:58, Sébastien Marchand wrote: > s/syzgy/syzygy/ Done. https://codereview.chromium.org/2946063002/diff/320001/syzygy/integration_tes... syzygy/integration_tests/make_integration_tests_clang.py:64: On 2017/07/06 20:28:57, Sébastien Marchand wrote: > Add a BL Done. https://codereview.chromium.org/2946063002/diff/320001/syzygy/integration_tes... syzygy/integration_tests/make_integration_tests_clang.py:67: On 2017/07/06 20:28:58, Sébastien Marchand wrote: > Remove one of these BLs Done.
LGTM!
The CQ bit was checked by njanevsk@google.com
CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 340001, "attempt_start_ts": 1499438465437440,
"parent_rev": "3ccd5606a4ef239e359003aaba9e9f77967cdb79", "commit_rev":
"d98b9eeb23efbbfa81fcca245c7ef68d0ec5028c"}
Message was sent while issue was closed.
Description was changed from ========== Python script to compile and link the integration tests using llvm's clang-cl BUG= ========== to ========== Python script to compile and link the integration tests using llvm's clang-cl BUG= Review-Url: https://codereview.chromium.org/2946063002 Committed: https://github.com/google/syzygy/commit/d98b9eeb23efbbfa81fcca245c7ef68d0ec5028c ==========
Message was sent while issue was closed.
Committed patchset #17 (id:340001) as https://github.com/google/syzygy/commit/d98b9eeb23efbbfa81fcca245c7ef68d0ec5028c
Message was sent while issue was closed.
some nits https://codereview.chromium.org/2946063002/diff/340001/syzygy/integration_tes... File syzygy/integration_tests/make_integration_tests_clang.py (right): https://codereview.chromium.org/2946063002/diff/340001/syzygy/integration_tes... syzygy/integration_tests/make_integration_tests_clang.py:86: build_dir + '\export_dll.dll.lib', nit: please use 'os.path.join' https://codereview.chromium.org/2946063002/diff/340001/syzygy/integration_tes... syzygy/integration_tests/make_integration_tests_clang.py:87: build_dir + '\syzyasan_rtl.dll.lib', nit: please use 'os.path.join' https://codereview.chromium.org/2946063002/diff/340001/syzygy/integration_tes... syzygy/integration_tests/make_integration_tests_clang.py:119: return os.path.join(output_dir, 'obj', os.path.split and [0] is complicated to read. You can use dirname and basename see: >>> import os >>> os.path.split("c:\\src\\dummy") ('c:\\src', 'dummy') >>> os.path.split("c:\\src\\dummy\\test.dll") ('c:\\src\\dummy', 'test.dll') >>> os.path.split("c:\\src\\dummy") ('c:\\src', 'dummy') >>> os.path.basename("c:\\src\\dummy\\test.dll") 'test.dll'
Message was sent while issue was closed.
On 2017/07/10 14:55:40, etienneb wrote: > some nits > > https://codereview.chromium.org/2946063002/diff/340001/syzygy/integration_tes... > File syzygy/integration_tests/make_integration_tests_clang.py (right): > > https://codereview.chromium.org/2946063002/diff/340001/syzygy/integration_tes... > syzygy/integration_tests/make_integration_tests_clang.py:86: build_dir + > '\export_dll.dll.lib', > nit: please use 'os.path.join' > > https://codereview.chromium.org/2946063002/diff/340001/syzygy/integration_tes... > syzygy/integration_tests/make_integration_tests_clang.py:87: build_dir + > '\syzyasan_rtl.dll.lib', > nit: please use 'os.path.join' > > https://codereview.chromium.org/2946063002/diff/340001/syzygy/integration_tes... > syzygy/integration_tests/make_integration_tests_clang.py:119: return > os.path.join(output_dir, 'obj', > os.path.split and [0] is complicated to read. > You can use dirname and basename > > see: > > >>> import os > >>> os.path.split("c:\\src\\dummy") > ('c:\\src', 'dummy') > >>> os.path.split("c:\\src\\dummy\\test.dll") > ('c:\\src\\dummy', 'test.dll') > >>> os.path.split("c:\\src\\dummy") > ('c:\\src', 'dummy') > >>> os.path.basename("c:\\src\\dummy\\test.dll") > 'test.dll' Thanks for the feedback. I addressed it here: https://codereview.chromium.org/2977433002/ |
