Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2531573003:
Make Valgrind memcheck work in swarming. (Closed)
|
Created:
4 years ago by ehmaldonado_webrtc Modified:
4 years ago Reviewers:
kjellander_webrtc CC:
webrtc-reviews_webrtc.org, tterriberry_mozilla.com, qiang.lu, niklas.enbom, yujie_mao (webrtc), peah-webrtc Target Ref:
refs/pending/heads/master Project:
webrtc Visibility:
Public. |
DescriptionMake Valgrind memcheck work in swarming.
Add a fallback path to tools/valgrind-webrtc/webrtc_tests.sh if locate_valgrind.sh fails.
This is a workaround to run memcheck on swarming, since locate_valgrind.sh fails even though the files are present. This is almost certainly because the way we use symlinks.
A warning message is displayed to warn the developers to follow the instructions to get the valgrind binaries.
Some extra suppressions were needed. The bug tracking them is https://bugs.webrtc.org/6773
R=kjellander@chromium.org
BUG=chromium:497757
Committed: https://crrev.com/0fa164a22c4538b50a2c4cf62aee261fee54e404
Cr-Commit-Position: refs/heads/master@{#15244}
Patch Set 1 #Patch Set 2 : Always include chromium's suppressions. #Patch Set 3 : Show warning message. #
Total comments: 4
Patch Set 4 : Add supression. #Patch Set 5 : Show warning at the top. #Patch Set 6 : Add more suppressions. #
Messages
Total messages: 26 (16 generated)
ehmaldonado@webrtc.org changed reviewers: + kjellander@webrtc.org
The idea is to add a fallback path. The only concern is how to properly warn the developers when they don't have valgrind correctly configured.
Description was changed from ========== stuff BUG= ========== to ========== Add a fallback path to tools/valgrind-webrtc/webrtc_tests.sh if locate_valgrind.sh fails. This is a workaround to run memcheck on swarming, since locate_valgrind.sh fails even though the files are present. This is almost certainly because the way we use symlinks. A warning message is also displayed. R=kjellander@chromium.org BUG=chromium:497757 ==========
Description was changed from ========== Add a fallback path to tools/valgrind-webrtc/webrtc_tests.sh if locate_valgrind.sh fails. This is a workaround to run memcheck on swarming, since locate_valgrind.sh fails even though the files are present. This is almost certainly because the way we use symlinks. A warning message is also displayed. R=kjellander@chromium.org BUG=chromium:497757 ========== to ========== Add a fallback path to tools/valgrind-webrtc/webrtc_tests.sh if locate_valgrind.sh fails. This is a workaround to run memcheck on swarming, since locate_valgrind.sh fails even though the files are present. This is almost certainly because the way we use symlinks. A warning message is displayed to warn the developers to follow the instructions to get the valgrind binaries. R=kjellander@chromium.org BUG=chromium:497757 ==========
Adding fallback is fine, it should only be used on the bots since it's been working so far if developers follow the existing instructions. Not sure why to display the warning twice though. And finally, sorry you had to spend so much time on this, but a full memcheck run in 22 minutes is truly awesome. The regular builds today is at 1h 30min. https://codereview.webrtc.org/2531573003/diff/40001/tools/valgrind-webrtc/val... File tools/valgrind-webrtc/valgrind.gni (right): https://codereview.webrtc.org/2531573003/diff/40001/tools/valgrind-webrtc/val... tools/valgrind-webrtc/valgrind.gni:24: "../../tools/valgrind/gtest_exclude/ash_unittests.gtest-memcheck.txt", but this change didn't really affect the problem, did it? I suggest restoring this file. https://codereview.webrtc.org/2531573003/diff/40001/tools/valgrind-webrtc/web... File tools/valgrind-webrtc/webrtc_tests.sh (right): https://codereview.webrtc.org/2531573003/diff/40001/tools/valgrind-webrtc/web... tools/valgrind-webrtc/webrtc_tests.sh:81: show_locate_valgrind_failed_warning What's the point with showing the warning twice?
Description was changed from ========== Add a fallback path to tools/valgrind-webrtc/webrtc_tests.sh if locate_valgrind.sh fails. This is a workaround to run memcheck on swarming, since locate_valgrind.sh fails even though the files are present. This is almost certainly because the way we use symlinks. A warning message is displayed to warn the developers to follow the instructions to get the valgrind binaries. R=kjellander@chromium.org BUG=chromium:497757 ========== to ========== Make Valgrind memcheck work in swarming. Add a fallback path to tools/valgrind-webrtc/webrtc_tests.sh if locate_valgrind.sh fails. This is a workaround to run memcheck on swarming, since locate_valgrind.sh fails even though the files are present. This is almost certainly because the way we use symlinks. A warning message is displayed to warn the developers to follow the instructions to get the valgrind binaries. R=kjellander@chromium.org BUG=chromium:497757 ==========
Yaaaaay! It worked! https://codereview.webrtc.org/2531573003/diff/40001/tools/valgrind-webrtc/val... File tools/valgrind-webrtc/valgrind.gni (right): https://codereview.webrtc.org/2531573003/diff/40001/tools/valgrind-webrtc/val... tools/valgrind-webrtc/valgrind.gni:24: "../../tools/valgrind/gtest_exclude/ash_unittests.gtest-memcheck.txt", On 2016/11/24 18:58:19, kjellander_webrtc wrote: > but this change didn't really affect the problem, did it? I suggest restoring > this file. Done. https://codereview.webrtc.org/2531573003/diff/40001/tools/valgrind-webrtc/web... File tools/valgrind-webrtc/webrtc_tests.sh (right): https://codereview.webrtc.org/2531573003/diff/40001/tools/valgrind-webrtc/web... tools/valgrind-webrtc/webrtc_tests.sh:81: show_locate_valgrind_failed_warning On 2016/11/24 18:58:19, kjellander_webrtc wrote: > What's the point with showing the warning twice? Just thought it would be almost impossible to miss that way. I'm still not sure if showing it at the top (near the error message from locate_valgrind.sh) or at the bottom (where is most likely to be seen).
On 2016/11/24 19:52:32, ehmaldonado_webrtc wrote: > Yaaaaay! > It worked! > > https://codereview.webrtc.org/2531573003/diff/40001/tools/valgrind-webrtc/val... > File tools/valgrind-webrtc/valgrind.gni (right): > > https://codereview.webrtc.org/2531573003/diff/40001/tools/valgrind-webrtc/val... > tools/valgrind-webrtc/valgrind.gni:24: > "../../tools/valgrind/gtest_exclude/ash_unittests.gtest-memcheck.txt", > On 2016/11/24 18:58:19, kjellander_webrtc wrote: > > but this change didn't really affect the problem, did it? I suggest restoring > > this file. > > Done. > > https://codereview.webrtc.org/2531573003/diff/40001/tools/valgrind-webrtc/web... > File tools/valgrind-webrtc/webrtc_tests.sh (right): > > https://codereview.webrtc.org/2531573003/diff/40001/tools/valgrind-webrtc/web... > tools/valgrind-webrtc/webrtc_tests.sh:81: show_locate_valgrind_failed_warning > On 2016/11/24 18:58:19, kjellander_webrtc wrote: > > What's the point with showing the warning twice? > > Just thought it would be almost impossible to miss that way. > I'm still not sure if showing it at the top (near the error message from > locate_valgrind.sh) or at the bottom (where is most likely to be seen). I don't think it matters if the user needs to look for the error since it should rarely be shown. I vote for early display only but I'm fine with either. If you display it late you should clearly explain the two cases (swarming and incorrect .gclient)
On 2016/11/25 06:11:04, kjellander_webrtc wrote: > On 2016/11/24 19:52:32, ehmaldonado_webrtc wrote: > > Yaaaaay! > > It worked! > > > > > https://codereview.webrtc.org/2531573003/diff/40001/tools/valgrind-webrtc/val... > > File tools/valgrind-webrtc/valgrind.gni (right): > > > > > https://codereview.webrtc.org/2531573003/diff/40001/tools/valgrind-webrtc/val... > > tools/valgrind-webrtc/valgrind.gni:24: > > "../../tools/valgrind/gtest_exclude/ash_unittests.gtest-memcheck.txt", > > On 2016/11/24 18:58:19, kjellander_webrtc wrote: > > > but this change didn't really affect the problem, did it? I suggest > restoring > > > this file. > > > > Done. > > > > > https://codereview.webrtc.org/2531573003/diff/40001/tools/valgrind-webrtc/web... > > File tools/valgrind-webrtc/webrtc_tests.sh (right): > > > > > https://codereview.webrtc.org/2531573003/diff/40001/tools/valgrind-webrtc/web... > > tools/valgrind-webrtc/webrtc_tests.sh:81: show_locate_valgrind_failed_warning > > On 2016/11/24 18:58:19, kjellander_webrtc wrote: > > > What's the point with showing the warning twice? > > > > Just thought it would be almost impossible to miss that way. > > I'm still not sure if showing it at the top (near the error message from > > locate_valgrind.sh) or at the bottom (where is most likely to be seen). > > I don't think it matters if the user needs to look for the error since it should > rarely be shown. I vote for early display only but I'm fine with either. If you > display it late you should clearly explain the two cases (swarming and incorrect > .gclient) Sorry I missed you already did that.
On 2016/11/25 06:11:55, kjellander_webrtc wrote: > On 2016/11/25 06:11:04, kjellander_webrtc wrote: > > On 2016/11/24 19:52:32, ehmaldonado_webrtc wrote: > > > Yaaaaay! > > > It worked! > > > > > > > > > https://codereview.webrtc.org/2531573003/diff/40001/tools/valgrind-webrtc/val... > > > File tools/valgrind-webrtc/valgrind.gni (right): > > > > > > > > > https://codereview.webrtc.org/2531573003/diff/40001/tools/valgrind-webrtc/val... > > > tools/valgrind-webrtc/valgrind.gni:24: > > > "../../tools/valgrind/gtest_exclude/ash_unittests.gtest-memcheck.txt", > > > On 2016/11/24 18:58:19, kjellander_webrtc wrote: > > > > but this change didn't really affect the problem, did it? I suggest > > restoring > > > > this file. > > > > > > Done. > > > > > > > > > https://codereview.webrtc.org/2531573003/diff/40001/tools/valgrind-webrtc/web... > > > File tools/valgrind-webrtc/webrtc_tests.sh (right): > > > > > > > > > https://codereview.webrtc.org/2531573003/diff/40001/tools/valgrind-webrtc/web... > > > tools/valgrind-webrtc/webrtc_tests.sh:81: > show_locate_valgrind_failed_warning > > > On 2016/11/24 18:58:19, kjellander_webrtc wrote: > > > > What's the point with showing the warning twice? > > > > > > Just thought it would be almost impossible to miss that way. > > > I'm still not sure if showing it at the top (near the error message from > > > locate_valgrind.sh) or at the bottom (where is most likely to be seen). > > > > I don't think it matters if the user needs to look for the error since it > should > > rarely be shown. I vote for early display only but I'm fine with either. If > you > > display it late you should clearly explain the two cases (swarming and > incorrect > > .gclient) > > Sorry I missed you already did that. PTAL
On 2016/11/25 07:40:05, ehmaldonado_webrtc wrote: > On 2016/11/25 06:11:55, kjellander_webrtc wrote: > > On 2016/11/25 06:11:04, kjellander_webrtc wrote: > > > On 2016/11/24 19:52:32, ehmaldonado_webrtc wrote: > > > > Yaaaaay! > > > > It worked! > > > > > > > > > > > > > > https://codereview.webrtc.org/2531573003/diff/40001/tools/valgrind-webrtc/val... > > > > File tools/valgrind-webrtc/valgrind.gni (right): > > > > > > > > > > > > > > https://codereview.webrtc.org/2531573003/diff/40001/tools/valgrind-webrtc/val... > > > > tools/valgrind-webrtc/valgrind.gni:24: > > > > "../../tools/valgrind/gtest_exclude/ash_unittests.gtest-memcheck.txt", > > > > On 2016/11/24 18:58:19, kjellander_webrtc wrote: > > > > > but this change didn't really affect the problem, did it? I suggest > > > restoring > > > > > this file. > > > > > > > > Done. > > > > > > > > > > > > > > https://codereview.webrtc.org/2531573003/diff/40001/tools/valgrind-webrtc/web... > > > > File tools/valgrind-webrtc/webrtc_tests.sh (right): > > > > > > > > > > > > > > https://codereview.webrtc.org/2531573003/diff/40001/tools/valgrind-webrtc/web... > > > > tools/valgrind-webrtc/webrtc_tests.sh:81: > > show_locate_valgrind_failed_warning > > > > On 2016/11/24 18:58:19, kjellander_webrtc wrote: > > > > > What's the point with showing the warning twice? > > > > > > > > Just thought it would be almost impossible to miss that way. > > > > I'm still not sure if showing it at the top (near the error message from > > > > locate_valgrind.sh) or at the bottom (where is most likely to be seen). > > > > > > I don't think it matters if the user needs to look for the error since it > > should > > > rarely be shown. I vote for early display only but I'm fine with either. If > > you > > > display it late you should clearly explain the two cases (swarming and > > incorrect > > > .gclient) > > > > Sorry I missed you already did that. > > PTAL LGTM but you might want to add another suppression + link to the webrtc bug you filed
Patchset #6 (id:100001) has been deleted
Patchset #6 (id:120001) has been deleted
The CQ bit was checked by ehmaldonado@webrtc.org
The patchset sent to the CQ was uploaded after l-g-t-m from kjellander@webrtc.org Link to the patchset: https://codereview.webrtc.org/2531573003/#ps140001 (title: "stuff")
The CQ bit was unchecked by ehmaldonado@webrtc.org
Patchset #6 (id:140001) has been deleted
Description was changed from ========== Make Valgrind memcheck work in swarming. Add a fallback path to tools/valgrind-webrtc/webrtc_tests.sh if locate_valgrind.sh fails. This is a workaround to run memcheck on swarming, since locate_valgrind.sh fails even though the files are present. This is almost certainly because the way we use symlinks. A warning message is displayed to warn the developers to follow the instructions to get the valgrind binaries. R=kjellander@chromium.org BUG=chromium:497757 ========== to ========== Make Valgrind memcheck work in swarming. Add a fallback path to tools/valgrind-webrtc/webrtc_tests.sh if locate_valgrind.sh fails. This is a workaround to run memcheck on swarming, since locate_valgrind.sh fails even though the files are present. This is almost certainly because the way we use symlinks. A warning message is displayed to warn the developers to follow the instructions to get the valgrind binaries. Some extra suppressions were needed. The bug tracking them is https://bugs.webrtc.org/6773 R=kjellander@chromium.org BUG=chromium:497757 ==========
The CQ bit was checked by ehmaldonado@webrtc.org
The patchset sent to the CQ was uploaded after l-g-t-m from kjellander@webrtc.org Link to the patchset: https://codereview.webrtc.org/2531573003/#ps160001 (title: "Add more suppressions.")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.webrtc.org/...
CQ is committing da patch.
Bot data: {"patchset_id": 160001, "attempt_start_ts": 1480080871667320,
"parent_rev": "8ff088d69bdec8dc8592e5efaf3ca94540d51b69", "commit_rev":
"aee16f0f7cd2afd9a1c29981830d1a0e30c09b4f"}
Message was sent while issue was closed.
Description was changed from ========== Make Valgrind memcheck work in swarming. Add a fallback path to tools/valgrind-webrtc/webrtc_tests.sh if locate_valgrind.sh fails. This is a workaround to run memcheck on swarming, since locate_valgrind.sh fails even though the files are present. This is almost certainly because the way we use symlinks. A warning message is displayed to warn the developers to follow the instructions to get the valgrind binaries. Some extra suppressions were needed. The bug tracking them is https://bugs.webrtc.org/6773 R=kjellander@chromium.org BUG=chromium:497757 ========== to ========== Make Valgrind memcheck work in swarming. Add a fallback path to tools/valgrind-webrtc/webrtc_tests.sh if locate_valgrind.sh fails. This is a workaround to run memcheck on swarming, since locate_valgrind.sh fails even though the files are present. This is almost certainly because the way we use symlinks. A warning message is displayed to warn the developers to follow the instructions to get the valgrind binaries. Some extra suppressions were needed. The bug tracking them is https://bugs.webrtc.org/6773 R=kjellander@chromium.org BUG=chromium:497757 ==========
Message was sent while issue was closed.
Committed patchset #6 (id:160001)
Message was sent while issue was closed.
Description was changed from ========== Make Valgrind memcheck work in swarming. Add a fallback path to tools/valgrind-webrtc/webrtc_tests.sh if locate_valgrind.sh fails. This is a workaround to run memcheck on swarming, since locate_valgrind.sh fails even though the files are present. This is almost certainly because the way we use symlinks. A warning message is displayed to warn the developers to follow the instructions to get the valgrind binaries. Some extra suppressions were needed. The bug tracking them is https://bugs.webrtc.org/6773 R=kjellander@chromium.org BUG=chromium:497757 ========== to ========== Make Valgrind memcheck work in swarming. Add a fallback path to tools/valgrind-webrtc/webrtc_tests.sh if locate_valgrind.sh fails. This is a workaround to run memcheck on swarming, since locate_valgrind.sh fails even though the files are present. This is almost certainly because the way we use symlinks. A warning message is displayed to warn the developers to follow the instructions to get the valgrind binaries. Some extra suppressions were needed. The bug tracking them is https://bugs.webrtc.org/6773 R=kjellander@chromium.org BUG=chromium:497757 Committed: https://crrev.com/0fa164a22c4538b50a2c4cf62aee261fee54e404 Cr-Commit-Position: refs/heads/master@{#15244} ==========
Message was sent while issue was closed.
Patchset 6 (id:??) landed as https://crrev.com/0fa164a22c4538b50a2c4cf62aee261fee54e404 Cr-Commit-Position: refs/heads/master@{#15244} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
