Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
8505028:
Update chrome_tests.sh to run DrMemory from Cygwin
|
Created:
9 years, 1 month ago by Reid Kleckner (google) Modified:
9 years, 1 month ago CC:
chromium-reviews, pam+watch_chromium.org, stuartmorgan+watch_chromium.org Base URL:
http://src.chromium.org/svn/trunk/src/ Visibility:
Public. |
DescriptionUpdate chrome_tests.sh to run DrMemory from Cygwin
Also change valgrind_tests.py to use cygpath to give DrMemory Windows paths
when running with Cygwin Python.
Patch Set 1 #
Total comments: 9
Patch Set 2 : '' #Patch Set 3 : '' #
Messages
Total messages: 14 (0 generated)
First Chromium CL, so you'll have to commit it if/when it looks good. This makes it possible to run chrome_tests.sh --tool drmemory from Cygwin. Both scripts find /usr/bin/python instead of depot_tools/python.bat, so they need the _Cygpath bits in valgrind_test.py, or drmemory.exe complains about files that don't exist.
I like the bash-fu. Alexander, can you please look at the .sh part of the change? I remember there were a few special cases you were fighting with, please double check. http://codereview.chromium.org/8505028/diff/1/tools/valgrind/valgrind_test.py File tools/valgrind/valgrind_test.py (right): http://codereview.chromium.org/8505028/diff/1/tools/valgrind/valgrind_test.py... tools/valgrind/valgrind_test.py:710: def _Cygpath(path): maybe this should me moved to tools\valgrind\common.py
http://codereview.chromium.org/8505028/diff/1/tools/valgrind/chrome_tests.sh File tools/valgrind/chrome_tests.sh (right): http://codereview.chromium.org/8505028/diff/1/tools/valgrind/chrome_tests.sh#... tools/valgrind/chrome_tests.sh:17: tool="" How about initially setting $tool to "memcheck" ? http://codereview.chromium.org/8505028/diff/1/tools/valgrind/chrome_tests.sh#... tools/valgrind/chrome_tests.sh:35: "memcheck" | "") # No --tool arg means memcheck. No "--tool" flag means memcheck, but a missing value of --tool does not. I.e. tools/valgrind/chrome_tests.sh -t base # correct tools/valgrind/chrome_tests.sh --tool=memcheck -t base # correct tools/valgrind/chrome_tests.sh --tool -t base # incorrect http://codereview.chromium.org/8505028/diff/1/tools/valgrind/valgrind_test.py File tools/valgrind/valgrind_test.py (right): http://codereview.chromium.org/8505028/diff/1/tools/valgrind/valgrind_test.py... tools/valgrind/valgrind_test.py:710: def _Cygpath(path): Am I right you're this function is called on every platform, even non-Cygwin? Then it should be named accordingly (I can't think of anything better than _NormalizePath)
http://codereview.chromium.org/8505028/diff/1/tools/valgrind/chrome_tests.sh File tools/valgrind/chrome_tests.sh (right): http://codereview.chromium.org/8505028/diff/1/tools/valgrind/chrome_tests.sh#... tools/valgrind/chrome_tests.sh:17: tool="" On 2011/11/10 12:12:13, Alexander Potapenko wrote: > How about initially setting $tool to "memcheck" ? Done. http://codereview.chromium.org/8505028/diff/1/tools/valgrind/chrome_tests.sh#... tools/valgrind/chrome_tests.sh:35: "memcheck" | "") # No --tool arg means memcheck. On 2011/11/10 12:12:13, Alexander Potapenko wrote: > No "--tool" flag means memcheck, but a missing value of --tool does not. I.e. > tools/valgrind/chrome_tests.sh -t base # correct > tools/valgrind/chrome_tests.sh --tool=memcheck -t base # correct > tools/valgrind/chrome_tests.sh --tool -t base # incorrect The loop above would set $tool to -t in your example, but I think it would pick memcheck for "chrome_tests.sh -t base --tool". http://codereview.chromium.org/8505028/diff/1/tools/valgrind/valgrind_test.py File tools/valgrind/valgrind_test.py (right): http://codereview.chromium.org/8505028/diff/1/tools/valgrind/valgrind_test.py... tools/valgrind/valgrind_test.py:710: def _Cygpath(path): On 2011/11/10 12:12:13, Alexander Potapenko wrote: > Am I right you're this function is called on every platform, even non-Cygwin? > Then it should be named accordingly (I can't think of anything better than > _NormalizePath) It's actually fairly specific to DrMemory, I think, but I suppose you could generalize it to cover any native Windows tool that you want to invoke from Cygwin. None of the other tools in this file support Windows, though, right? I'll actually make it a private static method on the DrMemory class. If and when there are other Windows tools we can move it back out.
http://codereview.chromium.org/8505028/diff/1/tools/valgrind/valgrind_test.py File tools/valgrind/valgrind_test.py (right): http://codereview.chromium.org/8505028/diff/1/tools/valgrind/valgrind_test.py... tools/valgrind/valgrind_test.py:710: def _Cygpath(path): On 2011/11/10 14:00:30, rnk_google wrote: > It's actually fairly specific to DrMemory, I think, but I suppose you could > generalize it to cover any native Windows tool that you want to invoke from > Cygwin. The only reason for doing your CL is to support running devenv-built binaries under DrMemory from Cygwin, right? Did you try tools\valgrind\chrome_tests.bat in Cygwin? > None of the other tools in this file support Windows, though, right? Wrong: TSan is working fine on Windows. No-one ever told me it's not working from Cygwin, not sure if it's very needed for DrM either. > I'll actually make it a private static method on the DrMemory class. If and > when there are other Windows tools we can move it back out. See above
http://codereview.chromium.org/8505028/diff/1/tools/valgrind/valgrind_test.py File tools/valgrind/valgrind_test.py (right): http://codereview.chromium.org/8505028/diff/1/tools/valgrind/valgrind_test.py... tools/valgrind/valgrind_test.py:710: def _Cygpath(path): On 2011/11/10 14:03:19, Timur Iskhodzhanov wrote: > On 2011/11/10 14:00:30, rnk_google wrote: > > It's actually fairly specific to DrMemory, I think, but I suppose you could > > generalize it to cover any native Windows tool that you want to invoke from > > Cygwin. > The only reason for doing your CL is to support running devenv-built binaries > under DrMemory from Cygwin, right? No. Because I'm using Cygwin, depot_tools uses those binaries instead of installing it's own. Therefore, both chrome_tests.sh and chrome_tests.bat invoke /usr/bin/python, which uses POSIX style paths. drmemory.exe expects Windows style paths, not the app. > Did you try tools\valgrind\chrome_tests.bat in Cygwin? Yes, it fails to import logging_utils because Cygwin Python doesn't like the backslashes in PYTHONPATH. I'm better at bash than cmd, so I added drmemory support to chrome_tests.sh instead of having chrome_tests.bat conditionally invoke cygpath. (chrome_tests.bat works fine from a cmd prompt, btw) > > None of the other tools in this file support Windows, though, right? > Wrong: TSan is working fine on Windows. > No-one ever told me it's not working from Cygwin, not sure if it's very needed > for DrM either. > > > I'll actually make it a private static method on the DrMemory class. If and > > when there are other Windows tools we can move it back out. > See above OK, I'll see if I can't get tsan running on Cygwin.
Did I get it right: you can't run .sh/.bat from Cygwin with the trunk version but .bat works OK? That said, I totally like the memcheck .sh part of the patch (replacing if's with a cute switch) and I don't really think the cygwin support is very needed. OTOH, I won't object for the cygwin support but please put the NormalizeWindowsPath function into common.py On 2011/11/10 14:19:09, Reid Kleckner wrote: > http://codereview.chromium.org/8505028/diff/1/tools/valgrind/valgrind_test.py > File tools/valgrind/valgrind_test.py (right): > > http://codereview.chromium.org/8505028/diff/1/tools/valgrind/valgrind_test.py... > tools/valgrind/valgrind_test.py:710: def _Cygpath(path): > On 2011/11/10 14:03:19, Timur Iskhodzhanov wrote: > > On 2011/11/10 14:00:30, rnk_google wrote: > > > It's actually fairly specific to DrMemory, I think, but I suppose you could > > > generalize it to cover any native Windows tool that you want to invoke from > > > Cygwin. > > The only reason for doing your CL is to support running devenv-built binaries > > under DrMemory from Cygwin, right? > > No. Because I'm using Cygwin, depot_tools uses those binaries instead of > installing it's own. Therefore, both chrome_tests.sh and chrome_tests.bat > invoke /usr/bin/python, which uses POSIX style paths. drmemory.exe expects > Windows style paths, not the app. > > > Did you try tools\valgrind\chrome_tests.bat in Cygwin? > > Yes, it fails to import logging_utils because Cygwin Python doesn't like the > backslashes in PYTHONPATH. I'm better at bash than cmd, so I added drmemory > support to chrome_tests.sh instead of having chrome_tests.bat conditionally > invoke cygpath. > > (chrome_tests.bat works fine from a cmd prompt, btw) > > > > None of the other tools in this file support Windows, though, right? > > Wrong: TSan is working fine on Windows. > > No-one ever told me it's not working from Cygwin, not sure if it's very needed > > for DrM either. > > > > > I'll actually make it a private static method on the DrMemory class. If and > > > when there are other Windows tools we can move it back out. > > See above > > OK, I'll see if I can't get tsan running on Cygwin.
On Thu, Nov 10, 2011 at 9:30 AM, <timurrrr@chromium.org> wrote: > Did I get it right: you can't run .sh/.bat from Cygwin with the trunk > version > but .bat works OK? > No, the bat works OK *from a cmd prompt*. That said, I totally like the memcheck .sh part of the patch (replacing if's > with a cute switch) > and I don't really think the cygwin support is very needed. > OTOH, I won't object for the cygwin support but please put the > NormalizeWindowsPath function into common.py OK, done. I agree it's not critical, but it's annoying to have to switch to a terminal that doesn't support tab completion to run the tests the way the bot does. Reid
LGTM from me now. Alexander, mind taking a second look? Reid, If everything goes OK I'll commit this tomorrow
Ping
Alexander?
On 2011/11/18 08:16:51, Timur Iskhodzhanov wrote: > Alexander? LGTM, sorry for missing the email.
On 2011/11/18 12:53:31, Alexander Potapenko wrote: > On 2011/11/18 08:16:51, Timur Iskhodzhanov wrote: > > Alexander? > > LGTM, sorry for missing the email. Looks like noone set the "commit" flag yet, so I've commited this as http://codereview.chromium.org/8636008/ + http://src.chromium.org/viewvc/chrome?view=rev&revision=111129 with one change: added "python" in the middle of the last chrome_tests.sh line (wasn't starting under Cygwin for me otherwise)
On Tue, Nov 22, 2011 at 5:37 AM, <timurrrr@chromium.org> wrote: > On 2011/11/18 12:53:31, Alexander Potapenko wrote: >> >> On 2011/11/18 08:16:51, Timur Iskhodzhanov wrote: >> > Alexander? > >> LGTM, sorry for missing the email. > > Looks like noone set the "commit" flag yet, > so I've commited this as > http://codereview.chromium.org/8636008/ > + http://src.chromium.org/viewvc/chrome?view=rev&revision=111129 Thanks, I didn't realize I could just let the CQ commit it for me without commit privs. Reid |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
