Error out in `gclient config` if --name is relative

Error out in `gclient config` if --name is relative

This helps prevent behavior such as setting '--name .' so that the .gclient
file contains '.' as the name of the solution, and the .gclient file ends
up a sibling of the .git file, rather than a sibling of the git checkout's
containing directory.

R=iannucci@chromium.org, thestig@chromium.org

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

[Lei Zhang, 2015-10-20 00:55:21]

lgtm

Would you mind holding off on landing this until we had a chance to build
consensus and give PDFium developers notice to stop doing this?

https://codereview.chromium.org/1406053003/diff/1/gclient.py
File gclient.py (right):

https://codereview.chromium.org/1406053003/diff/1/gclient.py#newcode1892
gclient.py:1892: pieces = name.split(os.sep)
just roll |pieces| into any() ?

[agable, 2015-10-20 01:05:27]

Yep absolutely willing to hold off.

I tried to also enforce it at a deeper level, having the WorkItem code raise an
exception if you asked it to work on an item whose name was a relative path, but
that caused all sorts of tests to fail (because the exception is being caught
and suppressed somewhere, resulting in some values being NoneType, I think) so
I've given up on that rather than invest more time.

https://codereview.chromium.org/1406053003/diff/1/gclient.py
File gclient.py (right):

https://codereview.chromium.org/1406053003/diff/1/gclient.py#newcode1892
gclient.py:1892: pieces = name.split(os.sep)
On 2015/10/20 at 00:55:21, Lei Zhang wrote:
> just roll |pieces| into any() ?

Done.

[iannucci, 2015-10-20 01:07:12]

fwiw, lgtm

[agable, 2015-10-27 20:25:26]

Seems like pdfium is all switched over. Happy to land this now?

[Lei Zhang, 2015-10-27 20:56:55]

On 2015/10/27 20:25:26, agable wrote:
> Seems like pdfium is all switched over. Happy to land this now?

Yep, should be good to go.

[agable, 2015-10-27 20:58:18]

The CQ bit was checked by agable@chromium.org

[agable, 2015-10-27 20:58:18]

The patchset sent to the CQ was uploaded after l-g-t-m from thestig@chromium.org
Link to the patchset: https://codereview.chromium.org/1406053003/#ps20001
(title: "Comments")

[commit-bot: I haz the power, 2015-10-27 20:58:23]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1406053003/20001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1406053003/20001

[commit-bot: I haz the power, 2015-10-27 21:02:50]

Committed patchset #2 (id:20001) as
http://src.chromium.org/viewvc/chrome?view=rev&revision=297343

[mtklein_C, 2015-11-03 13:27:01]

On 2015/10/27 at 21:02:50, commit-bot wrote:
> Committed patchset #2 (id:20001) as
http://src.chromium.org/viewvc/chrome?view=rev&revision=297343

Hey, we in Skia kind of liked using '--name .'.  Is that a fundamentally broken
thing?  It seemed to be working fine for us.

I don't suppose it's supported to do something like
    $ gclient config --unmanaged 'https://skia.googlesource.com/skia'
    $ printf ',s/"skia"/"."/\nwq\n' | ed .gclient
is it?

[agable, 2015-11-03 17:44:20]

On 2015/11/03 at 13:27:01, mtklein wrote:
> On 2015/10/27 at 21:02:50, commit-bot wrote:
> > Committed patchset #2 (id:20001) as
http://src.chromium.org/viewvc/chrome?view=rev&revision=297343
> 
> Hey, we in Skia kind of liked using '--name .'.  Is that a fundamentally
broken thing?  It seemed to be working fine for us.
> 
> I don't suppose it's supported to do something like
>     $ gclient config --unmanaged 'https://skia.googlesource.com/skia'
>     $ printf ',s/"skia"/"."/\nwq\n' | ed .gclient
> is it?

Yeah, it's a fundamentally broken thing. Gclient was never supposed to support
that behavior, various wrappers around gclient disallow that behavior already,
and it's just historical accident that we didn't notice that behavior worked
(and was actively used) until recently. Had a big discussion with thestig@,
jam@, and a bunch of folks in pdfium@ who were using this behavior, and
convinced them that using this behavior was a Bad Idea. We had no idea anyone
else was using it too.

