Warn if --gclientfile= specifies a with separators.

Warn if --gclientfile= specifies a with separators.
Given that we search for the gclient root in ancestor directories based on the file name, specifying a file with separators doesn't make a lot of sense.

BUG=336946
R=iannucci@chromium.org

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

[Randy Smith (Not in Mondays), 2014-01-30 19:52:15]

Robert: Can you give me initial comments on this patch?  I believe this fixes
the problem in issue 336946 without causing any new ones, but I'm not deeply
familiar with the code.  

Also, this is my first depot_tools patch in a while, so there are some process
issues I'm confused about:

* You show up in the OWNERS file, but codereview.chromium.org is suggesting
darin et al for reviewers, which suggests it thinks that this is a chromium
patch, which suggests I uploaded it wrong.  Any ideas how/what I should do about
it?

* I failed presubmit checks, at first because I didn't have python-coverage
installed, and after I fixed that, because "Coverage.py warning: No data was
collected."  Any idea what I should be doing that I'm not?

Thanks in advance ...

[iannucci, 2014-01-30 20:01:21]

On 2014/01/30 19:52:15, rdsmith wrote:
> Robert: Can you give me initial comments on this patch?  I believe this fixes
> the problem in issue 336946 without causing any new ones, but I'm not deeply
> familiar with the code.  
> 
> Also, this is my first depot_tools patch in a while, so there are some process
> issues I'm confused about:
> 
> * You show up in the OWNERS file, but http://codereview.chromium.org is
suggesting
> darin et al for reviewers, which suggests it thinks that this is a chromium
> patch, which suggests I uploaded it wrong.  Any ideas how/what I should do
about
> it?

Not sure, the Base URL looks fine to me.

> 
> * I failed presubmit checks, at first because I didn't have python-coverage
> installed, and after I fixed that, because "Coverage.py warning: No data was
> collected."  Any idea what I should be doing that I'm not?

Which version of coverage do you have? You should have 3.7+

> 
> Thanks in advance ...


I'm actually not sure this is the right fix... Why not just normalize the input
to the function in the first place by join'ing cwd+filename, then taking the
split of that?

For better or worse (actually: just worse), other projects have been known to
import things from depot_tools, which makes me wary of changing the function
signature of the method here.

[Randy Smith (Not in Mondays), 2014-01-30 20:55:24]

On 2014/01/30 20:01:21, iannucci wrote:
> On 2014/01/30 19:52:15, rdsmith wrote:
> > Robert: Can you give me initial comments on this patch?  I believe this
fixes
> > the problem in issue 336946 without causing any new ones, but I'm not deeply
> > familiar with the code.  
> > 
> > Also, this is my first depot_tools patch in a while, so there are some
process
> > issues I'm confused about:
> > 
> > * You show up in the OWNERS file, but http://codereview.chromium.org is
> suggesting
> > darin et al for reviewers, which suggests it thinks that this is a chromium
> > patch, which suggests I uploaded it wrong.  Any ideas how/what I should do
> about
> > it?
> 
> Not sure, the Base URL looks fine to me.
> 
> > 
> > * I failed presubmit checks, at first because I didn't have python-coverage
> > installed, and after I fixed that, because "Coverage.py warning: No data was
> > collected."  Any idea what I should be doing that I'm not?
> 
> Which version of coverage do you have? You should have 3.7+
> 
> > 
> > Thanks in advance ...
> 
> 
> I'm actually not sure this is the right fix... Why not just normalize the
input
> to the function in the first place by join'ing cwd+filename, then taking the
> split of that?

So that would solve my problem, and if you'd prefer that I'm good with it.  But
looking at the code, it didn't seem like the thing most in line with the intent
of the code.  If someone specifies --gclientfile=mygclientfiles/.gclient-file
and that file isn't there, do you want to be searching up the parent chain for
.gclient-file, or for mygclientfiles/.gclient-file?  

Of course, I look at that case, and I start wondering what they/we really want
the path to be.  If there's a subdirectory mygclientfiles, it's probably not
intended that the gclient root be that subdirectory; it's just a directory for
storing gclient files.  Probably they want the gclient root to be $CWD.  And
that is the way the current code behaves, which calls into question the premise
of my bug report.  

The thing that confused me and lead to my bug report was that, from my
perspective, the directory used by gclient for checkout was different if
--gclientfile was specified than if it wasn't.  But you could also frame what
was happening as that gclient always searched for the gclient file in parent
directories and chose the root based on the directory it found the file from,
and my specification of a gclient file in a different directory was indicating
to gclient that I wanted to checkout in the directory from which that gclient
file could be found at the path I specified (i.e. the interpretation in the last
paragraph).  Which means I shouldn't change the code at all.  

