Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1807013004:
Make DrMemory update process reliable (Closed)
|
Created:
4 years, 9 months ago by brucedawson Modified:
4 years, 9 months ago CC:
chromium-reviews, glider+watch_chromium.org, bruening+watch_chromium.org, zhaoqin1 Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionMake DrMemory update process reliable
The DrMemory update process only runs when it thinks that it needs to
but its heuristic is not accurate. A better thing to do is to always
update.
This change also adds setlocal so that environment variables don't
leak outside. This may also help with making this script more robust
in the future, once it has been in place for long enough for all
machines affected by the environment variable leakage to have rebooted.
BUG=595867
R=scottmg@chromium.org
Committed: https://chromium.googlesource.com/chromium/src/+/38e785b06f3942adad154fddd1b83d8a0f299a5e
Patch Set 1 #
Total comments: 2
Patch Set 2 : Moved setlocal, removed comment #
Total comments: 2
Patch Set 3 : Remove @ #Patch Set 4 : Pull to latest #
Total comments: 1
Messages
Total messages: 36 (12 generated)
brucedawson@chromium.org changed reviewers: + glider@chromium.org
Can you take a look at this? I don't know what the intent is but I know that the batch file as it is doesn't work reliably. That is, it doesn't unpack new toolchains when they are downloaded.
oshima@chromium.org changed reviewers: + bruening@chromium.org, oshima@chromium.org, thestig@chromium.org
+other owners. can one of you approve this? cc'nig zhaoqin@
scottmg@chromium.org changed reviewers: + scottmg@chromium.org
lgtm https://codereview.chromium.org/1807013004/diff/1/tools/valgrind/chrome_tests... File tools/valgrind/chrome_tests.bat (right): https://codereview.chromium.org/1807013004/diff/1/tools/valgrind/chrome_tests... tools/valgrind/chrome_tests.bat:1: @setlocal Normally @echo off setlocal
https://codereview.chromium.org/1807013004/diff/1/tools/valgrind/chrome_tests... File tools/valgrind/chrome_tests.bat (right): https://codereview.chromium.org/1807013004/diff/1/tools/valgrind/chrome_tests... tools/valgrind/chrome_tests.bat:1: @setlocal On 2016/03/17 23:25:34, scottmg wrote: > Normally > > @echo off > setlocal Won't that have the effect of turning off echo for the parent batch file if any? git.bat does that and that's a bug. I sometimes have to turn echo back on after calling git.bat.
scottmg@ - is there any risk from putting the @setlocal in there? Its absence seems like a bug (the batch file polluted my command prompt with lots of env variables) but do you know if anybody is depending on its absence?
I just verified that the order of @setlocal and @echo off makes no difference, so I'll change to the standard order.
Yeah, as far as I know setlocal is variables-only for better or worse. https://codereview.chromium.org/1807013004/diff/20001/tools/valgrind/chrome_t... File tools/valgrind/chrome_tests.bat (right): https://codereview.chromium.org/1807013004/diff/20001/tools/valgrind/chrome_t... tools/valgrind/chrome_tests.bat:6: @setlocal No @ needed now.
https://codereview.chromium.org/1807013004/diff/20001/tools/valgrind/chrome_t... File tools/valgrind/chrome_tests.bat (right): https://codereview.chromium.org/1807013004/diff/20001/tools/valgrind/chrome_t... tools/valgrind/chrome_tests.bat:6: @setlocal On 2016/03/18 00:12:46, scottmg wrote: > No @ needed now. Done.
The CQ bit was checked by brucedawson@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from scottmg@chromium.org Link to the patchset: https://codereview.chromium.org/1807013004/#ps40001 (title: "Remove @")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1807013004/40001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1807013004/40001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: ios_dbg_simulator_gn on tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_dbg_simulator...) ios_dbg_simulator_ninja on tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_dbg_simulator...) ios_rel_device_gn on tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_rel_device_gn...) ios_rel_device_ninja on tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_rel_device_ni...) mac_chromium_compile_dbg_ng on tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...) mac_chromium_gn_rel on tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_gn_r...) mac_chromium_rel_ng on tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
The CQ bit was checked by brucedawson@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from scottmg@chromium.org Link to the patchset: https://codereview.chromium.org/1807013004/#ps60001 (title: "Pull to latest")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1807013004/60001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1807013004/60001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: ios_dbg_simulator_gn on tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_dbg_simulator...) ios_dbg_simulator_ninja on tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_dbg_simulator...) ios_rel_device_gn on tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_rel_device_gn...) ios_rel_device_ninja on tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_rel_device_ni...) mac_chromium_gn_rel on tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_gn_r...) mac_chromium_rel_ng on tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
I don't understand why the patch is failing on all machines. The file has not been touched by anyone else in about four years, so it's not a matter of other changes coming in recently. Any ideas? Applying the patch from https://codereview.chromium.org/1807013004/#ps60001 DEBUG subprocess2( 215): git apply --index -3 -p1 --verbose; cwd=/b/build/slave/mac/build/src Failed to apply patch for tools/valgrind/chrome_tests.bat: While running git apply --index -3 -p1 --verbose; Checking patch tools/valgrind/chrome_tests.bat... error: while searching for: :: Use of this source code is governed by a BSD-style license that can be :: found in the LICENSE file. :: TODO(timurrrr): batch files 'export' all the variables to the parent shell set THISDIR=%~dp0 set TOOL_NAME="unknown" error: patch failed: tools/valgrind/chrome_tests.bat:3 Falling back to three-way merge... error: while searching for: :: Use of this source code is governed by a BSD-style license that can be :: found in the LICENSE file. :: TODO(timurrrr): batch files 'export' all the variables to the parent shell set THISDIR=%~dp0 set TOOL_NAME="unknown" error: patch failed: tools/valgrind/chrome_tests.bat:3 error: tools/valgrind/chrome_tests.bat: patch does not apply
On 2016/03/18 01:02:22, brucedawson wrote: > I don't understand why the patch is failing on all machines. The file has not > been touched by anyone else in about four years, so it's not a matter of other > changes coming in recently. Any ideas? > > Applying the patch from https://codereview.chromium.org/1807013004/#ps60001 > DEBUG subprocess2( 215): git apply --index -3 -p1 --verbose; > cwd=/b/build/slave/mac/build/src > Failed to apply patch for tools/valgrind/chrome_tests.bat: > While running git apply --index -3 -p1 --verbose; > Checking patch tools/valgrind/chrome_tests.bat... > error: while searching for: > :: Use of this source code is governed by a BSD-style license that can be > :: found in the LICENSE file. > > :: TODO(timurrrr): batch files 'export' all the variables to the parent shell > set THISDIR=%~dp0 > set TOOL_NAME="unknown" > > > error: patch failed: tools/valgrind/chrome_tests.bat:3 > Falling back to three-way merge... > error: while searching for: > :: Use of this source code is governed by a BSD-style license that can be > :: found in the LICENSE file. > > :: TODO(timurrrr): batch files 'export' all the variables to the parent shell > set THISDIR=%~dp0 > set TOOL_NAME="unknown" > > > error: patch failed: tools/valgrind/chrome_tests.bat:3 > error: tools/valgrind/chrome_tests.bat: patch does not apply It's because it's CRLF, the patchy do-dah doesn't work. You'll just have to `git cl land`.
On 2016/03/18 01:12:07, scottmg wrote: > On 2016/03/18 01:02:22, brucedawson wrote: > > I don't understand why the patch is failing on all machines. The file has not > > been touched by anyone else in about four years, so it's not a matter of other > > changes coming in recently. Any ideas? > > > > Applying the patch from https://codereview.chromium.org/1807013004/#ps60001 > > DEBUG subprocess2( 215): git apply --index -3 -p1 --verbose; > > cwd=/b/build/slave/mac/build/src > > Failed to apply patch for tools/valgrind/chrome_tests.bat: > > While running git apply --index -3 -p1 --verbose; > > Checking patch tools/valgrind/chrome_tests.bat... > > error: while searching for: > > :: Use of this source code is governed by a BSD-style license that can be > > :: found in the LICENSE file. > > > > :: TODO(timurrrr): batch files 'export' all the variables to the parent > shell > > set THISDIR=%~dp0 > > set TOOL_NAME="unknown" > > > > > > error: patch failed: tools/valgrind/chrome_tests.bat:3 > > Falling back to three-way merge... > > error: while searching for: > > :: Use of this source code is governed by a BSD-style license that can be > > :: found in the LICENSE file. > > > > :: TODO(timurrrr): batch files 'export' all the variables to the parent > shell > > set THISDIR=%~dp0 > > set TOOL_NAME="unknown" > > > > > > error: patch failed: tools/valgrind/chrome_tests.bat:3 > > error: tools/valgrind/chrome_tests.bat: patch does not apply > > It's because it's CRLF, the patchy do-dah doesn't work. You'll just have to `git > cl land`. Yep, we figured. I asked robliao@ to land it manually.
Message was sent while issue was closed.
Description was changed from ========== Make DrMemory update process reliable The DrMemory update process only runs when it thinks that it needs to but its heuristic is not accurate. A better thing to do is to always update. This change also adds setlocal so that environment variables don't leak outside. This may also help with making this script more robust in the future, once it has been in place for long enough for all machines affected by the environment variable leakage to have rebooted. BUG=595867 ========== to ========== Make DrMemory update process reliable The DrMemory update process only runs when it thinks that it needs to but its heuristic is not accurate. A better thing to do is to always update. This change also adds setlocal so that environment variables don't leak outside. This may also help with making this script more robust in the future, once it has been in place for long enough for all machines affected by the environment variable leakage to have rebooted. BUG=595867 R=scottmg@chromium.org Committed: https://crrev.com/38e785b06f3942adad154fddd1b83d8a0f299a5e Cr-Commit-Position: refs/heads/master@{#381870} ==========
Message was sent while issue was closed.
Patchset 4 (id:??) landed as https://crrev.com/38e785b06f3942adad154fddd1b83d8a0f299a5e Cr-Commit-Position: refs/heads/master@{#381870}
Message was sent while issue was closed.
Description was changed from ========== Make DrMemory update process reliable The DrMemory update process only runs when it thinks that it needs to but its heuristic is not accurate. A better thing to do is to always update. This change also adds setlocal so that environment variables don't leak outside. This may also help with making this script more robust in the future, once it has been in place for long enough for all machines affected by the environment variable leakage to have rebooted. BUG=595867 R=scottmg@chromium.org Committed: https://crrev.com/38e785b06f3942adad154fddd1b83d8a0f299a5e Cr-Commit-Position: refs/heads/master@{#381870} ========== to ========== Make DrMemory update process reliable The DrMemory update process only runs when it thinks that it needs to but its heuristic is not accurate. A better thing to do is to always update. This change also adds setlocal so that environment variables don't leak outside. This may also help with making this script more robust in the future, once it has been in place for long enough for all machines affected by the environment variable leakage to have rebooted. BUG=595867 R=scottmg@chromium.org Committed: https://chromium.googlesource.com/chromium/src/+/38e785b06f3942adad154fddd1b8... ==========
Message was sent while issue was closed.
Committed patchset #4 (id:60001) manually as 38e785b06f3942adad154fddd1b83d8a0f299a5e (presubmit successful).
Message was sent while issue was closed.
On 2016/03/18 01:54:31, robliao wrote: > Committed patchset #4 (id:60001) manually as > 38e785b06f3942adad154fddd1b83d8a0f299a5e (presubmit successful). PROTIP: Changing the git author of the patch isn't sufficient to actually change the author that gets landed. You still need git cl try -c "...". Sorry Bruce!
Message was sent while issue was closed.
timurrrr@google.com changed reviewers: + timurrrr@google.com
Message was sent while issue was closed.
https://codereview.chromium.org/1807013004/diff/60001/tools/valgrind/chrome_t... File tools/valgrind/chrome_tests.bat (left): https://codereview.chromium.org/1807013004/diff/60001/tools/valgrind/chrome_t... tools/valgrind/chrome_tests.bat:35: if NOT "%DRMEMORY_COMMAND%"=="" GOTO :RUN_TESTS No, you shouldn't remove this line. This line is very useful if one wants to test a custom build of Dr.Memory, e.g. during DrM development.
Message was sent while issue was closed.
I have no idea how this .bat file should be working, deferring to other owners.
Message was sent while issue was closed.
On 2016/03/18 15:23:50, the_wrong_timurrrr_2 wrote: > https://codereview.chromium.org/1807013004/diff/60001/tools/valgrind/chrome_t... > File tools/valgrind/chrome_tests.bat (left): > > https://codereview.chromium.org/1807013004/diff/60001/tools/valgrind/chrome_t... > tools/valgrind/chrome_tests.bat:35: if NOT "%DRMEMORY_COMMAND%"=="" GOTO > :RUN_TESTS > No, you shouldn't remove this line. > This line is very useful if one wants to test a custom build of Dr.Memory, e.g. > during DrM development. Sorry, should we revert then?
Message was sent while issue was closed.
> Sorry, should we revert then? We can put it back eventually. The main problem is that it is incompatible with the lack of 'setlocal'. Now we have setlocal and eventually the environment variable leakage will go away. If the goal of the branch is to give a way to prevent unpacking of DrMemory then we can just check a separate environment variable that is specialized for that purpose. i.e; if "%DRMEMORY_SKIPUNPACK%"=="" GOTO :RUN_TESTS
Message was sent while issue was closed.
No, the goal of this branch is to allow using a custom Dr.Memory binary that may be located wherever you like on your filesystem. If DRMEMORY_COMMAND is not specified, DrM should unpack on each invocation.
Message was sent while issue was closed.
Discussion should be on crbug.com/595867 |
