Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
14093004:
Ensure that we pick up 'git.bat' and 'svn.bat' on Windows. (Closed)
|
Created:
7 years, 8 months ago by Dirk Pranke Modified:
7 years, 8 months ago CC:
chromium-reviews, cmp-cc_chromium.org, M-A Ruel, iannucci+depot_tools_chromium.org, iannucci Visibility:
Public. |
DescriptionEnsure that we pick up 'git.bat' and 'svn.bat' on Windows.
R=maruel@chromium.org
BUG=227526
Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=193865
Patch Set 1 #Patch Set 2 : be much more careful about finding scripts and binaries #
Total comments: 1
Patch Set 3 : fix path for git on win using _binary_subdirs #Patch Set 4 : fix typo on win, reformat, and clarify an unrelated TODO #
Total comments: 11
Patch Set 5 : more simplification per szager #Patch Set 6 : fix typos and handle msys properly by using the git in the path #Patch Set 7 : clean up whitespace diffs #
Total comments: 6
Patch Set 8 : simplify even further #Messages
Total messages: 19 (0 generated)
Having shell=True be the default behavior is dangerous and undesirable. Would be much better to use sys.platform or platform.system() to determine that we're running on windows, and then find the full path to the .bat file and execute that instead.
On 2013/04/10 21:42:44, Do Not Use -- Aaron Gable wrote: > Having shell=True be the default behavior is dangerous and undesirable. Would be > much better to use sys.platform or platform.system() to determine that we're > running on windows, and then find the full path to the .bat file and execute > that instead. I agree that it's dangerous in general as a wrapper around subprocess. I think it's much less dangerous (if not entirely safe) in this case. That said, I do agree that the proper fix is to find the real binaries. Are you suggesting that we shouldn't land this change and do the proper thing?
Okay, please take another look ... this should be significantly more robust.
https://codereview.chromium.org/14093004/diff/4001/fetch.py File fetch.py (right): https://codereview.chromium.org/14093004/diff/4001/fetch.py#newcode69 fetch.py:69: return spawn.find_executable(os.path.join(SCRIPT_PATH, binary_name + '_bin', Isn't git installed to depot_tools\git-1.8.0_bin\ (not depot_tools\git\) on windows machines?
On 2013/04/11 18:21:09, Aaron Gable wrote: > https://codereview.chromium.org/14093004/diff/4001/fetch.py > File fetch.py (right): > > https://codereview.chromium.org/14093004/diff/4001/fetch.py#newcode69 > fetch.py:69: return spawn.find_executable(os.path.join(SCRIPT_PATH, binary_name > + '_bin', > Isn't git installed to depot_tools\git-1.8.0_bin\ (not depot_tools\git\) on > windows machines? Bah, you're right. It used to be git_bin, but the executable turns out to actually have been in git_bin\bin\git.exe anyway.
I think this is actually working now. Please take another look?
On 2013/04/11 19:04:26, Dirk Pranke wrote: > I think this is actually working now. Please take another look? Lgtm! Let's get this in so Robbie can start landing his unittests.
https://codereview.chromium.org/14093004/diff/12001/fetch.py File fetch.py (right): https://codereview.chromium.org/14093004/diff/12001/fetch.py#newcode65 fetch.py:65: if sys.platform == 'win32': This is unnecessary; spawn.find_executable will look for the '.exe' suffix automatically. https://codereview.chromium.org/14093004/diff/12001/fetch.py#newcode72 fetch.py:72: return spawn.find_executable( What's the purpose of this call? As far as I can tell, it only checks for the existence of the file, and returns the path if it exists, without checking whether it's executable. Also, you need to handle the case where binary_name is not in self._binary_subdirs (otherwise you'll get a KeyError). Maybe: subdir = self._binary_subdirs.get(binary_name, '') full_path = os.path.join(SCRIPT_PATH, subdir, exe_name) if os.access(full_path, os.X_OK): return full_path return None Really, this whole method could be: path = os.environ['PATH'] if binary_name in self._binary_subdirs[binary_name]: subdir = os.path.join(SCRIPT_PATH, self._binary_subdirs[binary_name]) path = os.pathsep.join(path, subdir) return spawn.find_executable(binary_name) https://codereview.chromium.org/14093004/diff/12001/fetch.py#newcode80 fetch.py:80: path = os.path.join(SCRIPT_PATH, script + '.py') path = os.path.join(SCRIPT_PATH, script) if not script.endswith('.py): path += '.py' https://codereview.chromium.org/14093004/diff/12001/fetch.py#newcode114 fetch.py:114: self._binary_subdirs['git'] = os.path.join('git-1.8.0_bin', 'bin') if sys.platform == 'win32': self._binary_subdirs['git'] = ... https://codereview.chromium.org/14093004/diff/12001/fetch.py#newcode123 fetch.py:123: self._binary_subdirs['svn'] = 'svn_bin' if sys.platform == 'win32': ...
https://codereview.chromium.org/14093004/diff/12001/fetch.py File fetch.py (right): https://codereview.chromium.org/14093004/diff/12001/fetch.py#newcode65 fetch.py:65: if sys.platform == 'win32': On 2013/04/11 21:30:01, szager1 wrote: > This is unnecessary; spawn.find_executable will look for the '.exe' suffix > automatically. Huh, so it does. That's great, as it simplifies things and removes some disappointment. I had thought it did but it was failing when I was testing things last night, but I think it turns out those things either weren't actually in my path or weren't where I thought they were. https://codereview.chromium.org/14093004/diff/12001/fetch.py#newcode72 fetch.py:72: return spawn.find_executable( On 2013/04/11 21:30:01, szager1 wrote: > What's the purpose of this call? As far as I can tell, it only checks for the > existence of the file, and returns the path if it exists, without checking > whether it's executable. > I thought that spawn.find_executable() does actually check that something is executable? If not, I agree it's no different than os.path.exists() + returning the string. > Also, you need to handle the case where binary_name is not in > self._binary_subdirs (otherwise you'll get a KeyError). > No, I don't want to handle that case. I consider that a programming error, as this routine should only be called by routines like run_git and run_svn, for a specific set of binaries dealt with via mixins. It is not a general purpose routine. I can add in asserts() and/or comments to make this clearer. > > Really, this whole method could be: > > path = os.environ['PATH'] > if binary_name in self._binary_subdirs[binary_name]: > subdir = os.path.join(SCRIPT_PATH, self._binary_subdirs[binary_name]) > path = os.pathsep.join(path, subdir) > return spawn.find_executable(binary_name) I don't know quite what you were getting at with this pseudocode, but you're not actually doing anything with 'path' here? https://codereview.chromium.org/14093004/diff/12001/fetch.py#newcode80 fetch.py:80: path = os.path.join(SCRIPT_PATH, script + '.py') On 2013/04/11 21:30:01, szager1 wrote: > path = os.path.join(SCRIPT_PATH, script) > if not script.endswith('.py): > path += '.py' Again, this is too general for my intentions. https://codereview.chromium.org/14093004/diff/12001/fetch.py#newcode114 fetch.py:114: self._binary_subdirs['git'] = os.path.join('git-1.8.0_bin', 'bin') On 2013/04/11 21:30:01, szager1 wrote: > if sys.platform == 'win32': > self._binary_subdirs['git'] = ... I'm not sure if you would still want me to make these changes given what I've written above. I always want binary_subdirs to be populated.
Frankly, this code still doesn't feel quite right to me. You have this
_find_executable() method with a bunch of code to make sure you leave no stone
unturned when searching for an executable. That makes it look like very
generic, catch-all, just-do-the-right-thing code.
But then you say that you really only want to deal with very specific use cases,
and you prefer the code to error out if the code is used in an unexpected way.
That's really my overall stylistic comment: I find the code over-architected for
what it's actually doing.
If you really want the code to be porcelain-ish, then why not keep it simple:
class Checkout:
def run(*args, **kwargs):
print 'Running: %s' % args
subprocess.check_call(args, **kwargs)
def run_svn(self, *args, **kwargs):
if sys.platform == 'win32':
exe = os.path.join(SCRIPT_PATH, 'svn_bin', 'svn.exe')
else:
exe = 'svn'
self.run((exe,) + args, **kwargs)
def run_git(self, *args, **kwargs):
if sys.platform == 'win32':
exe = os.path.join(SCRIPT_PATH, 'git-1.8.0_bin', 'bin', 'git.exe')
else:
exe = 'git'
self.run((exe,) + args, **kwargs)
def run_gclient(self, *args, **kwargs):
exe = os.path.join(SCRIPT_PATH, 'gclient.py')
self.run((sys.executable, exe) + args, **kwargs)
Even if you disregard all of this, I'll still l-g-t-m if you fix the other
stuff.
https://codereview.chromium.org/14093004/diff/12001/fetch.py
File fetch.py (right):
https://codereview.chromium.org/14093004/diff/12001/fetch.py#newcode72
fetch.py:72: return spawn.find_executable(
On 2013/04/11 21:52:08, Dirk Pranke wrote:
> On 2013/04/11 21:30:01, szager1 wrote:
> > What's the purpose of this call? As far as I can tell, it only checks for
the
> > existence of the file, and returns the path if it exists, without checking
> > whether it's executable.
> >
>
> I thought that spawn.find_executable() does actually check that something is
> executable? If not, I agree it's no different than os.path.exists() +
returning
> the string.
>
> > Also, you need to handle the case where binary_name is not in
> > self._binary_subdirs (otherwise you'll get a KeyError).
> >
>
> No, I don't want to handle that case. I consider that a programming error, as
> this routine should only be called by routines like run_git and run_svn, for a
> specific set of binaries dealt with via mixins. It is not a general purpose
> routine. I can add in asserts() and/or comments to make this clearer.
>
> >
> > Really, this whole method could be:
> >
> > path = os.environ['PATH']
> > if binary_name in self._binary_subdirs[binary_name]:
> > subdir = os.path.join(SCRIPT_PATH, self._binary_subdirs[binary_name])
> > path = os.pathsep.join(path, subdir)
> > return spawn.find_executable(binary_name)
>
> I don't know quite what you were getting at with this pseudocode, but you're
not
> actually doing anything with 'path' here?
Whoops, should be:
spawn.find_executable(binary_name, path)
https://codereview.chromium.org/14093004/diff/12001/fetch.py#newcode114
fetch.py:114: self._binary_subdirs['git'] = os.path.join('git-1.8.0_bin', 'bin')
On 2013/04/11 21:52:08, Dirk Pranke wrote:
> On 2013/04/11 21:30:01, szager1 wrote:
> > if sys.platform == 'win32':
> > self._binary_subdirs['git'] = ...
>
> I'm not sure if you would still want me to make these changes given what I've
> written above. I always want binary_subdirs to be populated.
Done.
On 2013/04/11 22:18:43, szager1 wrote: > Frankly, this code still doesn't feel quite right to me. You have this > _find_executable() method with a bunch of code to make sure you leave no stone > unturned when searching for an executable. That makes it look like very > generic, catch-all, just-do-the-right-thing code. > > But then you say that you really only want to deal with very specific use cases, > and you prefer the code to error out if the code is used in an unexpected way. > > That's really my overall stylistic comment: I find the code over-architected for > what it's actually doing. > Thanks for the feedback. This whole thing originally grew out of the desire to (a) share common code for handling file not found and dryrun and (b) to deal with finding git and svn on win. You may well be right that I've gone off into the weeds here. Let me try another approach and see where we end up.
On 2013/04/11 23:13:35, Dirk Pranke wrote: > Let me try another approach and see where we end up. Right, I think this ends up much cleaner and simpler. WDYT?
https://codereview.chromium.org/14093004/diff/25001/fetch.py File fetch.py (right): https://codereview.chromium.org/14093004/diff/25001/fetch.py#newcode62 fetch.py:62: print 'Running: %s %s' % (tool, Just curious, why print a symbolic name for the command here, rather than cmd_prefix? Makes the terminal output less useful, because I can't cut and paste it to the shell prompt to reproduce. https://codereview.chromium.org/14093004/diff/25001/fetch.py#newcode80 fetch.py:80: if sys.platform == 'win32' and not spawn.find_executable('git'): Optional, but better organization: class GitCheckout(Checkout): if sys.platform == 'win32' and ...: exe = os.path.join(SCRIPT_PATH, ...) else exe = 'git' def run_git(self, *cmd, **kwargs): return self.run('git', (self.exe,), *cmd, **kwargs) https://codereview.chromium.org/14093004/diff/25001/fetch.py#newcode90 fetch.py:90: if sys.platform == 'win32' and not spawn.find_executable('svn'): Same comment.
https://codereview.chromium.org/14093004/diff/25001/fetch.py File fetch.py (right): https://codereview.chromium.org/14093004/diff/25001/fetch.py#newcode62 fetch.py:62: print 'Running: %s %s' % (tool, On 2013/04/12 03:47:07, szager1 wrote: > Just curious, why print a symbolic name for the command here, rather than > cmd_prefix? Makes the terminal output less useful, because I can't cut and > paste it to the shell prompt to reproduce. Heh. I actually want to be able to cut and paste, so I hate the 'Running: ' part. Partially, I didn't want to print out the path to python and the '.py', but I think I can fix that by doing a find_executable on gclient as well. https://codereview.chromium.org/14093004/diff/25001/fetch.py#newcode80 fetch.py:80: if sys.platform == 'win32' and not spawn.find_executable('git'): On 2013/04/12 03:47:07, szager1 wrote: > Optional, but better organization: > > class GitCheckout(Checkout): > if sys.platform == 'win32' and ...: > exe = os.path.join(SCRIPT_PATH, ...) > else > exe = 'git' > I'm not really a fan of class-level statement control flow, so I'll pass on this. https://codereview.chromium.org/14093004/diff/25001/fetch.py#newcode90 fetch.py:90: if sys.platform == 'win32' and not spawn.find_executable('svn'): On 2013/04/12 03:47:07, szager1 wrote: > Same comment. same reply :)
one more once?
lgtm, thanks for the improvements
On 2013/04/12 05:35:08, szager1 wrote: > lgtm, thanks for the improvements Thanks for the great reviewing! It ended up *much* better than it started.
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/dpranke@chromium.org/14093004/30001
Message was sent while issue was closed.
Change committed as 193865 |
