Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
11175016:
Selective build clobbering feature (landmines.py and android build scripts). (Closed)
|
Created:
8 years, 2 months ago by iannucci Modified:
8 years, 1 month ago CC:
chromium-reviews, peter+watch_chromium.org, bulach+watch_chromium.org, yfriedman+watch_chromium.org, ilevy+watch_chromium.org Base URL:
http://git.chromium.org/chromium/src.git@master Visibility:
Public. |
DescriptionSelective build clobbering feature (landmines.py and android build scripts).
Adds the ability for devs/troopers/etc. to set 'landmines' in the tree so that
the build will selectively clobber when a builder moves over a revision with such
a change.
This cl has an basis landmines.py, and hooks the clobber mechanism to the android
build scripts.
The relevant cl which implements this for
compile.py is here: https://chromiumcodereview.appspot.com/11234013/
I'm planning to also implement an informational invocation for gclient to let devs know
about any potential landmines so they can decide if they need to clobber.
R=cmp,maruel@chromium.org
BUG=121897
Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=166085
Patch Set 1 #
Total comments: 10
Patch Set 2 : Fix some nits... all this is going to be rewritten shortly #Patch Set 3 : Total rewrite :). #
Total comments: 25
Patch Set 4 : fix bug when generator is make-android #Patch Set 5 : some fixes for andriod #Patch Set 6 : this is simpler #Patch Set 7 : rebase #Patch Set 8 : make landmines set prior to checking clobber #Patch Set 9 : fix nits #
Total comments: 18
Patch Set 10 : Fix some things! #
Total comments: 1
Patch Set 11 : ich habe die nit fixiert #
Total comments: 13
Patch Set 12 : fix more #
Total comments: 3
Patch Set 13 : Fix memoize to really require no params #Patch Set 14 : Rebase and add note to keep functions in sync #
Messages
Total messages: 35 (0 generated)
Saw this CL in my review list. I'd prefer this functionality be written in a stand alone python script rather than as a new function in buildbot_functions. We are trying to avoid additional complex bash functions. Due to bash's "flexibility", it's hard to ensure correctness. Using python will also allow you to parse factory_props.
On 2012/10/20 08:38:54, Isaac wrote: > Saw this CL in my review list. > > I'd prefer this functionality be written in a stand alone python script rather > than as a new function in buildbot_functions. We are trying to avoid additional > complex bash functions. Due to bash's "flexibility", it's hard to ensure > correctness. Using python will also allow you to parse factory_props. Yeah, ok. I'll see if I can't pull the code out into a common python module. I should have done that in the first place anyway. I'm also writing a gclient hook for this and realized too late that I should have just done that from the beginning. Oh well. Unfortunately, there's no uniform data standard for build options, which means that there will be some sort of data reconciliation at all the callsites :(. Some data is on the command line, some is in the env (possibly in multiple gyp variables), and some of it comes via json. bleh.
https://chromiumcodereview.appspot.com/11175016/diff/1/build/landmines.py File build/landmines.py (right): https://chromiumcodereview.appspot.com/11175016/diff/1/build/landmines.py#new... build/landmines.py:1: # Copyright (c) 2012 The Chromium Authors. All rights reserved. before line 1, insert a #!/usr/bin/python call https://chromiumcodereview.appspot.com/11175016/diff/1/build/landmines.py#new... build/landmines.py:9: On every bulid, a list of landmines is generated by running this file and bulid -> build https://chromiumcodereview.appspot.com/11175016/diff/1/build/landmines.py#new... build/landmines.py:18: import json line 18 before line 17 https://chromiumcodereview.appspot.com/11175016/diff/1/build/landmines.py#new... build/landmines.py:20: # Expect no args, JSON on stdin append a period https://chromiumcodereview.appspot.com/11175016/diff/1/build/landmines.py#new... build/landmines.py:21: assert len(sys.argv) == 1 would be better to output usage text here, and print something like lines 24-31 as an example for a user https://chromiumcodereview.appspot.com/11175016/diff/1/build/landmines.py#new... build/landmines.py:33: def o(key, default=''): o is very bare, how about opt instead https://chromiumcodereview.appspot.com/11175016/diff/1/build/landmines.py#new... build/landmines.py:36: MSVS_VERSION = o('msvs', {}).get('version', '') this line is not used, let's remove it https://chromiumcodereview.appspot.com/11175016/diff/1/build/landmines.py#new... build/landmines.py:38: # Now for the good part let's ditch this comment https://chromiumcodereview.appspot.com/11175016/diff/1/build/landmines.py#new... build/landmines.py:41: o('builder') == 'ninja': to span multiple lines, we prefer to prepend a parens to the conditional beginning on line 40, drop the \, and append a parens to the end of line 41 https://chromiumcodereview.appspot.com/11175016/diff/1/build/landmines.py#new... build/landmines.py:42: print 'Need to clobber winja goma due to backend cwd cache fix' append a period to the end of the sentence here let's call sys.exit(1) in this if after line 42 i'd prefer it if you encapsulated lines 40-42+ into a main() method, then called it via something like: if __name__ == 'main': sys.exit(main())
Alright, how about this version? All the logic has moved into landmines.py, which runs as a gclient hook, and places .landmine_triggered files into any output directories (e.g. in out/Release and out/Debug) which need clobbering. The android build script and compile.py simply check for the presence of the file, and clobber if it's there. The trigger file actually contains the output of the diff, which allows the actual reasons for the trigger at the time that the build actually makes a decision. In a separate cl, I'll add a phony gyp action which warns individual devs about any landmines that were triggered in their working copy at the time that they build (so if it's a debug-only trigger, and they're building release, gclient won't bug them.)
On 2012/10/24 00:09:15, iannucci wrote: > Alright, how about this version? > > All the logic has moved into landmines.py, which runs as a gclient hook, and > places .landmine_triggered files into any output directories (e.g. in > out/Release and out/Debug) which need clobbering. > > The android build script and compile.py simply check for the presence of the > file, and clobber if it's there. > > The trigger file actually contains the output of the diff, which allows the > actual reasons for the trigger at the time that the build actually makes a > decision. ^^ Allows actual reasons for the trigger **to be printed** at the time... > > In a separate cl, I'll add a phony gyp action which warns individual devs about > any landmines that were triggered in their working copy at the time that they > build (so if it's a debug-only trigger, and they're building release, gclient > won't bug them.)
Purely style nits. https://chromiumcodereview.appspot.com/11175016/diff/4/build/gyp_helper.py File build/gyp_helper.py (right): https://chromiumcodereview.appspot.com/11175016/diff/4/build/gyp_helper.py#ne... build/gyp_helper.py:1: import os Copyright line. https://chromiumcodereview.appspot.com/11175016/diff/4/build/gyp_helper.py#ne... build/gyp_helper.py:3: def apply_gyp_environment(file_path=None): why =None? https://chromiumcodereview.appspot.com/11175016/diff/4/build/gyp_helper.py#ne... build/gyp_helper.py:4: """ In general we start the comment right after """. https://chromiumcodereview.appspot.com/11175016/diff/4/build/gyp_helper.py#ne... build/gyp_helper.py:24: val = file_data.get(var) val&var are confusing, find a better variable name? https://chromiumcodereview.appspot.com/11175016/diff/4/build/gyp_helper.py#ne... build/gyp_helper.py:27: print 'INFO: Environment value for "%s" overrides value in %s.' % ( Is this necessary? Because otherwise you could do: os.environ.setdefault(var, val) Or you can look at the return value of setdefault(). https://chromiumcodereview.appspot.com/11175016/diff/4/build/gyp_helper.py#ne... build/gyp_helper.py:33: def apply_chromium_gyp_env(): The function names are really confusing. https://chromiumcodereview.appspot.com/11175016/diff/4/build/gyp_helper.py#ne... build/gyp_helper.py:34: script_dir = os.path.dirname(os.path.realpath(__file__)) You can only do that while the module is being parsed. Afterward you don't know if someone called os.chdir(). Make it a global variable. https://chromiumcodereview.appspot.com/11175016/diff/4/build/gyp_helper.py#ne... build/gyp_helper.py:41: Remove extraneous line. https://chromiumcodereview.appspot.com/11175016/diff/4/build/landmines.py File build/landmines.py (right): https://chromiumcodereview.appspot.com/11175016/diff/4/build/landmines.py#new... build/landmines.py:29: def memoize(default=''): Please a one-liner docstring. It's a nice trick though, never thought about that. I don't think '' is a good default, I'd prefer None. https://chromiumcodereview.appspot.com/11175016/diff/4/build/landmines.py#new... build/landmines.py:37: print '%s -> %r' % (func.__name__, val[0]) Was this for debugging purpose? At best you'd use logging, but I think it's just better to remove it. https://chromiumcodereview.appspot.com/11175016/diff/4/build/landmines.py#new... build/landmines.py:110: ''' Consistent docstring style, we setled for """ https://chromiumcodereview.appspot.com/11175016/diff/4/build/landmines.py#new... build/landmines.py:174: 2 lines
Nit fixes. Isaac, could you look at the android buildscript once more? I moved the gyp stuff to be with the rest of the environment setup, and I manually invoke the landmines.py script after setting the gyp environment. This will end up calling landmines twice (once in env setup and once in gclient as a hook), but since it runs pretty much instantaneously, I think that should be OK. compile.py is fine with landmines running as a hook, since clobber happens after hooks run (which Isaac pointed out, is slightly crazy :). When we get a real clobber phase, landmines can probably be removed from the hooks and will be explicitly invoked by the clobber step. Or something like that. https://chromiumcodereview.appspot.com/11175016/diff/4/build/gyp_helper.py File build/gyp_helper.py (right): https://chromiumcodereview.appspot.com/11175016/diff/4/build/gyp_helper.py#ne... build/gyp_helper.py:1: import os On 2012/10/24 00:40:58, Marc-Antoine Ruel wrote: > Copyright line. Done https://chromiumcodereview.appspot.com/11175016/diff/4/build/gyp_helper.py#ne... build/gyp_helper.py:3: def apply_gyp_environment(file_path=None): On 2012/10/24 00:40:58, Marc-Antoine Ruel wrote: > why =None? This whole function was transplanted verbatim from gyp_chromium. Not sure why it was =None there either. Weird. https://chromiumcodereview.appspot.com/11175016/diff/4/build/gyp_helper.py#ne... build/gyp_helper.py:4: """ On 2012/10/24 00:40:58, Marc-Antoine Ruel wrote: > In general we start the comment right after """. Done https://chromiumcodereview.appspot.com/11175016/diff/4/build/gyp_helper.py#ne... build/gyp_helper.py:9: file_contents = open(file_path).read() I went ahead and put a 'with' on this... No reason to wait for python to close this handle. https://chromiumcodereview.appspot.com/11175016/diff/4/build/gyp_helper.py#ne... build/gyp_helper.py:24: val = file_data.get(var) On 2012/10/24 00:40:58, Marc-Antoine Ruel wrote: > val&var are confusing, find a better variable name? Better? https://chromiumcodereview.appspot.com/11175016/diff/4/build/gyp_helper.py#ne... build/gyp_helper.py:27: print 'INFO: Environment value for "%s" overrides value in %s.' % ( On 2012/10/24 00:40:58, Marc-Antoine Ruel wrote: > Is this necessary? Because otherwise you could do: > os.environ.setdefault(var, val) > > Or you can look at the return value of setdefault(). Again, moved from gyp_chromium :D. Did this have a purpose there? I'm assuming yes? https://chromiumcodereview.appspot.com/11175016/diff/4/build/gyp_helper.py#ne... build/gyp_helper.py:33: def apply_chromium_gyp_env(): On 2012/10/24 00:40:58, Marc-Antoine Ruel wrote: > The function names are really confusing. New ones better? https://chromiumcodereview.appspot.com/11175016/diff/4/build/gyp_helper.py#ne... build/gyp_helper.py:34: script_dir = os.path.dirname(os.path.realpath(__file__)) On 2012/10/24 00:40:58, Marc-Antoine Ruel wrote: > You can only do that while the module is being parsed. Afterward you don't know > if someone called os.chdir(). Make it a global variable. Good catch https://chromiumcodereview.appspot.com/11175016/diff/4/build/landmines.py File build/landmines.py (right): https://chromiumcodereview.appspot.com/11175016/diff/4/build/landmines.py#new... build/landmines.py:29: def memoize(default=''): On 2012/10/24 00:40:58, Marc-Antoine Ruel wrote: > Please a one-liner docstring. It's a nice trick though, never thought about > that. I don't think '' is a good default, I'd prefer None. I have '' as the default because get_landmines expects strings back from all these guys, though looking at it now, I expect a lot less .startswith usage. I'll make it None. https://chromiumcodereview.appspot.com/11175016/diff/4/build/landmines.py#new... build/landmines.py:37: print '%s -> %r' % (func.__name__, val[0]) On 2012/10/24 00:40:58, Marc-Antoine Ruel wrote: > Was this for debugging purpose? At best you'd use logging, but I think it's just > better to remove it. I'd like this to show up in the buildbot logs (very useful for debugging)... is there a better way to do that? https://chromiumcodereview.appspot.com/11175016/diff/4/build/landmines.py#new... build/landmines.py:110: ''' On 2012/10/24 00:40:58, Marc-Antoine Ruel wrote: > Consistent docstring style, we setled for """ And just when I was getting good at using single quotes... ;D
Looking forward to this CL! Some style comments inline. Bigger picture questions: - Is this going to get run twice on android when we gyp in gclient runhooks? - How are you going to get variables for android (i.e. pre-gyp)? A better strategy might be to look at factory_properties & build_properties on all platforms, so you don't depend on gyp. Then at a later point we could move this to before the gyp phase. https://chromiumcodereview.appspot.com/11175016/diff/27001/build/android/buil... File build/android/buildbot/buildbot_functions.sh (right): https://chromiumcodereview.appspot.com/11175016/diff/27001/build/android/buil... build/android/buildbot/buildbot_functions.sh:67: local out_path="${SRC_ROOT}/out/${BUILDTYPE}" let's move this to where it is used. Let's call it build_path. https://chromiumcodereview.appspot.com/11175016/diff/27001/build/android/buil... build/android/buildbot/buildbot_functions.sh:68: local trigger_path="$out_path/.landmines_triggered" how about landmine_file_path ? https://chromiumcodereview.appspot.com/11175016/diff/27001/build/android/buil... build/android/buildbot/buildbot_functions.sh:82: find "${SRC_ROOT}/out" -maxdepth 1 -type f -exec rm \{\} \+ find "${SRC_ROOT}/out" -maxdepth 1 -type f -exec rm -f {} \; https://chromiumcodereview.appspot.com/11175016/diff/27001/build/android/buil... build/android/buildbot/buildbot_functions.sh:89: if [ -e "$out_path" ] ; then switch this to [[ -e $out_path ]] (quotes around variables are not needed for double bracket tests) https://chromiumcodereview.appspot.com/11175016/diff/27001/build/landmines.py File build/landmines.py (right): https://chromiumcodereview.appspot.com/11175016/diff/27001/build/landmines.py... build/landmines.py:75: def platform(): Please document all of these functions. https://chromiumcodereview.appspot.com/11175016/diff/27001/build/landmines.py... build/landmines.py:90: def builder(): What does this function do? Some android bots use make-android and some use ninja. You can find out which one you're on by parsing factory properties.
CL style is fine with me, I'll leave Isaac to do the proper review. https://chromiumcodereview.appspot.com/11175016/diff/4/build/gyp_helper.py File build/gyp_helper.py (right): https://chromiumcodereview.appspot.com/11175016/diff/4/build/gyp_helper.py#ne... build/gyp_helper.py:27: print 'INFO: Environment value for "%s" overrides value in %s.' % ( On 2012/10/24 04:12:25, iannucci wrote: > On 2012/10/24 00:40:58, Marc-Antoine Ruel wrote: > > Is this necessary? Because otherwise you could do: > > os.environ.setdefault(var, val) > > > > Or you can look at the return value of setdefault(). > > Again, moved from gyp_chromium :D. Did this have a purpose there? I'm assuming > yes? No idea. I don't mind though. https://chromiumcodereview.appspot.com/11175016/diff/4/build/landmines.py File build/landmines.py (right): https://chromiumcodereview.appspot.com/11175016/diff/4/build/landmines.py#new... build/landmines.py:37: print '%s -> %r' % (func.__name__, val[0]) On 2012/10/24 04:12:25, iannucci wrote: > On 2012/10/24 00:40:58, Marc-Antoine Ruel wrote: > > Was this for debugging purpose? At best you'd use logging, but I think it's > just > > better to remove it. > > I'd like this to show up in the buildbot logs (very useful for debugging)... is > there a better way to do that? I just fear spew for devs. You can leave it in and I'll see if I like/hate it. https://chromiumcodereview.appspot.com/11175016/diff/27001/build/gyp_helper.py File build/gyp_helper.py (right): https://chromiumcodereview.appspot.com/11175016/diff/27001/build/gyp_helper.p... build/gyp_helper.py:12: 2 lines https://chromiumcodereview.appspot.com/11175016/diff/27001/build/gyp_helper.p... build/gyp_helper.py:41: 2 lines
On 2012/10/24 04:54:07, Isaac wrote: > Looking forward to this CL! Some style comments inline. > > Bigger picture questions: > > - Is this going to get run twice on android when we gyp in gclient runhooks? Yeah I think I mentioned this? > - How are you going to get variables for android (i.e. pre-gyp)? A better > strategy might be to look at factory_properties & build_properties on all > platforms, so you don't depend on gyp. Then at a later point we could move this > to before the gyp phase. > Devs don't have *_properties. Also, this cl moves all the gyp env setup before calling the script (at least as far as I can tell. Did I miss something?) Will go through comments now. > https://chromiumcodereview.appspot.com/11175016/diff/27001/build/android/buil... > File build/android/buildbot/buildbot_functions.sh (right): > > https://chromiumcodereview.appspot.com/11175016/diff/27001/build/android/buil... > build/android/buildbot/buildbot_functions.sh:67: local > out_path="${SRC_ROOT}/out/${BUILDTYPE}" > let's move this to where it is used. Let's call it build_path. > > https://chromiumcodereview.appspot.com/11175016/diff/27001/build/android/buil... > build/android/buildbot/buildbot_functions.sh:68: local > trigger_path="$out_path/.landmines_triggered" > how about landmine_file_path ? > > https://chromiumcodereview.appspot.com/11175016/diff/27001/build/android/buil... > build/android/buildbot/buildbot_functions.sh:82: find "${SRC_ROOT}/out" > -maxdepth 1 -type f -exec rm \{\} \+ > find "${SRC_ROOT}/out" -maxdepth 1 -type f -exec rm -f {} \; > > https://chromiumcodereview.appspot.com/11175016/diff/27001/build/android/buil... > build/android/buildbot/buildbot_functions.sh:89: if [ -e "$out_path" ] ; then > switch this to [[ -e $out_path ]] (quotes around variables are not needed for > double bracket tests) > > https://chromiumcodereview.appspot.com/11175016/diff/27001/build/landmines.py > File build/landmines.py (right): > > https://chromiumcodereview.appspot.com/11175016/diff/27001/build/landmines.py... > build/landmines.py:75: def platform(): > Please document all of these functions. > > https://chromiumcodereview.appspot.com/11175016/diff/27001/build/landmines.py... > build/landmines.py:90: def builder(): > What does this function do? Some android bots use make-android and some use > ninja. You can find out which one you're on by parsing factory properties.
You missed gclient runhooks. But you cannot change the clobber on android to happen after gyp. -1
OK I misunderstood this. The android stuff is OK. Let's fix comments and I'll take another look. https://chromiumcodereview.appspot.com/11175016/diff/27001/build/android/buil... File build/android/buildbot/buildbot_functions.sh (right): https://chromiumcodereview.appspot.com/11175016/diff/27001/build/android/buil... build/android/buildbot/buildbot_functions.sh:77: if [[ -f "$trigger_path" ]]; then This should check [[ -z $BUILDBOT_CLOBBER ]] instead of checking if trigger_path is set so if both conditions are met it runs the find command. https://chromiumcodereview.appspot.com/11175016/diff/27001/build/landmines.py File build/landmines.py (right): https://chromiumcodereview.appspot.com/11175016/diff/27001/build/landmines.py... build/landmines.py:168: diff = difflib.unified_diff(old_landmines, new_landmines, Why not read these files into python, call splitlines(), and then use a set diff operation? Why do you need to depend on file stats?
Fixed a bunch of stuff... Had some questions inline. Thanks, Rob https://chromiumcodereview.appspot.com/11175016/diff/27001/build/android/buil... File build/android/buildbot/buildbot_functions.sh (right): https://chromiumcodereview.appspot.com/11175016/diff/27001/build/android/buil... build/android/buildbot/buildbot_functions.sh:67: local out_path="${SRC_ROOT}/out/${BUILDTYPE}" On 2012/10/24 04:54:07, Isaac wrote: > let's move this to where it is used. Let's call it build_path. Done. https://chromiumcodereview.appspot.com/11175016/diff/27001/build/android/buil... build/android/buildbot/buildbot_functions.sh:68: local trigger_path="$out_path/.landmines_triggered" On 2012/10/24 04:54:07, Isaac wrote: > how about landmine_file_path ? I went for an even more bland name :) https://chromiumcodereview.appspot.com/11175016/diff/27001/build/android/buil... build/android/buildbot/buildbot_functions.sh:77: if [[ -f "$trigger_path" ]]; then On 2012/10/24 19:23:29, Isaac wrote: > This should check [[ -z $BUILDBOT_CLOBBER ]] instead of checking if trigger_path > is set so if both conditions are met it runs the find command. Makes sense. Done. https://chromiumcodereview.appspot.com/11175016/diff/27001/build/android/buil... build/android/buildbot/buildbot_functions.sh:82: find "${SRC_ROOT}/out" -maxdepth 1 -type f -exec rm \{\} \+ On 2012/10/24 04:54:07, Isaac wrote: > find "${SRC_ROOT}/out" -maxdepth 1 -type f -exec rm -f {} \; -f is good. I vastly prefer +, unless you feel strongly about invoking rm once per file instead of passing it all the files to delete? https://chromiumcodereview.appspot.com/11175016/diff/27001/build/android/buil... build/android/buildbot/buildbot_functions.sh:89: if [ -e "$out_path" ] ; then On 2012/10/24 04:54:07, Isaac wrote: > switch this to [[ -e $out_path ]] (quotes around variables are not needed for > double bracket tests) Done. https://chromiumcodereview.appspot.com/11175016/diff/27001/build/landmines.py File build/landmines.py (right): https://chromiumcodereview.appspot.com/11175016/diff/27001/build/landmines.py... build/landmines.py:75: def platform(): On 2012/10/24 04:54:07, Isaac wrote: > Please document all of these functions. But... but... self documenting code! j/k :D Done https://chromiumcodereview.appspot.com/11175016/diff/27001/build/landmines.py... build/landmines.py:90: def builder(): On 2012/10/24 04:54:07, Isaac wrote: > What does this function do? Some android bots use make-android and some use > ninja. You can find out which one you're on by parsing factory properties. Blehh... I don't want to write bot-only code here. It seems to me that it's a bug that 'generators' has 'android' in it at all. It should be generator==whatever and OS=android. (like on ios). I added some code to work around this conflation. Is ok? https://chromiumcodereview.appspot.com/11175016/diff/27001/build/landmines.py... build/landmines.py:168: diff = difflib.unified_diff(old_landmines, new_landmines, On 2012/10/24 19:23:29, Isaac wrote: > Why not read these files into python, call splitlines(), and then use a set diff > operation? Why do you need to depend on file stats? Well... This is actually doing essentially that (but using python's stdlib code instead of set math). old_landmines and new_landmines are lists of strings. The timestamps are just flavor for the diff output. That way you can see when the old landmines were created. N=0 makes a minimal diff (without context lines), since we don't need to use the diff for patches. I guess the set code to do the same would be: (old - new) & (new - old) (need to show additions as well as subtractions) but using sets is not order-preserving, which I think would make the output more confusing. I think using a diff is pretty logical here, since that's essentially what we're triggering on anyway, and devs are pretty used to looking at diff outputs. Unless you're saying that there is a problem with difflib? It's been in the python standard library since 2.1 (unified_diff since 2.3), so I'd imagine it works well by now.
My question is more -- why do we care about ordering? I would think non-ordered sets would be more consistent. btw, in python you can do a symmetric diff in one operation: a ^ b
Oh how about that! Didn't know about that... xor op makes sense :) I think I'd still need to do both ops explicitly, since I need to annotate which lines are added and which are removed. On the other hand, if we preserve order, if would let us add reasons that span multiple lines. That's my last excuse, I promise :D. If you still think that set math would be better, I'll do that. On Wed, Oct 24, 2012 at 5:15 PM, <ilevy@chromium.org> wrote: > My question is more -- why do we care about ordering? I would think > non-ordered > sets would be more consistent. > > btw, in python you can do a symmetric diff in one operation: a ^ b > > https://chromiumcodereview.**appspot.com/11175016/<https://chromiumcodereview... >
Sounds like you have good reasons to use diff, so that sgtm. Will let maruel / cmp do the final review but lgtm. https://chromiumcodereview.appspot.com/11175016/diff/20005/build/landmines.py File build/landmines.py (right): https://chromiumcodereview.appspot.com/11175016/diff/20005/build/landmines.py... build/landmines.py:196: os.unlink(triggered) nit: os.remove is the more common name for this fun.
https://chromiumcodereview.appspot.com/11175016/diff/26002/build/gyp_helper.py File build/gyp_helper.py (right): https://chromiumcodereview.appspot.com/11175016/diff/26002/build/gyp_helper.p... build/gyp_helper.py:11: CHROME_SRC = os.path.abspath(os.path.join(SCRIPT_DIR, os.pardir)) CHROME_SRC = os.path.dirname(SCRIPT_DIR) https://chromiumcodereview.appspot.com/11175016/diff/26002/build/landmines.py File build/landmines.py (right): https://chromiumcodereview.appspot.com/11175016/diff/26002/build/landmines.py... build/landmines.py:144: Get absolute path to the output directory dependant on build and target. Returns output directory absolute path dependent on build and targets. http://www.grammar-monster.com/easily_confused/dependant_dependent.htm https://chromiumcodereview.appspot.com/11175016/diff/26002/build/landmines.py... build/landmines.py:151: if build_tool == 'xcode': My feeling: you should destroy them all. Much simpler. https://chromiumcodereview.appspot.com/11175016/diff/26002/build/landmines.py... build/landmines.py:161: ret = os.path.join(src_dir, 'sconsbuild', target) else: raise NotImplementError() https://chromiumcodereview.appspot.com/11175016/diff/26002/build/landmines.py... build/landmines.py:165: def main(): if len(sys.argv) > 1: print('Unknown argument %s' % sys.argv[1:]) return 1 https://chromiumcodereview.appspot.com/11175016/diff/26002/build/landmines.py... build/landmines.py:167: src_dir = os.path.dirname(os.path.dirname(os.path.realpath(__file__))) Use a global variable.
https://chromiumcodereview.appspot.com/11175016/diff/26002/build/gyp_helper.py File build/gyp_helper.py (right): https://chromiumcodereview.appspot.com/11175016/diff/26002/build/gyp_helper.p... build/gyp_helper.py:11: CHROME_SRC = os.path.abspath(os.path.join(SCRIPT_DIR, os.pardir)) On 2012/10/29 15:10:43, Marc-Antoine Ruel wrote: > CHROME_SRC = os.path.dirname(SCRIPT_DIR) *facepalm* Done. https://chromiumcodereview.appspot.com/11175016/diff/26002/build/landmines.py File build/landmines.py (right): https://chromiumcodereview.appspot.com/11175016/diff/26002/build/landmines.py... build/landmines.py:144: Get absolute path to the output directory dependant on build and target. On 2012/10/29 15:10:43, Marc-Antoine Ruel wrote: > Returns output directory absolute path dependent on build and targets. > > http://www.grammar-monster.com/easily_confused/dependant_dependent.htm Done. https://chromiumcodereview.appspot.com/11175016/diff/26002/build/landmines.py... build/landmines.py:151: if build_tool == 'xcode': On 2012/10/29 15:10:43, Marc-Antoine Ruel wrote: > My feeling: you should destroy them all. Much simpler. No support for anything else? I still need xcode in any event (for iphone). The only things that would die would be scons, since we still use make and msvs (at least for now, bwahahahah). https://chromiumcodereview.appspot.com/11175016/diff/26002/build/landmines.py... build/landmines.py:161: ret = os.path.join(src_dir, 'sconsbuild', target) On 2012/10/29 15:10:43, Marc-Antoine Ruel wrote: > else: > raise NotImplementError() Done. https://chromiumcodereview.appspot.com/11175016/diff/26002/build/landmines.py... build/landmines.py:165: def main(): On 2012/10/29 15:10:43, Marc-Antoine Ruel wrote: > if len(sys.argv) > 1: > print('Unknown argument %s' % sys.argv[1:]) > return 1 A lot of words for: assert len(sys.argv) == 1, 'Unknown argument %s' % sys.argv[1:] ;). Done. https://chromiumcodereview.appspot.com/11175016/diff/26002/build/landmines.py... build/landmines.py:167: src_dir = os.path.dirname(os.path.dirname(os.path.realpath(__file__))) On 2012/10/29 15:10:43, Marc-Antoine Ruel wrote: > Use a global variable. Done.
The code looks good beside one last interrogation https://chromiumcodereview.appspot.com/11175016/diff/26002/build/landmines.py File build/landmines.py (right): https://chromiumcodereview.appspot.com/11175016/diff/26002/build/landmines.py... build/landmines.py:151: if build_tool == 'xcode': On 2012/10/29 23:56:39, iannucci wrote: > On 2012/10/29 15:10:43, Marc-Antoine Ruel wrote: > > My feeling: you should destroy them all. Much simpler. > > No support for anything else? I still need xcode in any event (for iphone). > > The only things that would die would be scons, since we still use make and msvs > (at least for now, bwahahahah). Sorry I wasn't clear, I meant: delete all of these directories unconditionally. It's safer. For example, I may well build *both* with msvs and ninja.
On 2012/10/30 00:53:09, Marc-Antoine Ruel wrote: > The code looks good beside one last interrogation > > https://chromiumcodereview.appspot.com/11175016/diff/26002/build/landmines.py > File build/landmines.py (right): > > https://chromiumcodereview.appspot.com/11175016/diff/26002/build/landmines.py... > build/landmines.py:151: if build_tool == 'xcode': > On 2012/10/29 23:56:39, iannucci wrote: > > On 2012/10/29 15:10:43, Marc-Antoine Ruel wrote: > > > My feeling: you should destroy them all. Much simpler. > > > > No support for anything else? I still need xcode in any event (for iphone). > > > > The only things that would die would be scons, since we still use make and > msvs > > (at least for now, bwahahahah). > > Sorry I wasn't clear, I meant: delete all of these directories unconditionally. > It's safer. For example, I may well build *both* with msvs and ninja. Oh I see. Well, in the actual example I started out with, the issue was only affecting ninja. It only seems like it's safer if msvs and ninja actually rely on each other's outputs, which, if true, seems like a bug, more than a reason to cross-clobber build types. I'm inclined to keep it in... If there is really an issue which targets both build systems, then it's still possible to target both with the same landmine. R
On 2012/10/31 01:14:55, iannucci wrote: > On 2012/10/30 00:53:09, Marc-Antoine Ruel wrote: > > The code looks good beside one last interrogation > > > > https://chromiumcodereview.appspot.com/11175016/diff/26002/build/landmines.py > > File build/landmines.py (right): > > > > > https://chromiumcodereview.appspot.com/11175016/diff/26002/build/landmines.py... > > build/landmines.py:151: if build_tool == 'xcode': > > On 2012/10/29 23:56:39, iannucci wrote: > > > On 2012/10/29 15:10:43, Marc-Antoine Ruel wrote: > > > > My feeling: you should destroy them all. Much simpler. > > > > > > No support for anything else? I still need xcode in any event (for iphone). > > > > > > The only things that would die would be scons, since we still use make and > > msvs > > > (at least for now, bwahahahah). > > > > Sorry I wasn't clear, I meant: delete all of these directories > unconditionally. > > It's safer. For example, I may well build *both* with msvs and ninja. > > Oh I see. Well, in the actual example I started out with, the issue was only > affecting ninja. > > It only seems like it's safer if msvs and ninja actually rely on each other's > outputs, which, if true, seems like a bug, more than a reason to cross-clobber > build types. > > I'm inclined to keep it in... If there is really an issue which targets both > build systems, then it's still possible to target both with the same landmine. > > R M-A, is this good to go, or do you really want me to make the only behavior be to Nuke All the Things? Thanks, R
lgtm, I don't mind.
https://chromiumcodereview.appspot.com/11175016/diff/35001/build/landmines.py File build/landmines.py (right): https://chromiumcodereview.appspot.com/11175016/diff/35001/build/landmines.py... build/landmines.py:31: """This decorator caches the return value of a parameterless pure function""" parameterless is not true in fact, it does accept parameters and doesn't use a dict to cache properly.
https://chromiumcodereview.appspot.com/11175016/diff/35001/build/landmines.py File build/landmines.py (right): https://chromiumcodereview.appspot.com/11175016/diff/35001/build/landmines.py... build/landmines.py:31: """This decorator caches the return value of a parameterless pure function""" On 2012/11/05 16:54:07, Marc-Antoine Ruel wrote: > parameterless is not true in fact, it does accept parameters and doesn't use a > dict to cache properly. So I wrote for another project: def memoize(func): """This decorator caches the return value of a function""" returns = {} @functools.wraps(func) def inner(*args, **kwargs): key = tuple(list(args) + [(k, kwargs[k]) for k in sorted(kwargs)]) if key not in returns: returns[key] = func(*args, **kwargs) print('%s(%s) -> %s' % (func.__name__, key, returns[key])) return returns[key] return inner and realized that it doesn't work for bound member function. Dang.
On 2012/11/05 17:40:22, Marc-Antoine Ruel wrote:
> and realized that it doesn't work for bound member function. Dang.
Ah forget it, it was because I was doing:
class Bar:
def foo(self): pass
class Baz:
def pouet(self):
self.zing = memoize(Bar().foo)
It failed because self.zing() bounded the memoized function to Baz. Oh well. :)
https://chromiumcodereview.appspot.com/11175016/diff/35001/build/landmines.py File build/landmines.py (right): https://chromiumcodereview.appspot.com/11175016/diff/35001/build/landmines.py... build/landmines.py:31: """This decorator caches the return value of a parameterless pure function""" On 2012/11/05 17:40:22, Marc-Antoine Ruel wrote: > On 2012/11/05 16:54:07, Marc-Antoine Ruel wrote: > > parameterless is not true in fact, it does accept parameters and doesn't use a > > dict to cache properly. > > So I wrote for another project: > > def memoize(func): > """This decorator caches the return value of a function""" > returns = {} > @functools.wraps(func) > def inner(*args, **kwargs): > key = tuple(list(args) + [(k, kwargs[k]) for k in sorted(kwargs)]) > if key not in returns: > returns[key] = func(*args, **kwargs) > print('%s(%s) -> %s' % (func.__name__, key, returns[key])) > return returns[key] > return inner > > and realized that it doesn't work for bound member function. Dang. Oh! You're right, it's taking parameters right now... I'll fix that (it's not used with any functions which need parameters, so there's no need to write it that way :). Good catch, will fix shortly.
Last comment fixed. Good to CQ?
lgtm
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/iannucci@chromium.org/11175016/41002
Change committed as 166085
fyi, the .landmines files killed some of the webkit tests. I had to manually delete them on the builders even after the patch was rolled out. If you reland this, please make sure that the 7zip on windows can handling unzipping them
Yeah I will. It's actually extract_build that's failing because it assumes that if it extracts something with 0 size, then that's an error. I'm planning on fixing extract build to match size against the size in the archive header instead. On Tue, Nov 6, 2012 at 7:50 AM, <jochen@chromium.org> wrote: > fyi, the .landmines files killed some of the webkit tests. I had to > manually > delete them on the builders even after the patch was rolled out. > > If you reland this, please make sure that the 7zip on windows can handling > unzipping them > > https://chromiumcodereview.**appspot.com/11175016/<https://chromiumcodereview... >
On 2012/11/06 20:44:16, iannucci wrote: > Yeah I will. It's actually extract_build that's failing because it assumes > that if it extracts something with 0 size, then that's an error. I'm > planning on fixing extract build to match size against the size in the > archive header instead. > > > On Tue, Nov 6, 2012 at 7:50 AM, <mailto:jochen@chromium.org> wrote: > > > fyi, the .landmines files killed some of the webkit tests. I had to > > manually > > delete them on the builders even after the patch was rolled out. > > > > If you reland this, please make sure that the 7zip on windows can handling > > unzipping them > > > > > https://chromiumcodereview.**appspot.com/11175016/%3Chttps://chromiumcoderevi...> > > The extract_build issue has been fixed. I just rebased and added a note to keep landmines.py:get_target_build_dir in sync with it's double in tools/build/scripts/slave/compile.py. PTAL? I'll cq this with the tools/build cl at the same time when that one gets lgtm'd
Please use fresh issue number to reland CL.
On 2012/11/13 20:24:44, Isaac wrote: > Please use fresh issue number to reland CL. New cl here: https://codereview.chromium.org/11377141 |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