The surface point here is that gclient is simply bad. It was a bad design
decision (years and years ago) to have to kinds of config files: .gclient and
DEPS. These should have been unified into a single format (DEPS), and the flow
should be more like "git clone foo && cd foo && gclient sync".

However, that's not how it works today. As it is, .gclient files and DEPS files
are two very different beasts. In particular, by default, all paths in all DEPS
files are evaluated *relative to the location of the .gclient file*. You'll note
that all the paths in Chromium's DEPS file start with "src/". Keeping the
.gclient file in its own directory to keep an airgap between your repository and
the rest of your filesystem is a purposeful decision.

Anyway, if you really want to use sed to modify your .gclient config, I can't
stop you. But I also can't guarantee that we won't break that behavior at any
time in the future (in fact, this CL almost included enforcement that would have
prevented that as well, but it was a bit too gnarly to put in quickly).

[Jamie Madill, 2015-11-05 15:02:17]

On 2015/11/03 17:44:20, agable wrote:
> On 2015/11/03 at 13:27:01, mtklein wrote:
> > On 2015/10/27 at 21:02:50, commit-bot wrote:
> > > Committed patchset #2 (id:20001) as
> http://src.chromium.org/viewvc/chrome?view=rev&revision=297343
> > 
> > Hey, we in Skia kind of liked using '--name .'.  Is that a fundamentally
> broken thing?  It seemed to be working fine for us.
> > 
> > I don't suppose it's supported to do something like
> >     $ gclient config --unmanaged 'https://skia.googlesource.com/skia'
> >     $ printf ',s/"skia"/"."/\nwq\n' | ed .gclient
> > is it?
> 
> Yeah, it's a fundamentally broken thing. Gclient was never supposed to support
> that behavior, various wrappers around gclient disallow that behavior already,
> and it's just historical accident that we didn't notice that behavior worked
> (and was actively used) until recently. Had a big discussion with thestig@,
> jam@, and a bunch of folks in pdfium@ who were using this behavior, and
> convinced them that using this behavior was a Bad Idea. We had no idea anyone
> else was using it too.
> 
> The surface point here is that gclient is simply bad. It was a bad design
> decision (years and years ago) to have to kinds of config files: .gclient and
> DEPS. These should have been unified into a single format (DEPS), and the flow
> should be more like "git clone foo && cd foo && gclient sync".
> 
> However, that's not how it works today. As it is, .gclient files and DEPS
files
> are two very different beasts. In particular, by default, all paths in all
DEPS
> files are evaluated *relative to the location of the .gclient file*. You'll
note
> that all the paths in Chromium's DEPS file start with "src/". Keeping the
> .gclient file in its own directory to keep an airgap between your repository
and
> the rest of your filesystem is a purposeful decision.
> 
> Anyway, if you really want to use sed to modify your .gclient config, I can't
> stop you. But I also can't guarantee that we won't break that behavior at any
> time in the future (in fact, this CL almost included enforcement that would
have
> prevented that as well, but it was a bit too gnarly to put in quickly).

Hi, this change also broke ANGLE. Our workaround might be to use absolute paths,
but
without changing our whole project layout, our .gclient file will always be
sibling
to an unmanaged .git folder. Please fix!

[mtklein, 2015-11-05 20:45:47]

