Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2946083002:
Moved source files into a list variable and add a custom build target to compile the integration te… (Closed)
DescriptionMoved source files into a list variable and add a custom build target to compile the integration tests using llvm
BUG=
Depends on:
Code Review:https://codereview.chromium.org/2937353002
and
https://codereview.chromium.org/2946063002/
Review-Url: https://codereview.chromium.org/2946083002
Committed: https://github.com/google/syzygy/commit/db08dd13dcf021804dfc4aa50c132bf335b269de
Patch Set 1 #Patch Set 2 : Updated dependency on another CL #Patch Set 3 : Changed argument names passed to the compilation script. #Patch Set 4 : Merge branch 'compilation_script' into gyp_file #Patch Set 5 : Rebased the CL. #Patch Set 6 : Rebased the CL. #Patch Set 7 : Updated to work with the updated compialtion script #
Total comments: 3
Patch Set 8 : Added flag to indicate the compiler for which the test cases are supported #Patch Set 9 : Added flag to indicate the compiler for which the test cases are supported #Patch Set 10 : Split the test table into Asan and non-Asan tests. #
Total comments: 3
Patch Set 11 : Included the deffered_free_tests when clang is used. #
Total comments: 8
Patch Set 12 : Add a custom action target for compiling the integration tests with clang. #Patch Set 13 : Add a custom action target for compiling the integration tests with clang. #
Total comments: 2
Patch Set 14 : Moved source files into a list variable and add a custom build target to compile the integration te… #
Total comments: 6
Patch Set 15 : Moved source files into a list variable and add a custom build target to compile the integration te… #
Total comments: 5
Patch Set 16 : Added comments for the variables. #
Total comments: 2
Patch Set 17 : Fixed few nits #Patch Set 18 : Added syzyasan_rtl and export_dll as build dependencies. #Patch Set 19 : Added syzyasan_rtl and export_dll as build dependencies. #Messages
Total messages: 54 (30 generated)
Description was changed from ========== Moved source files into a list variable and add a custom build target to compile the integration tests using llvm BUG= ========== to ========== Moved source files into a list variable and add a custom build target to compile the integration tests using llvm BUG= Depends on: Code Review:https://codereview.chromium.org/2937353002 and https://codereview.chromium.org/2946063002/ ==========
njanevsk@google.com changed reviewers: + etienneb@chromium.org, sebmarchand@chromium.org
PTAL
PTAL
If you look at the diff you'll see that there's nothing to review, you might need to rebase your CL to fix this.
PTAL
https://codereview.chromium.org/2946083002/diff/120001/syzygy/integration_tes... File syzygy/integration_tests/integration_tests.gyp (right): https://codereview.chromium.org/2946083002/diff/120001/syzygy/integration_tes... syzygy/integration_tests/integration_tests.gyp:16: 'variables': { Add comments describing these variables. https://codereview.chromium.org/2946083002/diff/120001/syzygy/integration_tes... syzygy/integration_tests/integration_tests.gyp:17: 'integration_tests_source_files': [ I'd call this integration_tests_common_source_files and I'll only keep the 'asan_*' and integration_tests_dll.cc in this list. We don't want to compile the other ones with Clang. (you'll need to make some changes to the code for this to work!) https://codereview.chromium.org/2946083002/diff/120001/syzygy/integration_tes... syzygy/integration_tests/integration_tests.gyp:130: 'sources': ['<@(integration_tests_source_files)', '<@(integration_tests_other_files)'], Split this on multiple lines, like it was before.
Made the major changes to add enumerated type for the supported compiler for each test case. PTAL.
I split the test case table into Asan and Non-Asan test cases. PTAL!
PTAL!
More comments. Note that I can't approve this until the parent CL has been committed. Usually we try to avoid reviewing CLs dependent on one that hasn't been committed. https://codereview.chromium.org/2946083002/diff/180001/syzygy/integration_tes... File syzygy/integration_tests/integration_tests.gyp (right): https://codereview.chromium.org/2946083002/diff/180001/syzygy/integration_tes... syzygy/integration_tests/integration_tests.gyp:26: 'bb_entry_tests.cc', Fix the indentation. This is something that comes up pretty often in your CLs and could easily be avoided by doing a self review before asking us for one. https://codereview.chromium.org/2946083002/diff/180001/syzygy/integration_tes... syzygy/integration_tests/integration_tests.gyp:131: '<@(integration_tests_other_files)' Same issue here, and you're using tabs instead of spaces. https://codereview.chromium.org/2946083002/diff/180001/syzygy/integration_tes... syzygy/integration_tests/integration_tests.gyp:134: '<(src)/base/base.gyp:base', Ditto. https://codereview.chromium.org/2946083002/diff/200001/syzygy/integration_tes... File syzygy/integration_tests/integration_tests.gyp (right): https://codereview.chromium.org/2946083002/diff/200001/syzygy/integration_tes... syzygy/integration_tests/integration_tests.gyp:17: 'integration_tests_common_source_files': [ Add some comments to describe what these list are. https://codereview.chromium.org/2946083002/diff/200001/syzygy/integration_tes... syzygy/integration_tests/integration_tests.gyp:122: '--target-name=integration_tests_clang_dll', Fix the indentation. https://codereview.chromium.org/2946083002/diff/200001/syzygy/integration_tes... syzygy/integration_tests/integration_tests.gyp:133: ], Use spaces instead of tabs. https://codereview.chromium.org/2946083002/diff/200001/syzygy/integration_tes... File syzygy/integration_tests/integration_tests_dll.h (right): https://codereview.chromium.org/2946083002/diff/200001/syzygy/integration_tes... syzygy/integration_tests/integration_tests_dll.h:24: // This macro declares the SygyAsan tests ids and the function that they're associated SyzyAsan (replace the g by a z) https://codereview.chromium.org/2946083002/diff/200001/syzygy/integration_tes... syzygy/integration_tests/integration_tests_dll.h:151: decl(kAsanInvalidAccessWithCorruptAllocatedBlockHeader, \ Fix the indentation, you're using tabs here. https://codereview.chromium.org/2946083002/diff/200001/syzygy/integration_tes... syzygy/integration_tests/integration_tests_dll.h:180: Remove the extra BLs. https://codereview.chromium.org/2946083002/diff/200001/syzygy/integration_tes... syzygy/integration_tests/integration_tests_dll.h:182: // This macro declares the non SygyAsan tests ids and the function that they're associated > 80 chars. https://codereview.chromium.org/2946083002/diff/200001/syzygy/integration_tes... syzygy/integration_tests/integration_tests_dll.h:200: #define END_TO_END_TEST_ID_TABLE(decl) END_TO_END_NON_ASAN_TESTS(decl) END_TO_END_ASAN_TESTS(decl) This line is > 80 chars.
PTAL
https://codereview.chromium.org/2946083002/diff/240001/syzygy/integration_tes... File syzygy/integration_tests/integration_tests.gyp (right): https://codereview.chromium.org/2946083002/diff/240001/syzygy/integration_tes... syzygy/integration_tests/integration_tests.gyp:135: '<(src)/base/base.gyp:base', I don't think that you need this dependency?
PTAL https://codereview.chromium.org/2946083002/diff/240001/syzygy/integration_tes... File syzygy/integration_tests/integration_tests.gyp (right): https://codereview.chromium.org/2946083002/diff/240001/syzygy/integration_tes... syzygy/integration_tests/integration_tests.gyp:135: '<(src)/base/base.gyp:base', On 2017/07/06 15:45:19, Sébastien Marchand wrote: > I don't think that you need this dependency? Done.
The CQ bit was checked by sebmarchand@chromium.org 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: Try jobs failed on following builders: win_dbg_try on master.tryserver.client.syzygy (JOB_FAILED, https://build.chromium.org/p/tryserver.client.syzygy/builders/win_dbg_try/bui...)
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...
https://codereview.chromium.org/2946083002/diff/250002/syzygy/integration_tes... File syzygy/integration_tests/integration_tests.gyp (right): https://codereview.chromium.org/2946083002/diff/250002/syzygy/integration_tes... syzygy/integration_tests/integration_tests.gyp:119: 'make_integration_tests_clang.py', You need sys.executable before that
https://codereview.chromium.org/2946083002/diff/250002/syzygy/integration_tes... File syzygy/integration_tests/integration_tests.gyp (right): https://codereview.chromium.org/2946083002/diff/250002/syzygy/integration_tes... syzygy/integration_tests/integration_tests.gyp:119: 'make_integration_tests_clang.py', On 2017/07/07 17:31:11, Sébastien Marchand wrote: > You need sys.executable before that Well, in fact you need: '<(python_exe)',
https://codereview.chromium.org/2946083002/diff/250002/syzygy/integration_tes... File syzygy/integration_tests/integration_tests.gyp (right): https://codereview.chromium.org/2946083002/diff/250002/syzygy/integration_tes... syzygy/integration_tests/integration_tests.gyp:121: '--input-files=<(_inputs)', This doesn't work, this gives the following command line: make_integration_tests_clang.py --output-dir=..\..\out\Release --input-files=asan_interceptors_tests.cc asan_page_protection_tests.cc deferred_free_tests.cc integration_tests_dll.cc --target-name=integration_tests_clang_dll all the source files are separated by a space so the option parser can't parse this.
Added python.exe. PTAL https://codereview.chromium.org/2946083002/diff/250002/syzygy/integration_tes... File syzygy/integration_tests/integration_tests.gyp (right): https://codereview.chromium.org/2946083002/diff/250002/syzygy/integration_tes... syzygy/integration_tests/integration_tests.gyp:119: 'make_integration_tests_clang.py', On 2017/07/07 17:31:59, Sébastien Marchand wrote: > On 2017/07/07 17:31:11, Sébastien Marchand wrote: > > You need sys.executable before that > > Well, in fact you need: > '<(python_exe)', Done. https://codereview.chromium.org/2946083002/diff/250002/syzygy/integration_tes... syzygy/integration_tests/integration_tests.gyp:119: 'make_integration_tests_clang.py', On 2017/07/07 17:31:11, Sébastien Marchand wrote: > You need sys.executable before that Done. https://codereview.chromium.org/2946083002/diff/250002/syzygy/integration_tes... syzygy/integration_tests/integration_tests.gyp:121: '--input-files=<(_inputs)', On 2017/07/07 17:41:38, Sébastien Marchand wrote: > This doesn't work, this gives the following command line: > make_integration_tests_clang.py --output-dir=..\..\out\Release > --input-files=asan_interceptors_tests.cc asan_page_protection_tests.cc > deferred_free_tests.cc integration_tests_dll.cc > --target-name=integration_tests_clang_dll > > all the source files are separated by a space so the option parser can't parse > this. The code works. The problem was in the python script. Here is a link to the CL with the fixed python script: https://codereview.chromium.org/2977433002/
The CQ bit was checked by sebmarchand@chromium.org 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.
https://codereview.chromium.org/2946083002/diff/270001/syzygy/integration_tes... File syzygy/integration_tests/integration_tests.gyp (right): https://codereview.chromium.org/2946083002/diff/270001/syzygy/integration_tes... syzygy/integration_tests/integration_tests.gyp:17: 'integration_tests_common_source_files': [ As mentioned in one of the previous reviews, please add a comment about these variables. https://codereview.chromium.org/2946083002/diff/270001/syzygy/integration_tes... syzygy/integration_tests/integration_tests.gyp:123: '--target-name=integration_tests_clang_dll', Can you use the target name here? (from above)
PTAL! https://codereview.chromium.org/2946083002/diff/270001/syzygy/integration_tes... File syzygy/integration_tests/integration_tests.gyp (right): https://codereview.chromium.org/2946083002/diff/270001/syzygy/integration_tes... syzygy/integration_tests/integration_tests.gyp:17: 'integration_tests_common_source_files': [ On 2017/07/10 15:12:43, Sébastien Marchand wrote: > As mentioned in one of the previous reviews, please add a comment about these > variables. Done. https://codereview.chromium.org/2946083002/diff/270001/syzygy/integration_tes... syzygy/integration_tests/integration_tests.gyp:123: '--target-name=integration_tests_clang_dll', On 2017/07/10 15:12:43, Sébastien Marchand wrote: > Can you use the target name here? (from above) Based on the response I got from the gyp-developers: https://groups.google.com/forum/#!topic/gyp-developer/yn6YBD9Y45U It is possible if we add a variable. I prefer to leave it this why cause it is only used only at one place. What you think?
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...
lgtm https://codereview.chromium.org/2946083002/diff/270001/syzygy/integration_tes... File syzygy/integration_tests/integration_tests.gyp (right): https://codereview.chromium.org/2946083002/diff/270001/syzygy/integration_tes... syzygy/integration_tests/integration_tests.gyp:123: '--target-name=integration_tests_clang_dll', On 2017/07/11 18:49:24, njanevsk wrote: > On 2017/07/10 15:12:43, Sébastien Marchand wrote: > > Can you use the target name here? (from above) > > Based on the response I got from the gyp-developers: > https://groups.google.com/forum/#!topic/gyp-developer/yn6YBD9Y45U > > It is possible if we add a variable. I prefer to leave it this why cause it is > only used only at one place. What you think? Yep, that's reasonable, let's leave it like this. https://codereview.chromium.org/2946083002/diff/290001/syzygy/integration_tes... File syzygy/integration_tests/integration_tests.gyp (right): https://codereview.chromium.org/2946083002/diff/290001/syzygy/integration_tes... syzygy/integration_tests/integration_tests.gyp:177: # runtime library1, and the iterator debugging relies on some Fix this.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: win_rel_try on master.tryserver.client.syzygy (JOB_FAILED, https://build.chromium.org/p/tryserver.client.syzygy/builders/win_rel_try/bui...)
https://codereview.chromium.org/2946083002/diff/290001/syzygy/integration_tes... File syzygy/integration_tests/integration_tests.gyp (right): https://codereview.chromium.org/2946083002/diff/290001/syzygy/integration_tes... syzygy/integration_tests/integration_tests.gyp:177: # runtime library1, and the iterator debugging relies on some On 2017/07/11 18:57:01, Sébastien Marchand wrote: > Fix this. Done.
The CQ bit was checked by njanevsk@google.com
The patchset sent to the CQ was uploaded after l-g-t-m from sebmarchand@chromium.org Link to the patchset: https://codereview.chromium.org/2946083002/#ps310001 (title: "Fixed few nits")
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
Try jobs failed on following builders: win_dbg_try on master.tryserver.client.syzygy (JOB_FAILED, https://build.chromium.org/p/tryserver.client.syzygy/builders/win_dbg_try/bui...)
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 checked by njanevsk@google.com
The patchset sent to the CQ was uploaded after l-g-t-m from sebmarchand@chromium.org Link to the patchset: https://codereview.chromium.org/2946083002/#ps350001 (title: "Added syzyasan_rtl and export_dll as build dependencies.")
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": 350001, "attempt_start_ts": 1500056593088880,
"parent_rev": "1242df937a63aaafa8a1ba9a2c7d18885bf6e18a", "commit_rev":
"db08dd13dcf021804dfc4aa50c132bf335b269de"}
Message was sent while issue was closed.
Description was changed from ========== Moved source files into a list variable and add a custom build target to compile the integration tests using llvm BUG= Depends on: Code Review:https://codereview.chromium.org/2937353002 and https://codereview.chromium.org/2946063002/ ========== to ========== Moved source files into a list variable and add a custom build target to compile the integration tests using llvm BUG= Depends on: Code Review:https://codereview.chromium.org/2937353002 and https://codereview.chromium.org/2946063002/ Review-Url: https://codereview.chromium.org/2946083002 Committed: https://github.com/google/syzygy/commit/db08dd13dcf021804dfc4aa50c132bf335b269de ==========
Message was sent while issue was closed.
Committed patchset #19 (id:350001) as https://github.com/google/syzygy/commit/db08dd13dcf021804dfc4aa50c132bf335b269de
Message was sent while issue was closed.
Patchset #20 (id:370001) has been deleted |
