|
|
Created:
4 years, 4 months ago by Yoshisato Yanagisawa Modified:
4 years, 4 months ago CC:
chromium-reviews, ghost stip (do not use), Nico Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionImport compare_build_artifacts.py from isolate recipe module.
Build determinism is necessary for improving try performance.
If two test binaries are the same, we usually do not need to run the test.
The imported script checks the two builds are the same and shows
error if they are different.
The script is imported to make it easy for chromium developers to
update the script and blacklist json to keep the script updated.
BUG=314403
Committed: https://crrev.com/6ff5a0f7921217fdfe82ea60f67cdfd86028a871
Cr-Commit-Position: refs/heads/master@{#413016}
Patch Set 1 #
Total comments: 9
Patch Set 2 : updated whitelist, added todo, auto select target. #Patch Set 3 : updated Android. #
Messages
Total messages: 17 (7 generated)
yyanagisawa@chromium.org changed reviewers: + maruel@chromium.org, thakis@chromium.org
+Mike about android files. lgtm with comments about a potential follow up change now that we switched to GN but this is not blocking this CL. https://codereview.chromium.org/2232723002/diff/1/tools/compare_build_artifac... File tools/compare_build_artifacts/compare_build_artifacts.py (right): https://codereview.chromium.org/2232723002/diff/1/tools/compare_build_artifac... tools/compare_build_artifacts/compare_build_artifacts.py:28: 'd8', Mike, is it true? https://codereview.chromium.org/2232723002/diff/1/tools/compare_build_artifac... tools/compare_build_artifacts/compare_build_artifacts.py:326: 'accessibility_unittests.exe', Expect the list to have to be updated. https://codereview.chromium.org/2232723002/diff/1/tools/compare_build_artifac... tools/compare_build_artifacts/compare_build_artifacts.py:495: ('', '.apk', '.app', '.dll', '.dylib', '.exe', '.nexe', '.so')) Add: # TODO(maruel): Add '.pdb'. In practice, what we want is to process all the target dependencies as per GN description. Now that we switched to GN, it makes sense to leverage the data there. For example, gn desc out\Release1 //base:base_unittests runtime_deps Then we do not need to use any blacklist, because we effectively switch to a passlist. This is much better. That said, it's not a blocker for this change so this can be done later. https://codereview.chromium.org/2232723002/diff/1/tools/compare_build_artifac... tools/compare_build_artifacts/compare_build_artifacts.py:676: parser.add_option('-t', '--target-platform', help='The target platform.') Small addition while at it: target = { 'darwin': 'mac', 'linux2': 'linux', 'win32': 'win' }.get(sys.platform, sys.platform) default=target, choices=('android', 'mac', 'linux', 'win') this way, it does get to the right platform most of the time (except android).
https://codereview.chromium.org/2232723002/diff/1/tools/compare_build_artifac... File tools/compare_build_artifacts/compare_build_artifacts.py (right): https://codereview.chromium.org/2232723002/diff/1/tools/compare_build_artifac... tools/compare_build_artifacts/compare_build_artifacts.py:326: 'accessibility_unittests.exe', On 2016/08/10 13:15:08, M-A Ruel wrote: > Expect the list to have to be updated. I am not confident but I have updated the whitelist. I just list up builds that causes difference from recent builds. https://codereview.chromium.org/2232723002/diff/1/tools/compare_build_artifac... tools/compare_build_artifacts/compare_build_artifacts.py:495: ('', '.apk', '.app', '.dll', '.dylib', '.exe', '.nexe', '.so')) On 2016/08/10 13:15:08, M-A Ruel wrote: > Add: > > # TODO(maruel): Add '.pdb'. > > In practice, what we want is to process all the target dependencies as per GN > description. Now that we switched to GN, it makes sense to leverage the data > there. For example, > > gn desc out\Release1 //base:base_unittests runtime_deps > > Then we do not need to use any blacklist, because we effectively switch to a > passlist. This is much better. > > That said, it's not a blocker for this change so this can be done later. Done. https://codereview.chromium.org/2232723002/diff/1/tools/compare_build_artifac... tools/compare_build_artifacts/compare_build_artifacts.py:676: parser.add_option('-t', '--target-platform', help='The target platform.') On 2016/08/10 13:15:09, M-A Ruel wrote: > Small addition while at it: > > target = { > 'darwin': 'mac', 'linux2': 'linux', 'win32': 'win' > }.get(sys.platform, sys.platform) > > default=target, choices=('android', 'mac', 'linux', 'win') > > this way, it does get to the right platform most of the time (except android). Done.
https://codereview.chromium.org/2232723002/diff/1/tools/compare_build_artifac... File tools/compare_build_artifacts/compare_build_artifacts.py (right): https://codereview.chromium.org/2232723002/diff/1/tools/compare_build_artifac... tools/compare_build_artifacts/compare_build_artifacts.py:28: 'd8', On 2016/08/10 13:15:08, M-A Ruel wrote: > Mike, is it true? FYI. As far as I checked with recent Android Deterministic build, only flatc is different.
lgtm https://codereview.chromium.org/2232723002/diff/1/tools/compare_build_artifac... File tools/compare_build_artifacts/compare_build_artifacts.py (right): https://codereview.chromium.org/2232723002/diff/1/tools/compare_build_artifac... tools/compare_build_artifacts/compare_build_artifacts.py:28: 'd8', On 2016/08/17 09:30:21, Yoshisato Yanagisawa wrote: > On 2016/08/10 13:15:08, M-A Ruel wrote: > > Mike, is it true? > > FYI. > As far as I checked with recent Android Deterministic build, only flatc is > different. Ok please update accordingly then commit.
Description was changed from ========== Import compare_build_artifacts.py from isolate recipe module. Build determinism is necessary for improving try performance. If two test binaries are the same, we usually do not need to run the test. The imported script checks the two builds are the same and shows error if they are different. The script is imported to make it easy for chromium developers to update the script and blacklist json to keep the script updated. BUG=314403 ========== to ========== Import compare_build_artifacts.py from isolate recipe module. Build determinism is necessary for improving try performance. If two test binaries are the same, we usually do not need to run the test. The imported script checks the two builds are the same and shows error if they are different. The script is imported to make it easy for chromium developers to update the script and blacklist json to keep the script updated. BUG=314403 ==========
yyanagisawa@chromium.org changed reviewers: + dpranke@chromium.org - thakis@chromium.org
Since Nico is OOO until 22th, let me change the reviewer.
lgtm
The CQ bit was checked by yyanagisawa@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from maruel@chromium.org Link to the patchset: https://codereview.chromium.org/2232723002/#ps40001 (title: "updated Android.")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Message was sent while issue was closed.
Description was changed from ========== Import compare_build_artifacts.py from isolate recipe module. Build determinism is necessary for improving try performance. If two test binaries are the same, we usually do not need to run the test. The imported script checks the two builds are the same and shows error if they are different. The script is imported to make it easy for chromium developers to update the script and blacklist json to keep the script updated. BUG=314403 ========== to ========== Import compare_build_artifacts.py from isolate recipe module. Build determinism is necessary for improving try performance. If two test binaries are the same, we usually do not need to run the test. The imported script checks the two builds are the same and shows error if they are different. The script is imported to make it easy for chromium developers to update the script and blacklist json to keep the script updated. BUG=314403 ==========
Message was sent while issue was closed.
Committed patchset #3 (id:40001)
Message was sent while issue was closed.
Description was changed from ========== Import compare_build_artifacts.py from isolate recipe module. Build determinism is necessary for improving try performance. If two test binaries are the same, we usually do not need to run the test. The imported script checks the two builds are the same and shows error if they are different. The script is imported to make it easy for chromium developers to update the script and blacklist json to keep the script updated. BUG=314403 ========== to ========== Import compare_build_artifacts.py from isolate recipe module. Build determinism is necessary for improving try performance. If two test binaries are the same, we usually do not need to run the test. The imported script checks the two builds are the same and shows error if they are different. The script is imported to make it easy for chromium developers to update the script and blacklist json to keep the script updated. BUG=314403 Committed: https://crrev.com/6ff5a0f7921217fdfe82ea60f67cdfd86028a871 Cr-Commit-Position: refs/heads/master@{#413016} ==========
Message was sent while issue was closed.
Patchset 3 (id:??) landed as https://crrev.com/6ff5a0f7921217fdfe82ea60f67cdfd86028a871 Cr-Commit-Position: refs/heads/master@{#413016} |