Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1312483002:
Add an environment variable that will cause md5_check.py to write .stamp file that is debuggable (Closed)
|
Created:
5 years, 4 months ago by agrieve Modified:
5 years, 3 months ago Reviewers:
jbudorick CC:
chromium-reviews, jbudorick+watch_chromium.org, klundberg+watch_chromium.org, yfriedman+watch_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionAdd an environment variable that causes md5_check.py to print what changed
This does change checksum calculation a little, but the semantics are unchanged.
Tested that this change has no impact on build time.
BUG=523420
Committed: https://crrev.com/a3bc51686b8f1d81a4630ffae947abb53580bf65
Cr-Commit-Position: refs/heads/master@{#346943}
Patch Set 1 #
Total comments: 1
Patch Set 2 : record per-file md5s for better diffs. Always record extended info #Patch Set 3 : don't include input_paths in hash #
Total comments: 13
Patch Set 4 : '->" #Patch Set 5 : use colorama #
Total comments: 4
Patch Set 6 : import colorama #Patch Set 7 : Add a period #Messages
Total messages: 17 (2 generated)
agrieve@chromium.org changed reviewers: + jbudorick@chromium.org
What's the motivation here? https://codereview.chromium.org/1312483002/diff/1/build/android/gyp/util/md5_... File build/android/gyp/util/md5_check.py (right): https://codereview.chromium.org/1312483002/diff/1/build/android/gyp/util/md5_... build/android/gyp/util/md5_check.py:92: for line in old_record: self.old_digest = old_record.readline().strip() ?
On 2015/08/23 at 02:31:01, jbudorick wrote: > What's the motivation here? er, I'm guessing it's the dependent CL. > > https://codereview.chromium.org/1312483002/diff/1/build/android/gyp/util/md5_... > File build/android/gyp/util/md5_check.py (right): > > https://codereview.chromium.org/1312483002/diff/1/build/android/gyp/util/md5_... > build/android/gyp/util/md5_check.py:92: for line in old_record: > self.old_digest = old_record.readline().strip() > > ?
On 2015/08/23 02:31:59, jbudorick wrote: > On 2015/08/23 at 02:31:01, jbudorick wrote: > > What's the motivation here? > > er, I'm guessing it's the dependent CL. > > > > > > https://codereview.chromium.org/1312483002/diff/1/build/android/gyp/util/md5_... > > File build/android/gyp/util/md5_check.py (right): > > > > > https://codereview.chromium.org/1312483002/diff/1/build/android/gyp/util/md5_... > > build/android/gyp/util/md5_check.py:92: for line in old_record: > > self.old_digest = old_record.readline().strip() > > > > ? Made some tweaks to this based on further usage. Should be good to go now.
https://codereview.chromium.org/1312483002/diff/40001/build/android/gyp/util/... File build/android/gyp/util/md5_check.py (right): https://codereview.chromium.org/1312483002/diff/40001/build/android/gyp/util/... build/android/gyp/util/md5_check.py:12: _PRINT_MD5_DIFFS = int(os.environ.get('PRINT_MD5_DIFFS', 0)) Environment variables tend to come back to bite us where we use them -- it's very hard to see that they're altering program behavior. This is debug only, so perhaps it's ok, but typically I'd want to see this handled via command-line flag (or, in this case, a function argument controlled by command-line flags elsewhere). https://codereview.chromium.org/1312483002/diff/40001/build/android/gyp/util/... build/android/gyp/util/md5_check.py:42: print '\033[93mDifference found in %s:\033[0m' % record_path It looks like some of the other gyp utilities, notably javac, user colorama for this. https://codereview.chromium.org/1312483002/diff/40001/build/android/gyp/util/... build/android/gyp/util/md5_check.py:118: new_record.write('\n' + '\n'.join(self.new_extended_info) + '\n') so now we're always writing the extended info? https://codereview.chromium.org/1312483002/diff/40001/build/android/gyp/util/... build/android/gyp/util/md5_check.py:122: return 'There\'s no difference' nit: double quotes when you've got an apostrophe.
https://codereview.chromium.org/1312483002/diff/40001/build/android/gyp/util/... File build/android/gyp/util/md5_check.py (right): https://codereview.chromium.org/1312483002/diff/40001/build/android/gyp/util/... build/android/gyp/util/md5_check.py:12: _PRINT_MD5_DIFFS = int(os.environ.get('PRINT_MD5_DIFFS', 0)) On 2015/08/26 16:48:49, jbudorick wrote: > Environment variables tend to come back to bite us where we use them -- it's > very hard to see that they're altering program behavior. > > This is debug only, so perhaps it's ok, but typically I'd want to see this > handled via command-line flag (or, in this case, a function argument controlled > by command-line flags elsewhere). Because a command-line flag would require re-gypping / re-GN'ing, it then causes a rules that use md5_check.py to be marked as dirty, which kind of eliminates the point of the flag (need to be able to turn it on without affecting what's dirty). The only side-effect it enables is extra printing (does not change md5 calculation at all), so I think it's best as a variable. https://codereview.chromium.org/1312483002/diff/40001/build/android/gyp/util/... build/android/gyp/util/md5_check.py:42: print '\033[93mDifference found in %s:\033[0m' % record_path On 2015/08/26 16:48:49, jbudorick wrote: > It looks like some of the other gyp utilities, notably javac, user colorama for > this. I looked into this, but IMO makes it more ugly than it's worth: if force or is_stale: import sys from util import build_utils if build_utils.COLORAMA_ROOT not in sys.path: sys.path.append(build_utils.COLORAMA_ROOT) import colorama print '%sDifference found in %s:%s' % ( colorama.Fore.YELLOW, record_path, colorama.Fore.RESET) print md5_checker.DescribeDifference() https://codereview.chromium.org/1312483002/diff/40001/build/android/gyp/util/... build/android/gyp/util/md5_check.py:118: new_record.write('\n' + '\n'.join(self.new_extended_info) + '\n') On 2015/08/26 16:48:49, jbudorick wrote: > so now we're always writing the extended info? Yes. I measured if this slowed anything down, and it doesn't. https://codereview.chromium.org/1312483002/diff/40001/build/android/gyp/util/... build/android/gyp/util/md5_check.py:122: return 'There\'s no difference' On 2015/08/26 16:48:49, jbudorick wrote: > nit: double quotes when you've got an apostrophe. Done.
https://codereview.chromium.org/1312483002/diff/40001/build/android/gyp/util/... File build/android/gyp/util/md5_check.py (right): https://codereview.chromium.org/1312483002/diff/40001/build/android/gyp/util/... build/android/gyp/util/md5_check.py:12: _PRINT_MD5_DIFFS = int(os.environ.get('PRINT_MD5_DIFFS', 0)) On 2015/08/26 at 17:59:13, agrieve wrote: > On 2015/08/26 16:48:49, jbudorick wrote: > > Environment variables tend to come back to bite us where we use them -- it's > > very hard to see that they're altering program behavior. > > > > This is debug only, so perhaps it's ok, but typically I'd want to see this > > handled via command-line flag (or, in this case, a function argument controlled > > by command-line flags elsewhere). > > Because a command-line flag would require re-gypping / re-GN'ing, it then causes a rules that use md5_check.py to be marked as dirty, which kind of eliminates the point of the flag (need to be able to turn it on without affecting what's dirty). It's dirty on the switch, but not afterwards. Are you getting into states that aren't (easily) reproducible? > > The only side-effect it enables is extra printing (does not change md5 calculation at all), so I think it's best as a variable. https://codereview.chromium.org/1312483002/diff/40001/build/android/gyp/util/... build/android/gyp/util/md5_check.py:42: print '\033[93mDifference found in %s:\033[0m' % record_path On 2015/08/26 at 17:59:13, agrieve wrote: > On 2015/08/26 16:48:49, jbudorick wrote: > > It looks like some of the other gyp utilities, notably javac, user colorama for > > this. > > I looked into this, but IMO makes it more ugly than it's worth: > > if force or is_stale: > import sys > from util import build_utils > if build_utils.COLORAMA_ROOT not in sys.path: > sys.path.append(build_utils.COLORAMA_ROOT) > import colorama > > print '%sDifference found in %s:%s' % ( > colorama.Fore.YELLOW, record_path, colorama.Fore.RESET) > print md5_checker.DescribeDifference() I'm not really interested in doing this in two different ways in this directory, one with hand-rolled symbols and one with a library.
https://codereview.chromium.org/1312483002/diff/40001/build/android/gyp/util/... File build/android/gyp/util/md5_check.py (right): https://codereview.chromium.org/1312483002/diff/40001/build/android/gyp/util/... build/android/gyp/util/md5_check.py:12: _PRINT_MD5_DIFFS = int(os.environ.get('PRINT_MD5_DIFFS', 0)) On 2015/08/26 at 18:02:51, jbudorick wrote: > On 2015/08/26 at 17:59:13, agrieve wrote: > > On 2015/08/26 16:48:49, jbudorick wrote: > > > Environment variables tend to come back to bite us where we use them -- it's > > > very hard to see that they're altering program behavior. > > > > > > This is debug only, so perhaps it's ok, but typically I'd want to see this > > > handled via command-line flag (or, in this case, a function argument controlled > > > by command-line flags elsewhere). > > > > Because a command-line flag would require re-gypping / re-GN'ing, it then causes a rules that use md5_check.py to be marked as dirty, which kind of eliminates the point of the flag (need to be able to turn it on without affecting what's dirty). > > It's dirty on the switch, but not afterwards. Are you getting into states that aren't (easily) reproducible? > > > > > The only side-effect it enables is extra printing (does not change md5 calculation at all), so I think it's best as a variable. Hrm. I guess I can live with this for now. https://codereview.chromium.org/1312483002/diff/40001/build/android/gyp/util/... build/android/gyp/util/md5_check.py:42: print '\033[93mDifference found in %s:\033[0m' % record_path On 2015/08/26 at 18:02:51, jbudorick wrote: > On 2015/08/26 at 17:59:13, agrieve wrote: > > On 2015/08/26 16:48:49, jbudorick wrote: > > > It looks like some of the other gyp utilities, notably javac, user colorama for > > > this. > > > > I looked into this, but IMO makes it more ugly than it's worth: > > > > if force or is_stale: > > import sys > > from util import build_utils > > if build_utils.COLORAMA_ROOT not in sys.path: > > sys.path.append(build_utils.COLORAMA_ROOT) > > import colorama > > > > print '%sDifference found in %s:%s' % ( > > colorama.Fore.YELLOW, record_path, colorama.Fore.RESET) > > print md5_checker.DescribeDifference() > > I'm not really interested in doing this in two different ways in this directory, one with hand-rolled symbols and one with a library. This, on the other hand...
https://codereview.chromium.org/1312483002/diff/40001/build/android/gyp/util/... File build/android/gyp/util/md5_check.py (right): https://codereview.chromium.org/1312483002/diff/40001/build/android/gyp/util/... build/android/gyp/util/md5_check.py:42: print '\033[93mDifference found in %s:\033[0m' % record_path On 2015/09/01 14:59:43, jbudorick wrote: > On 2015/08/26 at 18:02:51, jbudorick wrote: > > On 2015/08/26 at 17:59:13, agrieve wrote: > > > On 2015/08/26 16:48:49, jbudorick wrote: > > > > It looks like some of the other gyp utilities, notably javac, user > colorama for > > > > this. > > > > > > I looked into this, but IMO makes it more ugly than it's worth: > > > > > > if force or is_stale: > > > import sys > > > from util import build_utils > > > if build_utils.COLORAMA_ROOT not in sys.path: > > > sys.path.append(build_utils.COLORAMA_ROOT) > > > import colorama > > > > > > print '%sDifference found in %s:%s' % ( > > > colorama.Fore.YELLOW, record_path, colorama.Fore.RESET) > > > print md5_checker.DescribeDifference() > > > > I'm not really interested in doing this in two different ways in this > directory, one with hand-rolled symbols and one with a library. > > This, on the other hand... Colorama'ed.
https://codereview.chromium.org/1312483002/diff/80001/build/android/gyp/util/... File build/android/gyp/util/md5_check.py (right): https://codereview.chromium.org/1312483002/diff/80001/build/android/gyp/util/... build/android/gyp/util/md5_check.py:42: import sys I would rather avoid local imports. I don't know of anywhere else in build/android/ that does them. https://codereview.chromium.org/1312483002/diff/80001/build/android/gyp/util/... build/android/gyp/util/md5_check.py:129: return "There's no difference" supernit: "... difference."
https://codereview.chromium.org/1312483002/diff/80001/build/android/gyp/util/... File build/android/gyp/util/md5_check.py (right): https://codereview.chromium.org/1312483002/diff/80001/build/android/gyp/util/... build/android/gyp/util/md5_check.py:42: import sys On 2015/09/02 01:13:50, jbudorick wrote: > I would rather avoid local imports. I don't know of anywhere else in > build/android/ that does them. Done. https://codereview.chromium.org/1312483002/diff/80001/build/android/gyp/util/... build/android/gyp/util/md5_check.py:129: return "There's no difference" On 2015/09/02 01:13:50, jbudorick wrote: > supernit: "... difference." Done.
lgtm
The CQ bit was checked by agrieve@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1312483002/120001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1312483002/120001
Message was sent while issue was closed.
Committed patchset #7 (id:120001)
Message was sent while issue was closed.
Patchset 7 (id:??) landed as https://crrev.com/a3bc51686b8f1d81a4630ffae947abb53580bf65 Cr-Commit-Position: refs/heads/master@{#346943} |
