Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
11379003:
Add Windows ASAN bots. (Closed)
|
Created:
8 years, 1 month ago by iannucci Modified:
8 years ago CC:
chromium-reviews, nsylvain+cc_chromium.org, cmp+cc_chromium.org, Sébastien Marchand, Roger McFarlane (Chromium), Alexander Potapenko, kcc1 Base URL:
http://git.chromium.org/chromium/tools/build.git@neuter Visibility:
Public. |
DescriptionAdd Windows ASAN bots.
This involves adding a new step, and adding a new builder category to chromium.memory.
VMs are already allocated.
BUG=152226
Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=170787
Patch Set 1 #Patch Set 2 : Use real slaves #Patch Set 3 : Missing file #Patch Set 4 : Rebase, tweaks, lint #
Total comments: 5
Patch Set 5 : Cleanup some unnecessary pieces #
Total comments: 32
Patch Set 6 : Fixes and .gitignore #Patch Set 7 : #Patch Set 8 : Cover case with dotted folders #Patch Set 9 : Add CHROME_ALLOCATOR environment var to tests #
Total comments: 10
Patch Set 10 : Make zip_build a bit better #
Total comments: 12
Patch Set 11 : Some fixes. Also make the help message in zip_build more helpful. #Patch Set 12 : Make things a little less (more?) ugly #
Total comments: 11
Patch Set 13 : Substantially slim things down #Patch Set 14 : formatting #
Total comments: 9
Patch Set 15 : nits #Patch Set 16 : Change the window Builder's category to avoid closing the tree #
Messages
Total messages: 25 (0 generated)
https://chromiumcodereview.appspot.com/11379003/diff/8001/scripts/master/fact... File scripts/master/factory/gclient_factory.py (right): https://chromiumcodereview.appspot.com/11379003/diff/8001/scripts/master/fact... scripts/master/factory/gclient_factory.py:240: 'asan=1' in gclient_env.get('GYP_DEFINES', ''))): This change is necessary because asan on windows doesn't use clang. Adding this step causes windows to hork.
PTAL. You may want to grab a coffee for this one :)
+sebmarchand, +rogerm
Nice! https://chromiumcodereview.appspot.com/11379003/diff/8001/scripts/slave/chrom... File scripts/slave/chromium/win_apply_asan.py (right): https://chromiumcodereview.appspot.com/11379003/diff/8001/scripts/slave/chrom... scripts/slave/chromium/win_apply_asan.py:45: '--input-pdb=%s' % pdb, If the input PDB file can be expected to reside in the same directory as the input image (or reside in the original path to which it was generated), it may be more robust to omit the input-pdb parameter and let the instrumenter automatically locate the PDB file. This way you won't get broken if the naming convention changes again (as it just did for M25). https://chromiumcodereview.appspot.com/11379003/diff/8001/scripts/slave/chrom... scripts/slave/chromium/win_apply_asan.py:128: if IsAlreadyASAN(pe_image, pdb): With your naming convention (foo.asan.dll) excluding previously asan-ified files, is this necessary?
https://chromiumcodereview.appspot.com/11379003/diff/8001/scripts/slave/chrom... File scripts/slave/chromium/win_apply_asan.py (right): https://chromiumcodereview.appspot.com/11379003/diff/8001/scripts/slave/chrom... scripts/slave/chromium/win_apply_asan.py:45: '--input-pdb=%s' % pdb, On 2012/11/21 03:59:30, Roger McFarlane (Chromium) wrote: > If the input PDB file can be expected to reside in the same directory as the > input image (or reside in the original path to which it was generated), it may > be more robust to omit the input-pdb parameter and let the instrumenter > automatically locate the PDB file. This way you won't get broken if the naming > convention changes again (as it just did for M25). Done. Yeah... though I'm relying on the naming convention down below too... I would rather be able to detect: * Does this file have a matching pdb? * Has this exe been asan'd already? Then I could accurately give warnings... but it's better to get this version out first :). https://chromiumcodereview.appspot.com/11379003/diff/8001/scripts/slave/chrom... scripts/slave/chromium/win_apply_asan.py:128: if IsAlreadyASAN(pe_image, pdb): On 2012/11/21 03:59:30, Roger McFarlane (Chromium) wrote: > With your naming convention (foo.asan.dll) excluding previously asan-ified > files, is this necessary? Yeah... this block made more sense when I was renaming the output files over top of the originals. I took it all out.
https://chromiumcodereview.appspot.com/11379003/diff/8002/masters/master.chro... File masters/master.chromium.memory/master_win_cfg.py (right): https://chromiumcodereview.appspot.com/11379003/diff/8002/masters/master.chro... masters/master.chromium.memory/master_win_cfg.py:40: tests_1 = \ ? tests_1 = [ https://chromiumcodereview.appspot.com/11379003/diff/8002/masters/master.chro... masters/master.chromium.memory/master_win_cfg.py:49: ]+[ ] + [ https://chromiumcodereview.appspot.com/11379003/diff/8002/masters/master.chro... masters/master.chromium.memory/master_win_cfg.py:81: options=[ This is too complex to be done inside the function call, use a named variable. https://chromiumcodereview.appspot.com/11379003/diff/8002/masters/master.chro... masters/master.chromium.memory/master_win_cfg.py:93: 'GYP_DEFINES': ('asan=1 win_z7=1 chromium_win_pch=0 ' God kills a kitten everytime people align the GYP_DEFINES like that. https://chromiumcodereview.appspot.com/11379003/diff/8002/masters/master.chro... masters/master.chromium.memory/master_win_cfg.py:107: factory_properties={'asan': True, same https://chromiumcodereview.appspot.com/11379003/diff/8002/masters/master.chro... masters/master.chromium.memory/master_win_cfg.py:123: remove https://chromiumcodereview.appspot.com/11379003/diff/8002/scripts/master/fact... File scripts/master/factory/gclient_factory.py (right): https://chromiumcodereview.appspot.com/11379003/diff/8002/scripts/master/fact... scripts/master/factory/gclient_factory.py:241: factory_cmd_obj.AddUpdateClangStep() Just a note here; the check for asan=1 is so wrong, it should only look for clang=1. https://chromiumcodereview.appspot.com/11379003/diff/8002/scripts/slave/chrom... File scripts/slave/chromium/win_apply_asan.py (right): https://chromiumcodereview.appspot.com/11379003/diff/8002/scripts/slave/chrom... scripts/slave/chromium/win_apply_asan.py:16: 'sql.dll', Keep ordered https://chromiumcodereview.appspot.com/11379003/diff/8002/scripts/slave/chrom... scripts/slave/chromium/win_apply_asan.py:19: 'icuuc.dll' Comma on all items. https://chromiumcodereview.appspot.com/11379003/diff/8002/scripts/slave/chrom... scripts/slave/chromium/win_apply_asan.py:53: proc = subprocess.Popen(command, shell=True, stdout=subprocess.PIPE) Creating a process just to start a process is a tad overkill on Windows; you'll get significant performance improvement by using a few threads instead and a Queue.Queue(). https://chromiumcodereview.appspot.com/11379003/diff/8002/scripts/slave/chrom... scripts/slave/chromium/win_apply_asan.py:135: initargs=(options, stopped)) Is this code I/O bound or CPU bound? I'd guess I/O bound, in that case, you are totally saturating the disk if you start <number of CPU cores> separate processes. https://chromiumcodereview.appspot.com/11379003/diff/8002/scripts/slave/chrom... scripts/slave/chromium/win_apply_asan.py:164: parser.add_option('--build_directory', alignment is wrong, either all +4 or not. https://chromiumcodereview.appspot.com/11379003/diff/8002/scripts/slave/chrom... scripts/slave/chromium/win_apply_asan.py:178: if options.build_directory is None: if not options.build_directory: https://chromiumcodereview.appspot.com/11379003/diff/8002/scripts/slave/chrom... scripts/slave/chromium/win_apply_asan.py:205: 2 lines https://chromiumcodereview.appspot.com/11379003/diff/8002/scripts/slave/zip_b... File scripts/slave/zip_build.py (right): https://chromiumcodereview.appspot.com/11379003/diff/8002/scripts/slave/zip_b... scripts/slave/zip_build.py:203: parts = path.split('.', 1) what about foo.bar/base.exe ? https://chromiumcodereview.appspot.com/11379003/diff/8002/scripts/slave/zip_b... scripts/slave/zip_build.py:391: 2 lines
I also added CHROME_ALLOCATOR=WINHEAP to the environment via a new key in factory_properties. https://chromiumcodereview.appspot.com/11379003/diff/8002/masters/master.chro... File masters/master.chromium.memory/master_win_cfg.py (right): https://chromiumcodereview.appspot.com/11379003/diff/8002/masters/master.chro... masters/master.chromium.memory/master_win_cfg.py:40: tests_1 = \ On 2012/11/21 09:06:32, Marc-Antoine Ruel wrote: > ? > tests_1 = [ Done. https://chromiumcodereview.appspot.com/11379003/diff/8002/masters/master.chro... masters/master.chromium.memory/master_win_cfg.py:49: ]+[ On 2012/11/21 09:06:32, Marc-Antoine Ruel wrote: > ] + [ Done. https://chromiumcodereview.appspot.com/11379003/diff/8002/masters/master.chro... masters/master.chromium.memory/master_win_cfg.py:81: options=[ On 2012/11/21 09:06:32, Marc-Antoine Ruel wrote: > This is too complex to be done inside the function call, use a named variable. Done. https://chromiumcodereview.appspot.com/11379003/diff/8002/masters/master.chro... masters/master.chromium.memory/master_win_cfg.py:93: 'GYP_DEFINES': ('asan=1 win_z7=1 chromium_win_pch=0 ' On 2012/11/21 09:06:32, Marc-Antoine Ruel wrote: > God kills a kitten everytime people align the GYP_DEFINES like that. :( Done. https://chromiumcodereview.appspot.com/11379003/diff/8002/masters/master.chro... masters/master.chromium.memory/master_win_cfg.py:107: factory_properties={'asan': True, On 2012/11/21 09:06:32, Marc-Antoine Ruel wrote: > same Done. https://chromiumcodereview.appspot.com/11379003/diff/8002/masters/master.chro... masters/master.chromium.memory/master_win_cfg.py:123: On 2012/11/21 09:06:32, Marc-Antoine Ruel wrote: > remove Done. https://chromiumcodereview.appspot.com/11379003/diff/8002/scripts/master/fact... File scripts/master/factory/gclient_factory.py (right): https://chromiumcodereview.appspot.com/11379003/diff/8002/scripts/master/fact... scripts/master/factory/gclient_factory.py:241: factory_cmd_obj.AddUpdateClangStep() On 2012/11/21 09:06:32, Marc-Antoine Ruel wrote: > Just a note here; the check for asan=1 is so wrong, it should only look for > clang=1. Yes agreed... https://chromiumcodereview.appspot.com/11379003/diff/8002/scripts/slave/chrom... File scripts/slave/chromium/win_apply_asan.py (right): https://chromiumcodereview.appspot.com/11379003/diff/8002/scripts/slave/chrom... scripts/slave/chromium/win_apply_asan.py:16: 'sql.dll', On 2012/11/21 09:06:32, Marc-Antoine Ruel wrote: > Keep ordered Done. https://chromiumcodereview.appspot.com/11379003/diff/8002/scripts/slave/chrom... scripts/slave/chromium/win_apply_asan.py:19: 'icuuc.dll' On 2012/11/21 09:06:32, Marc-Antoine Ruel wrote: > Comma on all items. Done. https://chromiumcodereview.appspot.com/11379003/diff/8002/scripts/slave/chrom... scripts/slave/chromium/win_apply_asan.py:53: proc = subprocess.Popen(command, shell=True, stdout=subprocess.PIPE) On 2012/11/21 09:06:32, Marc-Antoine Ruel wrote: > Creating a process just to start a process is a tad overkill on Windows; you'll > get significant performance improvement by using a few threads instead and a > Queue.Queue(). Yeah I started down that path, but I felt that multiprocessing.Pool would be cleaner. I'm not sure the performance benefit is worth the readability hit? Only reason I was doing https://chromiumcodereview.appspot.com/11379003/diff/8002/scripts/slave/chrom... scripts/slave/chromium/win_apply_asan.py:135: initargs=(options, stopped)) On 2012/11/21 09:06:32, Marc-Antoine Ruel wrote: > Is this code I/O bound or CPU bound? I'd guess I/O bound, in that case, you are > totally saturating the disk if you start <number of CPU cores> separate > processes. That's why jobs is tweakable. https://chromiumcodereview.appspot.com/11379003/diff/8002/scripts/slave/chrom... scripts/slave/chromium/win_apply_asan.py:164: parser.add_option('--build_directory', On 2012/11/21 09:06:32, Marc-Antoine Ruel wrote: > alignment is wrong, either all +4 or not. Done. https://chromiumcodereview.appspot.com/11379003/diff/8002/scripts/slave/chrom... scripts/slave/chromium/win_apply_asan.py:178: if options.build_directory is None: On 2012/11/21 09:06:32, Marc-Antoine Ruel wrote: > if not options.build_directory: Done. https://chromiumcodereview.appspot.com/11379003/diff/8002/scripts/slave/chrom... scripts/slave/chromium/win_apply_asan.py:205: On 2012/11/21 09:06:32, Marc-Antoine Ruel wrote: > 2 lines Done. https://chromiumcodereview.appspot.com/11379003/diff/8002/scripts/slave/zip_b... File scripts/slave/zip_build.py (right): https://chromiumcodereview.appspot.com/11379003/diff/8002/scripts/slave/zip_b... scripts/slave/zip_build.py:203: parts = path.split('.', 1) On 2012/11/21 09:06:32, Marc-Antoine Ruel wrote: > what about foo.bar/base.exe ? Ack! Excellent point... I don't think we have that, but I'll fix. https://chromiumcodereview.appspot.com/11379003/diff/8002/scripts/slave/zip_b... scripts/slave/zip_build.py:391: On 2012/11/21 09:06:32, Marc-Antoine Ruel wrote: > 2 lines Done.
A couple more comments. I'll leave it to the infrastructure folks to sign off or not on the CL as a whole. https://chromiumcodereview.appspot.com/11379003/diff/8003/scripts/master/fact... File scripts/master/factory/chromium_factory.py (right): https://chromiumcodereview.appspot.com/11379003/diff/8003/scripts/master/fact... scripts/master/factory/chromium_factory.py:904: factory_properties=factory_properties) alignment +1 https://chromiumcodereview.appspot.com/11379003/diff/8003/scripts/slave/chrom... File scripts/slave/chromium/win_apply_asan.py (right): https://chromiumcodereview.appspot.com/11379003/diff/8003/scripts/slave/chrom... scripts/slave/chromium/win_apply_asan.py:44: '--input-image=%s' % pe_image, this indentation looks wonky. OPTIONS.instrument_exe to the next line and +4 for it and all other params? https://chromiumcodereview.appspot.com/11379003/diff/8003/scripts/slave/chrom... scripts/slave/chromium/win_apply_asan.py:135: initargs=(options, stopped)) everything +4 or aligned to opening paren. https://chromiumcodereview.appspot.com/11379003/diff/8003/scripts/slave/chrom... scripts/slave/chromium/win_apply_asan.py:159: 'third_party', 'syzygy', 'binaries', 'exe') everything +4 or everything aligned to opening paren. https://chromiumcodereview.appspot.com/11379003/diff/8003/scripts/slave/zip_b... File scripts/slave/zip_build.py (right): https://chromiumcodereview.appspot.com/11379003/diff/8003/scripts/slave/zip_b... scripts/slave/zip_build.py:201: if 'asan' in filters: Maybe take the filter as a collection of functions instead of names? Then define the asan filter as a library function. You can map the filter names specified as command-line parameters to the actual functions when you validate the params.
https://chromiumcodereview.appspot.com/11379003/diff/8003/scripts/master/fact... File scripts/master/factory/chromium_factory.py (right): https://chromiumcodereview.appspot.com/11379003/diff/8003/scripts/master/fact... scripts/master/factory/chromium_factory.py:904: factory_properties=factory_properties) On 2012/11/21 19:29:52, Roger McFarlane (Chromium) wrote: > alignment +1 Ew gross. Not sure how that got in there. https://chromiumcodereview.appspot.com/11379003/diff/8003/scripts/slave/chrom... File scripts/slave/chromium/win_apply_asan.py (right): https://chromiumcodereview.appspot.com/11379003/diff/8003/scripts/slave/chrom... scripts/slave/chromium/win_apply_asan.py:44: '--input-image=%s' % pe_image, On 2012/11/21 19:29:52, Roger McFarlane (Chromium) wrote: > this indentation looks wonky. > > OPTIONS.instrument_exe to the next line and +4 for it and all other params? Done. I'm still learning the way of the google-python... https://chromiumcodereview.appspot.com/11379003/diff/8003/scripts/slave/chrom... scripts/slave/chromium/win_apply_asan.py:135: initargs=(options, stopped)) On 2012/11/21 19:29:52, Roger McFarlane (Chromium) wrote: > everything +4 or aligned to opening paren. Done. https://chromiumcodereview.appspot.com/11379003/diff/8003/scripts/slave/chrom... scripts/slave/chromium/win_apply_asan.py:159: 'third_party', 'syzygy', 'binaries', 'exe') On 2012/11/21 19:29:52, Roger McFarlane (Chromium) wrote: > everything +4 or everything aligned to opening paren. Done. https://chromiumcodereview.appspot.com/11379003/diff/8003/scripts/slave/zip_b... File scripts/slave/zip_build.py (right): https://chromiumcodereview.appspot.com/11379003/diff/8003/scripts/slave/zip_b... scripts/slave/zip_build.py:201: if 'asan' in filters: On 2012/11/21 19:29:52, Roger McFarlane (Chromium) wrote: > Maybe take the filter as a collection of functions instead of names? > > Then define the asan filter as a library function. You can map the filter names > specified as command-line parameters to the actual functions when you validate > the params. Good idea. Done.
@maruel, if you could take another look, that would be awesome. I'd really like to get these bots online this week if possible :). Unless there's someone else who would be better to review?
generic question : why can't we replace the binary itself with the asanified version instead of creating a new one? https://chromiumcodereview.appspot.com/11379003/diff/9012/masters/master.chro... File masters/master.chromium.memory/master_win_cfg.py (right): https://chromiumcodereview.appspot.com/11379003/diff/9012/masters/master.chro... masters/master.chromium.memory/master_win_cfg.py:38: # This would be much less ugly if there was a naming convention in place for why not spelling them all out? This seems a bit overkill. The naming convention is to name them all like the target name in gyp, which means a lot of them will be renamed once we are done with the change to follow the convention. This also makes it super hard to grep for a test name since it's not spelled out explicitly. Lots of them already have alternate names in the factory to be able to call them using their target name. If you really hate it, you can add more alternate names in the factory so the list can have only 1 dimension. https://chromiumcodereview.appspot.com/11379003/diff/9012/masters/master.chro... masters/master.chromium.memory/master_win_cfg.py:89: compile_timeout=4800, do you think this is necessary? Does it really take longer to build with asan? We can actually give you a bare metal for that if you need one. https://chromiumcodereview.appspot.com/11379003/diff/9012/scripts/master/fact... File scripts/master/factory/commands.py (right): https://chromiumcodereview.appspot.com/11379003/diff/9012/scripts/master/fact... scripts/master/factory/commands.py:566: env = factory_properties.get('testing_env') is this needed? https://chromiumcodereview.appspot.com/11379003/diff/9012/scripts/master/fact... File scripts/master/factory/gclient_factory.py (right): https://chromiumcodereview.appspot.com/11379003/diff/9012/scripts/master/fact... scripts/master/factory/gclient_factory.py:212: target_arch=None, add_archive_steps=True): What about "skip_archive_steps=False" instead. It makes it a bit more obvious about what it does/ https://chromiumcodereview.appspot.com/11379003/diff/9012/scripts/slave/chrom... File scripts/slave/chromium/win_apply_asan.py (right): https://chromiumcodereview.appspot.com/11379003/diff/9012/scripts/slave/chrom... scripts/slave/chromium/win_apply_asan.py:168: '--build_directory', we use --build-dir everywhere else, we should probably stick with it. https://chromiumcodereview.appspot.com/11379003/diff/9012/scripts/slave/zip_b... File scripts/slave/zip_build.py (right): https://chromiumcodereview.appspot.com/11379003/diff/9012/scripts/slave/zip_b... scripts/slave/zip_build.py:395: if options.factory_properties.get('asan'): this is used for more than windows, so all the current asan bots will be using the filter now, is this expected?
General answer: Originally I had it build the asan version and then move it over on top of the non-asan'd version (instead of doing fancyness in zip_build). Unfortunately, this sucks for a couple reasons: * If a build product gets dropped, it will get asan'd over and over, growing without bound until it takes over the universe. That would be annoying. * It goofs with the build system's ability to do incremental builds. * Raciness compounds the above issues :) https://codereview.chromium.org/11379003/diff/9012/masters/master.chromium.me... File masters/master.chromium.memory/master_win_cfg.py (right): https://codereview.chromium.org/11379003/diff/9012/masters/master.chromium.me... masters/master.chromium.memory/master_win_cfg.py:38: # This would be much less ugly if there was a naming convention in place for On 2012/11/27 00:23:15, nsylvain wrote: > why not spelling them all out? This seems a bit overkill. The naming > convention is to name them all like the target name in gyp, which means a lot of > them will be renamed once we are done with the change to follow the convention. > This also makes it super hard to grep for a test name since it's not spelled > out explicitly. Lots of them already have alternate names in the factory to be > able to call them using their target name. If you really hate it, you can add > more alternate names in the factory so the list can have only 1 dimension. I'll add aliases so that this list becomes one-dimensional. Thanks for the tip. https://codereview.chromium.org/11379003/diff/9012/masters/master.chromium.me... masters/master.chromium.memory/master_win_cfg.py:89: compile_timeout=4800, On 2012/11/27 00:23:15, nsylvain wrote: > do you think this is necessary? Does it really take longer to build with asan? > We can actually give you a bare metal for that if you need one. I added that after it timed out on my bare metal machine :D. It's from all the PDBs :( https://codereview.chromium.org/11379003/diff/9012/scripts/master/factory/com... File scripts/master/factory/commands.py (right): https://codereview.chromium.org/11379003/diff/9012/scripts/master/factory/com... scripts/master/factory/commands.py:566: env = factory_properties.get('testing_env') On 2012/11/27 00:23:15, nsylvain wrote: > is this needed? Yes, because we need to set the CHROME_ALLOCATOR=WINHEAP env var during the test step. See bottom of master_win_cfg.py. https://codereview.chromium.org/11379003/diff/9012/scripts/master/factory/gcl... File scripts/master/factory/gclient_factory.py (right): https://codereview.chromium.org/11379003/diff/9012/scripts/master/factory/gcl... scripts/master/factory/gclient_factory.py:212: target_arch=None, add_archive_steps=True): On 2012/11/27 00:23:15, nsylvain wrote: > What about "skip_archive_steps=False" instead. It makes it a bit more obvious > about what it does/ Good call. Done. https://codereview.chromium.org/11379003/diff/9012/scripts/slave/chromium/win... File scripts/slave/chromium/win_apply_asan.py (right): https://codereview.chromium.org/11379003/diff/9012/scripts/slave/chromium/win... scripts/slave/chromium/win_apply_asan.py:168: '--build_directory', On 2012/11/27 00:23:15, nsylvain wrote: > we use --build-dir everywhere else, we should probably stick with it. Done. https://codereview.chromium.org/11379003/diff/9012/scripts/slave/zip_build.py File scripts/slave/zip_build.py (right): https://codereview.chromium.org/11379003/diff/9012/scripts/slave/zip_build.py... scripts/slave/zip_build.py:395: if options.factory_properties.get('asan'): On 2012/11/27 00:23:15, nsylvain wrote: > this is used for more than windows, so all the current asan bots will be using > the filter now, is this expected? I suppose it's not expected, but the filter will be a no-op on other platforms. It seemed like it would be messier to have it conditionally run on windows. This will have the side effect of one extra stat call on non-windows platforms (which should be fine, since the posix platforms should be able to handle the stat for a non-existent file quickly).
This CL adds a lot of unrelated changes. It would have been much less painful to split this CL in a few parts, in particular the slave-side from the master-side, and the R() refactoring part from the rest. https://codereview.chromium.org/11379003/diff/10011/masters/master.chromium.m... File masters/master.chromium.memory/master_win_cfg.py (right): https://codereview.chromium.org/11379003/diff/10011/masters/master.chromium.m... masters/master.chromium.memory/master_win_cfg.py:55: ] At +0 please https://codereview.chromium.org/11379003/diff/10011/masters/master.chromium.m... masters/master.chromium.memory/master_win_cfg.py:61: '--compiler=goma', alignment is excessive. Is your text editor angry? https://codereview.chromium.org/11379003/diff/10011/scripts/common/chromium_u... File scripts/common/chromium_utils.py (right): https://codereview.chromium.org/11379003/diff/10011/scripts/common/chromium_u... scripts/common/chromium_utils.py:536: new_src_path = func(src_path) With the code you wrote, you could as well do: if replacements: new_src_path = replacements[-1](src_path) Was it intended? https://codereview.chromium.org/11379003/diff/10011/scripts/master/factory/ch... File scripts/master/factory/chromium_commands.py (right): https://codereview.chromium.org/11379003/diff/10011/scripts/master/factory/ch... scripts/master/factory/chromium_commands.py:72: self._windows_asan_tool = J(s_dir, 'win_apply_asan.py') I don't think it's useful unless it used multiple times. (I don't mind that much) https://codereview.chromium.org/11379003/diff/10011/scripts/master/factory/ch... File scripts/master/factory/chromium_factory.py (right): https://codereview.chromium.org/11379003/diff/10011/scripts/master/factory/ch... scripts/master/factory/chromium_factory.py:265: for test in testnames: That's a good idea for transition. https://codereview.chromium.org/11379003/diff/10011/scripts/slave/chromium/wi... File scripts/slave/chromium/win_apply_asan.py (right): https://codereview.chromium.org/11379003/diff/10011/scripts/slave/chromium/wi... scripts/slave/chromium/win_apply_asan.py:75: return base+'.'+new_ext+'.'+ext add spaces
All fixed... I also cut this cl up into a series of smaller CLs: * Add the ability for tests to have more than one name: https://chromiumcodereview.appspot.com/11280205/ * Add a 'filter' option to zip_build to allow for build post-processing: https://chromiumcodereview.appspot.com/11413207/ * Add the win_apply_asan client script: https://chromiumcodereview.appspot.com/11428043/ This cl now does what it says in it's title. https://chromiumcodereview.appspot.com/11379003/diff/10011/masters/master.chr... File masters/master.chromium.memory/master_win_cfg.py (right): https://chromiumcodereview.appspot.com/11379003/diff/10011/masters/master.chr... masters/master.chromium.memory/master_win_cfg.py:55: ] On 2012/11/27 20:40:15, Marc-Antoine Ruel wrote: > At +0 please Done. https://chromiumcodereview.appspot.com/11379003/diff/10011/masters/master.chr... masters/master.chromium.memory/master_win_cfg.py:61: '--compiler=goma', On 2012/11/27 20:40:15, Marc-Antoine Ruel wrote: > alignment is excessive. Is your text editor angry? Yeah... I think so https://chromiumcodereview.appspot.com/11379003/diff/10011/scripts/common/chr... File scripts/common/chromium_utils.py (right): https://chromiumcodereview.appspot.com/11379003/diff/10011/scripts/common/chr... scripts/common/chromium_utils.py:536: new_src_path = func(src_path) On 2012/11/27 20:40:15, Marc-Antoine Ruel wrote: > With the code you wrote, you could as well do: > if replacements: > new_src_path = replacements[-1](src_path) > > Was it intended? Yeah... the intent was that there could be multiple filters, applied in order, since this is a library function. However, it would be equally well served by passing a single function, which would be simpler to reason about here, so I'll do that. https://chromiumcodereview.appspot.com/11379003/diff/10011/scripts/master/fac... File scripts/master/factory/chromium_commands.py (right): https://chromiumcodereview.appspot.com/11379003/diff/10011/scripts/master/fac... scripts/master/factory/chromium_commands.py:72: self._windows_asan_tool = J(s_dir, 'win_apply_asan.py') On 2012/11/27 20:40:15, Marc-Antoine Ruel wrote: > I don't think it's useful unless it used multiple times. > > (I don't mind that much) It seemed more consistent with the existing code. I'll keep it :) https://chromiumcodereview.appspot.com/11379003/diff/10011/scripts/slave/chro... File scripts/slave/chromium/win_apply_asan.py (right): https://chromiumcodereview.appspot.com/11379003/diff/10011/scripts/slave/chro... scripts/slave/chromium/win_apply_asan.py:75: return base+'.'+new_ext+'.'+ext On 2012/11/27 20:40:15, Marc-Antoine Ruel wrote: > add spaces Done.
https://chromiumcodereview.appspot.com/11379003/diff/17007/masters/master.chr... File masters/master.chromium.memory/master_win_cfg.py (right): https://chromiumcodereview.appspot.com/11379003/diff/17007/masters/master.chr... masters/master.chromium.memory/master_win_cfg.py:60: '--compiler=goma', '--build-tool=ninja' Interesting, here you don't use '--'. https://chromiumcodereview.appspot.com/11379003/diff/17007/masters/master.chr... masters/master.chromium.memory/master_win_cfg.py:72: 'GYP_DEFINES': ( FYI, running: print { 'f': 'b' 'c', 'g': 'bar' } prints {'g': 'bar', 'f': 'bc'} so the () is not strictly necessary. But I agree it aids comprehension. https://chromiumcodereview.appspot.com/11379003/diff/17007/masters/master.chr... masters/master.chromium.memory/master_win_cfg.py:92: 'browser_total_shards': 2, Keep dict entries sorted whenever possible. https://chromiumcodereview.appspot.com/11379003/diff/17007/scripts/master/fac... File scripts/master/factory/chromium_factory.py (right): https://chromiumcodereview.appspot.com/11379003/diff/17007/scripts/master/fac... scripts/master/factory/chromium_factory.py:882: is_windows_asan_builder = (slave_type == 'Builder' and and BuilderTester? https://chromiumcodereview.appspot.com/11379003/diff/17007/scripts/master/fac... scripts/master/factory/chromium_factory.py:884: factory_properties.get('asan')) but in scripts/master/factory/gclient_factory.py line 240 you check for asan=1 in GCLIENT_ENV instead. I guess it's fine for now but this should be cleaned up eventually.
https://chromiumcodereview.appspot.com/11379003/diff/17007/masters/master.chr... File masters/master.chromium.memory/master_win_cfg.py (right): https://chromiumcodereview.appspot.com/11379003/diff/17007/masters/master.chr... masters/master.chromium.memory/master_win_cfg.py:60: '--compiler=goma', '--build-tool=ninja' On 2012/11/28 15:07:48, Marc-Antoine Ruel wrote: > Interesting, here you don't use '--'. Ack... Done. https://chromiumcodereview.appspot.com/11379003/diff/17007/masters/master.chr... masters/master.chromium.memory/master_win_cfg.py:92: 'browser_total_shards': 2, On 2012/11/28 15:07:48, Marc-Antoine Ruel wrote: > Keep dict entries sorted whenever possible. Done. https://chromiumcodereview.appspot.com/11379003/diff/17007/scripts/master/fac... File scripts/master/factory/chromium_factory.py (right): https://chromiumcodereview.appspot.com/11379003/diff/17007/scripts/master/fac... scripts/master/factory/chromium_factory.py:882: is_windows_asan_builder = (slave_type == 'Builder' and On 2012/11/28 15:07:48, Marc-Antoine Ruel wrote: > and BuilderTester? Nope, just builder, since the zip step doesn't happen on builder testers already. https://chromiumcodereview.appspot.com/11379003/diff/17007/scripts/master/fac... scripts/master/factory/chromium_factory.py:884: factory_properties.get('asan')) On 2012/11/28 15:07:48, Marc-Antoine Ruel wrote: > but in scripts/master/factory/gclient_factory.py line 240 you check for asan=1 > in GCLIENT_ENV instead. I guess it's fine for now but this should be cleaned up > eventually. Agreed... I checked GCLIENT_ENV there to be consistent in that file, but it seems that generally, factory_properties is better. I'll break convention there to go towards the better way (factory_properties).
lgtm
On 2012/11/29 14:09:25, Marc-Antoine Ruel wrote: > lgtm Take another look... I missed a tiny (but crucial) change :/. Difference is in masters/master.chromium.memory/master_win_cfg.py .
lgtm Is the intention to make these tree closers once they prove themselves out?
On 2012/12/01 03:35:12, cmp wrote: > lgtm > > Is the intention to make these tree closers once they prove themselves out? Yup, that's the idea.
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/iannucci@chromium.org/11379003/19019
Message was sent while issue was closed.
Change committed as 170787
Message was sent while issue was closed.
Hi, Do you have an estimate of when these bots are green and stable? I thought the whole Memory waterfall's idea was to have a stable, always-green and fast subset of tool+test configurations. (Alexander, Kostya, correct me if I'm wrong) For slow or not-always-green tool+tests configurations we use the "Memory FYI" waterfall, For unstable tests we use the Experimental/FYI waterfall. If your estimate for meeting the Memory waterfall criteria is "more than a week", I'd recommend to move the bots to the FYI waterfall, make it stable there and then move back to Memory or MFYI.
It's taken a bit longer to get these guys to be solid-green than I had initially anticipated. The goal is to have these be tree closers soon, once they're stable, which is why I started them on memory instead of memory.fyi. The current redness is due a misconfiguration (CL waiting review), and 2 bugs in the asan tool (sebmarchand@ is looking at them). If we can't get them stable green by the end of this week, then I'll move them to memory.fyi until they're green, as I agree, they shouldn't be continuously red on memory. On Sat, Dec 8, 2012 at 1:08 AM, <timurrrr@chromium.org> wrote: > Hi, > > Do you have an estimate of when these bots are green and stable? > > I thought the whole Memory waterfall's idea was to have a stable, > always-green > and fast subset of tool+test configurations. > (Alexander, Kostya, correct me if I'm wrong) > > For slow or not-always-green tool+tests configurations we use the "Memory > FYI" > waterfall, > For unstable tests we use the Experimental/FYI waterfall. > > If your estimate for meeting the Memory waterfall criteria is "more than a > week", I'd recommend to move the bots to the FYI waterfall, make it stable > there > and then move back to Memory or MFYI. > > https://chromiumcodereview.**appspot.com/11379003/<https://chromiumcodereview... >
On Sat, Dec 8, 2012 at 1:40 PM, Robert Iannucci <iannucci@chromium.org> wrote: > If we can't get them stable green by the end of this week, then I'll move > them to memory.fyi until they're green, as I agree, they shouldn't be > continuously red on memory. To FYI then please, not MFYI. MFYI is stable too and watched by sheriffs, no need to distract them by constantly-red bots. |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