I'd still like to make some mod to make it less likely that someone else will
get in the same situation I did.  Maybe just a warning if gclient ends up using
a different directory for root than the gclient file is in?  WDYT?

> 
> For better or worse (actually: just worse), other projects have been known to
> import things from depot_tools, which makes me wary of changing the function
> signature of the method here.

[iannucci, 2014-01-30 21:18:53]

On 2014/01/30 20:55:24, rdsmith wrote:
> On 2014/01/30 20:01:21, iannucci wrote:
> > On 2014/01/30 19:52:15, rdsmith wrote:
> > > Robert: Can you give me initial comments on this patch?  I believe this
> fixes
> > > the problem in issue 336946 without causing any new ones, but I'm not
deeply
> > > familiar with the code.  
> > > 
> > > Also, this is my first depot_tools patch in a while, so there are some
> process
> > > issues I'm confused about:
> > > 
> > > * You show up in the OWNERS file, but http://codereview.chromium.org is
> > suggesting
> > > darin et al for reviewers, which suggests it thinks that this is a
chromium
> > > patch, which suggests I uploaded it wrong.  Any ideas how/what I should do
> > about
> > > it?
> > 
> > Not sure, the Base URL looks fine to me.
> > 
> > > 
> > > * I failed presubmit checks, at first because I didn't have
python-coverage
> > > installed, and after I fixed that, because "Coverage.py warning: No data
was
> > > collected."  Any idea what I should be doing that I'm not?
> > 
> > Which version of coverage do you have? You should have 3.7+
> > 
> > > 
> > > Thanks in advance ...
> > 
> > 
> > I'm actually not sure this is the right fix... Why not just normalize the
> input
> > to the function in the first place by join'ing cwd+filename, then taking the
> > split of that?
> 
> So that would solve my problem, and if you'd prefer that I'm good with it. 
But
> looking at the code, it didn't seem like the thing most in line with the
intent
> of the code.  If someone specifies --gclientfile=mygclientfiles/.gclient-file
> and that file isn't there, do you want to be searching up the parent chain for
> .gclient-file, or for mygclientfiles/.gclient-file?  
> 
> Of course, I look at that case, and I start wondering what they/we really want
> the path to be.  If there's a subdirectory mygclientfiles, it's probably not
> intended that the gclient root be that subdirectory; it's just a directory for
> storing gclient files.  Probably they want the gclient root to be $CWD.  And
> that is the way the current code behaves, which calls into question the
premise
> of my bug report.  
> 
> The thing that confused me and lead to my bug report was that, from my
> perspective, the directory used by gclient for checkout was different if
> --gclientfile was specified than if it wasn't.  But you could also frame what
> was happening as that gclient always searched for the gclient file in parent
> directories and chose the root based on the directory it found the file from,
> and my specification of a gclient file in a different directory was indicating
> to gclient that I wanted to checkout in the directory from which that gclient
> file could be found at the path I specified (i.e. the interpretation in the
last
> paragraph).  Which means I shouldn't change the code at all.  
> 
> I'd still like to make some mod to make it less likely that someone else will
> get in the same situation I did.  Maybe just a warning if gclient ends up
using
> a different directory for root than the gclient file is in?  WDYT?

Heh, yeah, it's difficult to say what the correct behavior is.  Frankly, I'm not
sure what the intended use case of --gclientfile IS. It certainly implies that
it's for picking a different gclient file, but since the gclientfile and the
'root' directory are typically conflated concepts, it's not clear what the
expectation should be if gclientfile has path components in it. I think it's
intended use is really just for specifying the 'name' of the file (e.g.
.gclient-blink), not a path to that file. In your case, I think you actually
want to pass --gclientfile=.gclient-blink.

