Fix SCM.ReadRootFile when self.gclient_root is not yet defined

Fix SCM.ReadRootFile when self.gclient_root is not yet defined

Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=47426

[davidjames, 2010-05-14 20:26:06]

Hi Marc-Antoine,

This patch fixes an issue where gcl try crashes when self.gclient_root is not
defined. Here's the stack trace I got below.

$ gcl try use_access --email davidjames@google.com --issue 2073005 --root
src/base --dry_run       
Traceback (most recent call last):
  File "/home/davidjames/depot_tools/gcl.py", line 1260, in <module>
    sys.exit(main())
  File "/home/davidjames/depot_tools/gcl.py", line 1246, in main
    TryChange(change_info, args, swallow_exception=False)
  File "/home/davidjames/depot_tools/gcl.py", line 878, in TryChange
    prog='gcl try')
  File "/home/davidjames/depot_tools/trychange.py", line 617, in TryChange
    checkouts[0].AutomagicalSettings()
  File "/home/davidjames/depot_tools/trychange.py", line 152, in
AutomagicalSettings
    self._GclStyleSettings()
  File "/home/davidjames/depot_tools/trychange.py", line 124, in
_GclStyleSettings
    'port': self.GetCodeReviewSetting('TRYSERVER_HTTP_PORT'),
  File "/home/davidjames/depot_tools/trychange.py", line 111, in
GetCodeReviewSetting
    settings_file = self.ReadRootFile(self.codereview_settings_file)
  File "/home/davidjames/depot_tools/trychange.py", line 185, in ReadRootFile
    data = SCM.ReadRootFile(self, filename)
  File "/home/davidjames/depot_tools/trychange.py", line 161, in ReadRootFile
    root = os.path.abspath(self.gclient_root)
  File "/usr/lib/python2.5/posixpath.py", line 402, in abspath
    if not isabs(path):
  File "/usr/lib/python2.5/posixpath.py", line 49, in isabs
    return s.startswith('/')
AttributeError: 'NoneType' object has no attribute 'startswith'
Sending crash report ...
A stack trace has been sent to the maintainers.

Cheers,

David

[M-A Ruel, 2010-05-14 21:46:52]

http://codereview.chromium.org/2102004/diff/1/2
File trychange.py (right):

http://codereview.chromium.org/2102004/diff/1/2#newcode162
trychange.py:162: if not self.gclient_root:
If gclient_root is not set, it's probably because --no_gclient was used. Then
it's better to use self.checkout_root instead, e.g. not search in upper
directories.

[M-A Ruel, 2010-05-14 21:50:25]

May I ask why you were using --root? You have a sparse checkout?

http://codereview.chromium.org/2102004/diff/1/2
File trychange.py (right):

http://codereview.chromium.org/2102004/diff/1/2#newcode162
trychange.py:162: if not self.gclient_root:
On 2010/05/14 21:46:52, Marc-Antoine Ruel wrote:
> If gclient_root is not set, it's probably because --no_gclient was used. Then
> it's better to use self.checkout_root instead, e.g. not search in upper
> directories.

Well, I was wrong since I forgot to read your message :)

So in any case, I think gclient_root shouldn't be updated there, just set root =
os.path.abspath(self.checkout_root)

[davidjames, 2010-05-15 00:38:58]

I'm using --root because the patch doesn't apply to the root of the repository
-- it's in a subdirectory of the repo. I'm just applying the patch from an
ordinary Chrome OS checkout (specifically
~/chromeos/src/third_party/chrome/files/base)

http://codereview.chromium.org/2102004/diff/1/2
File trychange.py (right):

http://codereview.chromium.org/2102004/diff/1/2#newcode162
trychange.py:162: if not self.gclient_root:
On 2010/05/14 21:50:26, Marc-Antoine Ruel wrote:
> On 2010/05/14 21:46:52, Marc-Antoine Ruel wrote:
> > If gclient_root is not set, it's probably because --no_gclient was used.
Then
> > it's better to use self.checkout_root instead, e.g. not search in upper
> > directories.
> 
> Well, I was wrong since I forgot to read your message :)
> 
> So in any case, I think gclient_root shouldn't be updated there, just set root
=
> os.path.abspath(self.checkout_root)

Done.

[davidjames, 2010-05-15 00:46:38]

http://codereview.chromium.org/2102004/diff/1/2
File trychange.py (right):

http://codereview.chromium.org/2102004/diff/1/2#newcode162
trychange.py:162: if not self.gclient_root:
On 2010/05/15 00:38:58, davidjames wrote:
> On 2010/05/14 21:50:26, Marc-Antoine Ruel wrote:
> > On 2010/05/14 21:46:52, Marc-Antoine Ruel wrote:
> > > If gclient_root is not set, it's probably because --no_gclient was used.
> Then
> > > it's better to use self.checkout_root instead, e.g. not search in upper
> > > directories.
> > 
> > Well, I was wrong since I forgot to read your message :)
> > 
> > So in any case, I think gclient_root shouldn't be updated there, just set
root
> =
> > os.path.abspath(self.checkout_root)
> 
> Done.

Oops. Actually, self.checkout_root doesn't work, because it doesn't contain my
gclient.settings. We need to do the search. In patch version #3, I updated it to
actually search for the gclient root, but not to update self.gclient_root

[M-A Ruel, 2010-05-15 00:53:13]

http://codereview.chromium.org/2102004/diff/10001/11001
File trychange.py (right):

http://codereview.chromium.org/2102004/diff/10001/11001#newcode165
trychange.py:165: root = gclient_utils.FindGclientRoot(cur)
As I wrote earlier, I still prefer to use root = cur since if you specify a
root, you usually don't want it to mess with interwinded checkouts.

[M-A Ruel, 2010-05-15 00:54:45]

On 2010/05/15 00:46:38, davidjames wrote:
> Oops. Actually, self.checkout_root doesn't work, because it doesn't contain my
> gclient.settings. We need to do the search. In patch version #3, I updated it
to
> actually search for the gclient root, but not to update self.gclient_root

ok

lgtm

[davidjames, 2010-05-17 17:20:37]

On 2010/05/15 00:54:45, Marc-Antoine Ruel wrote:
> On 2010/05/15 00:46:38, davidjames wrote:
> > Oops. Actually, self.checkout_root doesn't work, because it doesn't contain
my
> > gclient.settings. We need to do the search. In patch version #3, I updated
it
> to
> > actually search for the gclient root, but not to update self.gclient_root
> 
> ok
> 
> lgtm

Great! Can you commit? I don't have commit access to Chromium yet (only Chrome
OS).

Cheers,

David