On 2015/11/03 at 17:44:20, agable wrote:
> On 2015/11/03 at 13:27:01, mtklein wrote:
> > On 2015/10/27 at 21:02:50, commit-bot wrote:
> > > Committed patchset #2 (id:20001) as
http://src.chromium.org/viewvc/chrome?view=rev&revision=297343
> > 
> > Hey, we in Skia kind of liked using '--name .'.  Is that a fundamentally
broken thing?  It seemed to be working fine for us.
> > 
> > I don't suppose it's supported to do something like
> >     $ gclient config --unmanaged 'https://skia.googlesource.com/skia'
> >     $ printf ',s/"skia"/"."/\nwq\n' | ed .gclient
> > is it?
> 
> Yeah, it's a fundamentally broken thing. Gclient was never supposed to support
that behavior, various wrappers around gclient disallow that behavior already,
and it's just historical accident that we didn't notice that behavior worked
(and was actively used) until recently. Had a big discussion with thestig@,
jam@, and a bunch of folks in pdfium@ who were using this behavior, and
convinced them that using this behavior was a Bad Idea. We had no idea anyone
else was using it too.
> 
> The surface point here is that gclient is simply bad. It was a bad design
decision (years and years ago) to have to kinds of config files: .gclient and
DEPS. These should have been unified into a single format (DEPS), and the flow
should be more like "git clone foo && cd foo && gclient sync".
> 
> However, that's not how it works today. As it is, .gclient files and DEPS
files are two very different beasts. In particular, by default, all paths in all
DEPS files are evaluated *relative to the location of the .gclient file*. You'll
note that all the paths in Chromium's DEPS file start with "src/". Keeping the
.gclient file in its own directory to keep an airgap between your repository and
the rest of your filesystem is a purposeful decision.
> 
> Anyway, if you really want to use sed to modify your .gclient config, I can't
stop you. But I also can't guarantee that we won't break that behavior at any
time in the future (in fact, this CL almost included enforcement that would have
prevented that as well, but it was a bit too gnarly to put in quickly).

Ah, this all SGTM for Skia.  We don't really use anything from gclient beyond
just syncing DEPS.  I think we can just stop using gclient and sync our DEPS
with a Python script.

[agable, 2015-11-05 20:47:04]

On 2015/11/05 at 20:45:47, mtklein wrote:
> 
> Ah, this all SGTM for Skia.  We don't really use anything from gclient beyond
just syncing DEPS.  I think we can just stop using gclient and sync our DEPS
with a Python script.

I mean, please don't *actually* do that. gclient is the python script that you
want to use to sync DEPS. The path that I laid out is the path that we hope to
take gclient down in the near future; please don't roll your own :)

[mtklein, 2015-11-05 20:51:48]

On 2015/11/05 at 20:47:04, agable wrote:
> On 2015/11/05 at 20:45:47, mtklein wrote:
> > 
> > Ah, this all SGTM for Skia.  We don't really use anything from gclient
beyond just syncing DEPS.  I think we can just stop using gclient and sync our
DEPS with a Python script.
> 
> I mean, please don't *actually* do that. gclient is the python script that you
want to use to sync DEPS. The path that I laid out is the path that we hope to
take gclient down in the near future; please don't roll your own :)

No, really, I mean to actually do that.  `git clone foo && cd foo && gclient
sync` is exactly what we were doing before, and if we can't do that anymore, the
least disruptive change is something like `git clone foo && cd foo &&
./not-gclient sync`.

[agable, 2015-11-05 21:03:36]

On 2015/11/05 at 20:51:48, mtklein wrote:
> On 2015/11/05 at 20:47:04, agable wrote:
> > On 2015/11/05 at 20:45:47, mtklein wrote:
> > > 
> > > Ah, this all SGTM for Skia.  We don't really use anything from gclient
beyond just syncing DEPS.  I think we can just stop using gclient and sync our
DEPS with a Python script.
> > 
> > I mean, please don't *actually* do that. gclient is the python script that
you want to use to sync DEPS. The path that I laid out is the path that we hope
to take gclient down in the near future; please don't roll your own :)
> 
> No, really, I mean to actually do that.  `git clone foo && cd foo && gclient
sync` is exactly what we were doing before, and if we can't do that anymore, the
least disruptive change is something like `git clone foo && cd foo &&
./not-gclient sync`.

And how were you doing that? The skia repo doesn't appear to have a .gclient
file checked in, so you have to run gclient config at some point. (And if you
do/did have a .gclient file checked in, I'm a bit terrified.)

Additionally, I'm confused when you say you can't use your old system anymore.
You can. All this change did was prevent you from running 'gclient config
--name=.' Already in-place checkouts should be completely unaffected by this
change.

If that's not true, I'm happy to revert it. In the mean time, I'd like to work
with you to bring Skia's repository layout in line with what gclient expects.