In the event of the FindUpwards function, I think that FindUpwards(path,
'../foobar) means 'go up from path, until "../foobar" exists, then stop and
return the path'. I'm not sure changing it is really correct.

It may be worth warning/erroring if gclientfile contains a os.sep though.

> 
> > 
> > For better or worse (actually: just worse), other projects have been known
to
> > import things from depot_tools, which makes me wary of changing the function
> > signature of the method here.

[Randy Smith (Not in Mondays), 2014-01-30 22:18:39]

On 2014/01/30 21:18:53, iannucci wrote:
> On 2014/01/30 20:55:24, rdsmith wrote:
> > On 2014/01/30 20:01:21, iannucci wrote:
> > > On 2014/01/30 19:52:15, rdsmith wrote:
> > > > Robert: Can you give me initial comments on this patch?  I believe this
> > fixes
> > > > the problem in issue 336946 without causing any new ones, but I'm not
> deeply
> > > > familiar with the code.  
> > > > 
> > > > Also, this is my first depot_tools patch in a while, so there are some
> > process
> > > > issues I'm confused about:
> > > > 
> > > > * You show up in the OWNERS file, but http://codereview.chromium.org is
> > > suggesting
> > > > darin et al for reviewers, which suggests it thinks that this is a
> chromium
> > > > patch, which suggests I uploaded it wrong.  Any ideas how/what I should
do
> > > about
> > > > it?
> > > 
> > > Not sure, the Base URL looks fine to me.
> > > 
> > > > 
> > > > * I failed presubmit checks, at first because I didn't have
> python-coverage
> > > > installed, and after I fixed that, because "Coverage.py warning: No data
> was
> > > > collected."  Any idea what I should be doing that I'm not?
> > > 
> > > Which version of coverage do you have? You should have 3.7+
> > > 
> > > > 
> > > > Thanks in advance ...
> > > 
> > > 
> > > I'm actually not sure this is the right fix... Why not just normalize the
> > input
> > > to the function in the first place by join'ing cwd+filename, then taking
the
> > > split of that?
> > 
> > So that would solve my problem, and if you'd prefer that I'm good with it. 
> But
> > looking at the code, it didn't seem like the thing most in line with the
> intent
> > of the code.  If someone specifies
--gclientfile=mygclientfiles/.gclient-file
> > and that file isn't there, do you want to be searching up the parent chain
for
> > .gclient-file, or for mygclientfiles/.gclient-file?  
> > 
> > Of course, I look at that case, and I start wondering what they/we really
want
> > the path to be.  If there's a subdirectory mygclientfiles, it's probably not
> > intended that the gclient root be that subdirectory; it's just a directory
for
> > storing gclient files.  Probably they want the gclient root to be $CWD.  And
> > that is the way the current code behaves, which calls into question the
> premise
> > of my bug report.  
> > 
> > The thing that confused me and lead to my bug report was that, from my
> > perspective, the directory used by gclient for checkout was different if
> > --gclientfile was specified than if it wasn't.  But you could also frame
what
> > was happening as that gclient always searched for the gclient file in parent
> > directories and chose the root based on the directory it found the file
from,
> > and my specification of a gclient file in a different directory was
indicating
> > to gclient that I wanted to checkout in the directory from which that
gclient
> > file could be found at the path I specified (i.e. the interpretation in the
> last
> > paragraph).  Which means I shouldn't change the code at all.  
> > 
> > I'd still like to make some mod to make it less likely that someone else
will
> > get in the same situation I did.  Maybe just a warning if gclient ends up
> using
> > a different directory for root than the gclient file is in?  WDYT?
> 
> Heh, yeah, it's difficult to say what the correct behavior is.  Frankly, I'm
not
> sure what the intended use case of --gclientfile IS. It certainly implies that
> it's for picking a different gclient file, but since the gclientfile and the
> 'root' directory are typically conflated concepts, it's not clear what the
> expectation should be if gclientfile has path components in it. I think it's
> intended use is really just for specifying the 'name' of the file (e.g.
> .gclient-blink), not a path to that file. In your case, I think you actually
> want to pass --gclientfile=.gclient-blink.
> 
> In the event of the FindUpwards function, I think that FindUpwards(path,
> '../foobar) means 'go up from path, until "../foobar" exists, then stop and
> return the path'. I'm not sure changing it is really correct.
> 
> It may be worth warning/erroring if gclientfile contains a os.sep though.

This all makes sense to me, and I've switched that patch over to implementing
that error.  PTAL?

> 
> > 
> > > 
> > > For better or worse (actually: just worse), other projects have been known
> to
> > > import things from depot_tools, which makes me wary of changing the
function
> > > signature of the method here.

[iannucci, 2014-01-31 08:56:09]

lgtm

https://codereview.chromium.org/150633004/diff/40001/gclient.py
File gclient.py (right):

https://codereview.chromium.org/150633004/diff/40001/gclient.py#newcode1865
gclient.py:1865: self.error('--gclientfile target must be a pure filename')
maybe "--gclientfile must be a filename, not a path"?

[Randy Smith (Not in Mondays), 2014-02-04 19:29:20]

Thanks!

Two, hopefully quick, questions:
* Could you give me some guidance on submission?  It doesn't look like the
(depot_tools equivalent of) the CQ will work, since this CL thinks that I don't
have good reviewers yet, and I'm guessing "git cl dcommit" won't work because it
looks like depot_tools is a pure git repository.  

* What testing is needed before landing?  My presubmit checks continue to fail
for what looks like a configuration reason ("Coverage.py warning: No data was
collected.").

https://codereview.chromium.org/150633004/diff/40001/gclient.py
File gclient.py (right):

https://codereview.chromium.org/150633004/diff/40001/gclient.py#newcode1865
gclient.py:1865: self.error('--gclientfile target must be a pure filename')
On 2014/01/31 08:56:09, iannucci wrote:
> maybe "--gclientfile must be a filename, not a path"?

Done.

[iannucci, 2014-02-04 21:38:18]

On 2014/02/04 19:29:20, rdsmith wrote:
> Thanks!
> 
> Two, hopefully quick, questions:
> * Could you give me some guidance on submission?  It doesn't look like the
> (depot_tools equivalent of) the CQ will work, since this CL thinks that I
don't
> have good reviewers yet, and I'm guessing "git cl dcommit" won't work because
it
> looks like depot_tools is a pure git repository.  
> 

The CQ should work fine for this CL

> * What testing is needed before landing?  My presubmit checks continue to fail
> for what looks like a configuration reason ("Coverage.py warning: No data was
> collected.").

Install the python-coverage module (with pip). It should be at least v3.7. If
you're on ubuntu, you may need to upgrade pip (make sure it's installed with
apt-get, then run `pip install --upgrade pip`).

> 
> https://codereview.chromium.org/150633004/diff/40001/gclient.py
> File gclient.py (right):
> 
> https://codereview.chromium.org/150633004/diff/40001/gclient.py#newcode1865
> gclient.py:1865: self.error('--gclientfile target must be a pure filename')
> On 2014/01/31 08:56:09, iannucci wrote:
> > maybe "--gclientfile must be a filename, not a path"?
> 
> Done.

[Randy Smith (Not in Mondays), 2014-02-05 17:41:22]

"sudo apt-get install pip" tells me "E: Unable to locate package pip" ??


On Tue, Feb 4, 2014 at 4:38 PM, <iannucci@chromium.org> wrote:

> On 2014/02/04 19:29:20, rdsmith wrote:
>
>> Thanks!
>>
>
>  Two, hopefully quick, questions:
>> * Could you give me some guidance on submission?  It doesn't look like the
>> (depot_tools equivalent of) the CQ will work, since this CL thinks that I
>>
> don't
>
>> have good reviewers yet, and I'm guessing "git cl dcommit" won't work
>> because
>>
> it
>
>> looks like depot_tools is a pure git repository.
>>
>
>
> The CQ should work fine for this CL
>
>
>  * What testing is needed before landing?  My presubmit checks continue to
>> fail
>> for what looks like a configuration reason ("Coverage.py warning: No data
>> was
>> collected.").
>>
>
> Install the python-coverage module (with pip). It should be at least v3.7.
> If
> you're on ubuntu, you may need to upgrade pip (make sure it's installed
> with
> apt-get, then run `pip install --upgrade pip`).
>
>
>
>  https://codereview.chromium.org/150633004/diff/40001/gclient.py
>> File gclient.py (right):
>>
>
>  https://codereview.chromium.org/150633004/diff/40001/
>> gclient.py#newcode1865
>> gclient.py:1865: self.error('--gclientfile target must be a pure
>> filename')
>> On 2014/01/31 08:56:09, iannucci wrote:
>> > maybe "--gclientfile must be a filename, not a path"?
>>
>
>  Done.
>>
>
>
>
> https://codereview.chromium.org/150633004/
>

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[iannucci, 2014-02-05 18:50:39]

I think it's called python-pip in apt

[Randy Smith (Not in Mondays), 2014-02-05 19:13:47]

Well, I installed that, but I'm still getting "Coverage.py warning: No data was
collected." followed by "FATAL: not at 100% coverage."  I'm also getting an
actual test failure:

tests/git_number_test.py (0.09s) failed
Traceback (most recent call last):
  File "tests/git_number_test.py", line 85, in <module>
    '3.7'
  File
"/usr/local/google/home/rdsmith/Sandboxen/depot_tools/testing_support/coverage_utils.py",
line 38, in covered_main
    if not coverage.collector.CTracer:
AttributeError: 'module' object has no attribute 'CTracer'

But on the bright side, it really looks like it's running tests now, the
presubmit check found an indentation error (fixed), and I don't see any way the
above issues could be related to what I've done, so unless you object, I'm
inclined to land with what I have.

[iannucci, 2014-02-05 19:18:56]

landing through CQ will run all presubmits

lgtm

[Randy Smith (Not in Mondays), 2014-02-05 19:25:22]

The CQ bit was checked by rdsmith@chromium.org

[commit-bot: I haz the power, 2014-02-05 19:25:28]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/rdsmith@chromium.org/150633004/120001

[commit-bot: I haz the power, 2014-02-05 19:28:22]

Change committed as 249077