Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
61623008:
If the destination directory doesn't contain the desired repo, delete it (Closed)
|
Created:
7 years, 1 month ago by borenet Modified:
6 years, 11 months ago CC:
chromium-reviews, Dirk Pranke, cmp-cc_chromium.org, M-A Ruel, iannucci+depot_tools_chromium.org, ilevy-cc_chromium.org, reed1 Base URL:
https://chromium.googlesource.com/chromium/tools/depot_tools.git@master Visibility:
Public. |
DescriptionIf the destination directory doesn't contain the desired repo, delete it
Basic functionality - this may not be in quite the right place.
Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=245404
Patch Set 1 #Patch Set 2 : Rebase #
Total comments: 16
Patch Set 3 : Add prompt when used in interactive mode without --force, address comments #
Total comments: 7
Patch Set 4 : Use raw_input(prompt) instead of print #Patch Set 5 : Remove print() #Patch Set 6 : Error out if the conflicting directory isn't deleted. #
Total comments: 10
Patch Set 7 : Address GetRemoteURL comments #
Total comments: 8
Patch Set 8 : Remove URL rewriting #Patch Set 9 : Remove no-longer-used function #
Total comments: 4
Patch Set 10 : Address comments from meeting #Patch Set 11 : Address comments from meeting (first upload failed) #
Total comments: 8
Patch Set 12 : Guard with CHROME_HEADLESS #Patch Set 13 : Guard with CHROME_HEADLESS (retry upload) #Patch Set 14 : Guard with CHROME_HEADLESS (retry #2) #Patch Set 15 : Rebase, fix conflicts, remove unused variable #
Total comments: 16
Patch Set 16 : Move some logic into functions #Patch Set 17 : Move some logic into functions (retry upload) #
Total comments: 13
Patch Set 18 : Address maruel's comments #Patch Set 19 : Fix failing tests #
Total comments: 8
Patch Set 20 : Fix line length #
Messages
Total messages: 45 (0 generated)
https://codereview.chromium.org/61623008/diff/20001/gclient.py File gclient.py (right): https://codereview.chromium.org/61623008/diff/20001/gclient.py#newcode644 gclient.py:644: This may not be in the right place. Any alternative suggestions? I kept things simple for now, but this could also be placed behind a flag so that this only happens on the bots. https://codereview.chromium.org/61623008/diff/20001/gclient.py#newcode655 gclient.py:655: gclient_utils.rmtree(dest_dir) Alternatively, we could move into a temporary directory to avoid clobbering things. I think ilevy@ mentioned using such a temporary directory and swapping out the contents of the directory as appropriate. For example, when syncing Skia forward across the DEPS change, third_party/skia gets moved into ".deleteme". If, at some later time, we sync backward across the DEPS change, we swap the current contents of third_party/skia with those that we previously moved into ".deleteme" before syncing. This strikes me as tricky to pull off correctly, and I'm not sure that it will save us all that much for a small (<50M) repo.
This looks good to me, though I'm still thinking through the potential impact on bots/developers. I think this is almost always the right behavior for bots, and is /probably/ always the best behavior for devs, though I'm not entirely sure. [I really wish there was a '--bot' flag which toggled 'do it right' v. 'do it nicely'. '--force' kind-of does that, but it's a bit ill-defined in that regard.] https://codereview.chromium.org/61623008/diff/20001/gclient.py File gclient.py (right): https://codereview.chromium.org/61623008/diff/20001/gclient.py#newcode644 gclient.py:644: On 2013/11/12 19:33:37, borenet wrote: > This may not be in the right place. Any alternative suggestions? > > I kept things simple for now, but this could also be placed behind a flag so > that this only happens on the bots. the 'force' flag would probably be sufficient https://codereview.chromium.org/61623008/diff/20001/gclient.py#newcode653 gclient.py:653: logging.debug('%s does not contain a checkout of %s. Removing.' % I think this should actually be at least a warn, since it's destructive. [if we don't end up placing behind --force] We should probably also prompt if stdout is not closed or --force is not supplied, to avoid nuking developer changes. https://codereview.chromium.org/61623008/diff/20001/gclient.py#newcode655 gclient.py:655: gclient_utils.rmtree(dest_dir) On 2013/11/12 19:33:37, borenet wrote: > Alternatively, we could move into a temporary directory to avoid clobbering > things. I think ilevy@ mentioned using such a temporary directory and swapping > out the contents of the directory as appropriate. For example, when syncing > Skia forward across the DEPS change, third_party/skia gets moved into > ".deleteme". If, at some later time, we sync backward across the DEPS change, > we swap the current contents of third_party/skia with those that we previously > moved into ".deleteme" before syncing. This strikes me as tricky to pull off > correctly, and I'm not sure that it will save us all that much for a small > (<50M) repo. This sounds like a lot of complexity to me with very minimal upside (aka premature optimization). I think git_cache would obviate the need for this, since the new clone/checkout would be relatively lightweight. Since skia is already a git repo (right?) it would get this for free just by setting git_cache (which I believe all the bots do already). Of course, this will not be true for non-git repos. But IMO the solution there should be to switch the bots to git checkouts, not to add complexity to gclient. https://codereview.chromium.org/61623008/diff/20001/tests/gclient_scm_test.py File tests/gclient_scm_test.py (right): https://codereview.chromium.org/61623008/diff/20001/tests/gclient_scm_test.py... tests/gclient_scm_test.py:1222: origin %s (push) nit: you could do %(url)s in the string and then supply {'url': git_scm.url} as the interpolation argument to be a bit more concise/clear Also, I'm generally uneasy with gclient's obsession with the maintained remote being 'origin'. But that's not your fault :) https://codereview.chromium.org/61623008/diff/20001/tests/gclient_smoketest.py File tests/gclient_smoketest.py (right): https://codereview.chromium.org/61623008/diff/20001/tests/gclient_smoketest.p... tests/gclient_smoketest.py:1363: expected = sorted([ not sure the sorted is necessary, since the prefixes can just be sorted in the test.
Uploaded patch set 3. https://codereview.chromium.org/61623008/diff/20001/gclient.py File gclient.py (right): https://codereview.chromium.org/61623008/diff/20001/gclient.py#newcode644 gclient.py:644: On 2013/11/13 06:26:30, iannucci wrote: > On 2013/11/12 19:33:37, borenet wrote: > > This may not be in the right place. Any alternative suggestions? > > > > I kept things simple for now, but this could also be placed behind a flag so > > that this only happens on the bots. > > the 'force' flag would probably be sufficient Done. https://codereview.chromium.org/61623008/diff/20001/gclient.py#newcode653 gclient.py:653: logging.debug('%s does not contain a checkout of %s. Removing.' % On 2013/11/13 06:26:30, iannucci wrote: > I think this should actually be at least a warn, since it's destructive. > > [if we don't end up placing behind --force] We should probably also prompt if > stdout is not closed or --force is not supplied, to avoid nuking developer > changes. Done. https://codereview.chromium.org/61623008/diff/20001/gclient.py#newcode655 gclient.py:655: gclient_utils.rmtree(dest_dir) On 2013/11/13 06:26:30, iannucci wrote: > On 2013/11/12 19:33:37, borenet wrote: > > Alternatively, we could move into a temporary directory to avoid clobbering > > things. I think ilevy@ mentioned using such a temporary directory and > swapping > > out the contents of the directory as appropriate. For example, when syncing > > Skia forward across the DEPS change, third_party/skia gets moved into > > ".deleteme". If, at some later time, we sync backward across the DEPS change, > > we swap the current contents of third_party/skia with those that we previously > > moved into ".deleteme" before syncing. This strikes me as tricky to pull off > > correctly, and I'm not sure that it will save us all that much for a small > > (<50M) repo. > > This sounds like a lot of complexity to me with very minimal upside (aka > premature optimization). I think git_cache would obviate the need for this, > since the new clone/checkout would be relatively lightweight. Since skia is > already a git repo (right?) it would get this for free just by setting git_cache > (which I believe all the bots do already). > > Of course, this will not be true for non-git repos. But IMO the solution there > should be to switch the bots to git checkouts, not to add complexity to gclient. Agreed. I think the most I'd want to do is move into some ".deleteme" directory. https://codereview.chromium.org/61623008/diff/20001/tests/gclient_scm_test.py File tests/gclient_scm_test.py (right): https://codereview.chromium.org/61623008/diff/20001/tests/gclient_scm_test.py... tests/gclient_scm_test.py:1222: origin %s (push) On 2013/11/13 06:26:30, iannucci wrote: > nit: you could do %(url)s in the string and then supply {'url': git_scm.url} as > the interpolation argument to be a bit more concise/clear > > Also, I'm generally uneasy with gclient's obsession with the maintained remote > being 'origin'. But that's not your fault :) Done. I'll leave the "origin" --> REMOTE_NAME refactoring for a future change. https://codereview.chromium.org/61623008/diff/20001/tests/gclient_smoketest.py File tests/gclient_smoketest.py (right): https://codereview.chromium.org/61623008/diff/20001/tests/gclient_smoketest.p... tests/gclient_smoketest.py:1363: expected = sorted([ On 2013/11/13 06:26:30, iannucci wrote: > not sure the sorted is necessary, since the prefixes can just be sorted in the > test. Done. https://codereview.chromium.org/61623008/diff/80001/gclient.py File gclient.py (right): https://codereview.chromium.org/61623008/diff/80001/gclient.py#newcode673 gclient.py:673: I'd like to point out that this causes some strange (to me) behavior when we don't delete the destination directory: if no files conflict, you end up with *both* checkouts. In my test I tried to add verification that the syncs failed without --force and then succeeded with --force. However, since I'm checking out two different repos (one in git, the other in svn), nothing conflicted and I ended up with both checkouts in the same directory. I think in practice this won't be an issue, but it's worth thinking about.
https://codereview.chromium.org/61623008/diff/80001/gclient.py File gclient.py (right): https://codereview.chromium.org/61623008/diff/80001/gclient.py#newcode662 gclient.py:662: usr_input = raw_input() raw_input() takes a prompt, which would be a bit better than the print, I think. https://codereview.chromium.org/61623008/diff/80001/gclient.py#newcode673 gclient.py:673: On 2013/11/13 19:12:24, borenet wrote: > I'd like to point out that this causes some strange (to me) behavior when we > don't delete the destination directory: if no files conflict, you end up with > *both* checkouts. In my test I tried to add verification that the syncs failed > without --force and then succeeded with --force. However, since I'm checking > out two different repos (one in git, the other in svn), nothing conflicted and I > ended up with both checkouts in the same directory. I think in practice this > won't be an issue, but it's worth thinking about. I'm inclined to just abort if we didn't end up deleting by this point. If it's a dev, then they need to take care of it manually (or press 'y'), and if it's a bot, it's running with '--force'.
Uploaded patch set 5. https://codereview.chromium.org/61623008/diff/80001/gclient.py File gclient.py (right): https://codereview.chromium.org/61623008/diff/80001/gclient.py#newcode662 gclient.py:662: usr_input = raw_input() On 2013/11/14 07:38:15, iannucci wrote: > raw_input() takes a prompt, which would be a bit better than the print, I think. Done. https://codereview.chromium.org/61623008/diff/80001/gclient.py#newcode673 gclient.py:673: On 2013/11/14 07:38:15, iannucci wrote: > On 2013/11/13 19:12:24, borenet wrote: > > I'd like to point out that this causes some strange (to me) behavior when we > > don't delete the destination directory: if no files conflict, you end up with > > *both* checkouts. In my test I tried to add verification that the syncs > failed > > without --force and then succeeded with --force. However, since I'm checking > > out two different repos (one in git, the other in svn), nothing conflicted and > I > > ended up with both checkouts in the same directory. I think in practice this > > won't be an issue, but it's worth thinking about. > > I'm inclined to just abort if we didn't end up deleting by this point. If it's a > dev, then they need to take care of it manually (or press 'y'), and if it's a > bot, it's running with '--force'. Sounds fine to me, but I think it might also be okay for the user to take responsibility for what happens if they don't take the recommended action.
https://codereview.chromium.org/61623008/diff/80001/gclient.py File gclient.py (right): https://codereview.chromium.org/61623008/diff/80001/gclient.py#newcode673 gclient.py:673: On 2013/11/14 18:31:21, borenet wrote: > On 2013/11/14 07:38:15, iannucci wrote: > > On 2013/11/13 19:12:24, borenet wrote: > > > I'd like to point out that this causes some strange (to me) behavior when we > > > don't delete the destination directory: if no files conflict, you end up > with > > > *both* checkouts. In my test I tried to add verification that the syncs > > failed > > > without --force and then succeeded with --force. However, since I'm > checking > > > out two different repos (one in git, the other in svn), nothing conflicted > and > > I > > > ended up with both checkouts in the same directory. I think in practice > this > > > won't be an issue, but it's worth thinking about. > > > > I'm inclined to just abort if we didn't end up deleting by this point. If it's > a > > dev, then they need to take care of it manually (or press 'y'), and if it's a > > bot, it's running with '--force'. > > Sounds fine to me, but I think it might also be okay for the user to take > responsibility for what happens if they don't take the recommended action. Yeah, I agree, the thing is that the gclient solution will be in a totally weird state, and it won't be clear what to do about it. Moreover, it will appear as if gclient 'worked', and so the user may not even know that they're in a weird state for along time afterwards. If we abort, then the user will be manually forced to move the repo aside, delete it, or re-run gclient and press 'y' instead of 'n'.
Uploaded patch set 6. https://codereview.chromium.org/61623008/diff/80001/gclient.py File gclient.py (right): https://codereview.chromium.org/61623008/diff/80001/gclient.py#newcode673 gclient.py:673: On 2013/11/14 18:45:25, iannucci wrote: > On 2013/11/14 18:31:21, borenet wrote: > > On 2013/11/14 07:38:15, iannucci wrote: > > > On 2013/11/13 19:12:24, borenet wrote: > > > > I'd like to point out that this causes some strange (to me) behavior when > we > > > > don't delete the destination directory: if no files conflict, you end up > > with > > > > *both* checkouts. In my test I tried to add verification that the syncs > > > failed > > > > without --force and then succeeded with --force. However, since I'm > > checking > > > > out two different repos (one in git, the other in svn), nothing conflicted > > and > > > I > > > > ended up with both checkouts in the same directory. I think in practice > > this > > > > won't be an issue, but it's worth thinking about. > > > > > > I'm inclined to just abort if we didn't end up deleting by this point. If > it's > > a > > > dev, then they need to take care of it manually (or press 'y'), and if it's > a > > > bot, it's running with '--force'. > > > > Sounds fine to me, but I think it might also be okay for the user to take > > responsibility for what happens if they don't take the recommended action. > > Yeah, I agree, the thing is that the gclient solution will be in a totally weird > state, and it won't be clear what to do about it. Moreover, it will appear as if > gclient 'worked', and so the user may not even know that they're in a weird > state for along time afterwards. If we abort, then the user will be manually > forced to move the repo aside, delete it, or re-run gclient and press 'y' > instead of 'n'. SGTM. Changed to raise Error if the directory isn't deleted.
Sorry i didn't look closely at the remote url methods before :( https://codereview.chromium.org/61623008/diff/260001/gclient.py File gclient.py (right): https://codereview.chromium.org/61623008/diff/260001/gclient.py#newcode655 gclient.py:655: if sys.__stdout__.isatty(): I would combine these two conditions, I think https://codereview.chromium.org/61623008/diff/260001/gclient.py#newcode657 gclient.py:657: while usr_input not in ('y', 'n'): maybe while `usr_input.lower() not in` and initialize usr_input to '' https://codereview.chromium.org/61623008/diff/260001/gclient.py#newcode661 gclient.py:661: '(y/n)' % (dest_dir, url)) I think you may want to re-formulate the end of this prompt '...canlceled (y/n): ' https://codereview.chromium.org/61623008/diff/260001/gclient_scm.py File gclient_scm.py (right): https://codereview.chromium.org/61623008/diff/260001/gclient_scm.py#newcode227 gclient_scm.py:227: return None Maybe just `git config remote.origin.url` ? Also I think there's a utility method for getting config entries. Sorry I didn't mention this earlier :( https://codereview.chromium.org/61623008/diff/260001/gclient_scm.py#newcode1104 gclient_scm.py:1104: return None I think we should probably rely on the XML version of info. I think there's a method which does that already (CaptureLocalInfo)
Sorry for the delay - I've been preoccupied by the office move. https://codereview.chromium.org/61623008/diff/260001/gclient.py File gclient.py (right): https://codereview.chromium.org/61623008/diff/260001/gclient.py#newcode655 gclient.py:655: if sys.__stdout__.isatty(): On 2013/11/15 18:25:25, iannucci wrote: > I would combine these two conditions, I think Done. https://codereview.chromium.org/61623008/diff/260001/gclient.py#newcode657 gclient.py:657: while usr_input not in ('y', 'n'): On 2013/11/15 18:25:25, iannucci wrote: > maybe while `usr_input.lower() not in` and initialize usr_input to '' Changed to do raw_input(...).lower() since I would otherwise need to do user_input.lower() twice. https://codereview.chromium.org/61623008/diff/260001/gclient.py#newcode661 gclient.py:661: '(y/n)' % (dest_dir, url)) On 2013/11/15 18:25:25, iannucci wrote: > I think you may want to re-formulate the end of this prompt > > '...canlceled (y/n): ' Done. https://codereview.chromium.org/61623008/diff/260001/gclient_scm.py File gclient_scm.py (right): https://codereview.chromium.org/61623008/diff/260001/gclient_scm.py#newcode227 gclient_scm.py:227: return None On 2013/11/15 18:25:25, iannucci wrote: > Maybe just `git config remote.origin.url` ? Also I think there's a utility > method for getting config entries. > > Sorry I didn't mention this earlier :( I didn't see such a utility method, but I did see other uses of "git config remote.origin.url". Changed GetRemoteURL to use that and changed the other instances to call GetRemoteURL. https://codereview.chromium.org/61623008/diff/260001/gclient_scm.py#newcode1104 gclient_scm.py:1104: return None On 2013/11/15 18:25:25, iannucci wrote: > I think we should probably rely on the XML version of info. I think there's a > method which does that already (CaptureLocalInfo) Done. https://codereview.chromium.org/61623008/diff/320001/tests/gclient_scm_test.py File tests/gclient_scm_test.py (right): https://codereview.chromium.org/61623008/diff/320001/tests/gclient_scm_test.p... tests/gclient_scm_test.py:699: self.mox.StubOutWithMock(gclient_scm.scm.SVN, 'Capture', True) The test failed without adding these lines, presumably because another test stubs out gclient_scm.scm.SVN.Capture? https://codereview.chromium.org/61623008/diff/320001/tests/gclient_smoketest.py File tests/gclient_smoketest.py (right): https://codereview.chromium.org/61623008/diff/320001/tests/gclient_smoketest.... tests/gclient_smoketest.py:766: def testUnversionedRepository(self): I'm a little confused about what this is testing, but the behavior has certainly changed. Since third_party/foo is still in DEPS, it gets deleted and re-synced on the second call to "gclient sync". https://codereview.chromium.org/61623008/diff/320001/tests/gclient_smoketest.... tests/gclient_smoketest.py:1070: def testPreDepsHooks(self): This test is removed in https://codereview.chromium.org/68853002/, but since that hasn't been merged yet...
Friendly ping
lgtm. Isaac/cmp: Do you think this would be worth manually pushing to bots on a particular waterfall first? I'm worried that if something goes wrong that we'll end up hosing the SVN servers as all the bots try to sync from scratch after blowing away their checkouts.
On 2013/12/02 19:43:55, iannucci wrote: > lgtm. > > Isaac/cmp: Do you think this would be worth manually pushing to bots on a > particular waterfall first? I'm worried that if something goes wrong that we'll > end up hosing the SVN servers as all the bots try to sync from scratch after > blowing away their checkouts. Not sure how you would roll it out to all bots on a particular waterfall first, but chromium.fyi is probably the easiest place if that's an option. I assume it would need to be hand-placed on bots directly. In that case, I'd pick one of each Mac/Win/Linux on chromium.fyi or on the main Chromium waterfall and deploy it there, then see how that goes.
https://chromiumcodereview.appspot.com/61623008/diff/20001/gclient.py File gclient.py (right): https://chromiumcodereview.appspot.com/61623008/diff/20001/gclient.py#newcode655 gclient.py:655: gclient_utils.rmtree(dest_dir) On 2013/11/12 19:33:37, borenet wrote: > Alternatively, we could move into a temporary directory to avoid clobbering > things. I think ilevy@ mentioned using such a temporary directory and swapping > out the contents of the directory as appropriate. For example, when syncing > Skia forward across the DEPS change, third_party/skia gets moved into > ".deleteme". If, at some later time, we sync backward across the DEPS change, > we swap the current contents of third_party/skia with those that we previously > moved into ".deleteme" before syncing. This strikes me as tricky to pull off > correctly, and I'm not sure that it will save us all that much for a small > (<50M) repo. Other than bots, I don't think it's OK for gclient to delete folders unfortunately. You can use tempfile library to create a unique temp directory with prefix like "skia_deleteme_" and move things there.. https://chromiumcodereview.appspot.com/61623008/diff/320001/gclient.py File gclient.py (right): https://chromiumcodereview.appspot.com/61623008/diff/320001/gclient.py#newcod... gclient.py:652: if os.path.isdir(dest_dir) and actual_remote_url != url: gclient has logic to reset remote urls. If we use this for the basis on deletion, maybe we should remove the git remote url rewriting logic. What about, alternatively, checking if the scm structure is unexpected (i.e. root dir doesn't match what gclient wants to check out) instead of looking at remote url? Also, will this logic work for syncing backwards across the split?
https://chromiumcodereview.appspot.com/61623008/diff/20001/gclient.py File gclient.py (right): https://chromiumcodereview.appspot.com/61623008/diff/20001/gclient.py#newcode655 gclient.py:655: gclient_utils.rmtree(dest_dir) Ignore this comment, it's stale.
If we still have any high-level concerns or arguments against this approach, let's please meet to work those out. https://chromiumcodereview.appspot.com/61623008/diff/20001/gclient.py File gclient.py (right): https://chromiumcodereview.appspot.com/61623008/diff/20001/gclient.py#newcode655 gclient.py:655: gclient_utils.rmtree(dest_dir) On 2013/12/03 18:25:49, Isaac wrote: > Ignore this comment, it's stale. Confirming that you're okay with the deletion approach, guarded by the "prompt if not --force"? https://chromiumcodereview.appspot.com/61623008/diff/320001/gclient.py File gclient.py (right): https://chromiumcodereview.appspot.com/61623008/diff/320001/gclient.py#newcod... gclient.py:652: if os.path.isdir(dest_dir) and actual_remote_url != url: On 2013/12/03 18:25:05, Isaac wrote: > gclient has logic to reset remote urls. If we use this for the basis on > deletion, maybe we should remove the git remote url rewriting logic. What > about, alternatively, checking if the scm structure is unexpected (i.e. root dir > doesn't match what gclient wants to check out) instead of looking at remote url? > Apologies for my lack of familiarity with gclient, but how would using root dirs as a basis for comparison work? > Also, will this logic work for syncing backwards across the split? The current logic works on my git-based checkout going in both directions, with the caveat that, in Skia's case, going in the reverse direction results in third_party/skia retaining its .git directory associated with the root-level Skia repo, rather than going back to being associated with the Chromium repo. As a result, third_party/skia contains the files which Skia has in the root level of its repo but aren't present in Chromium's src/third_party/skia. I don't know of a sensible way around that, since at sync time we don't know how the DEPS have changed since the last sync. The sync completes without failure though, and the resulting checkouts in third_party/skia/src, include, and gyp have the right contents.
> Apologies for my lack of familiarity with gclient, but how would using root dirs > as a basis for comparison work? It would use git rev-parse --show-toplevel, but there's not a corresponding command for svn so your approach seems fine. Consider my comments non-blocking. https://codereview.chromium.org/61623008/diff/20001/gclient.py File gclient.py (right): https://codereview.chromium.org/61623008/diff/20001/gclient.py#newcode655 gclient.py:655: gclient_utils.rmtree(dest_dir) On 2013/12/03 21:12:02, borenet wrote: > On 2013/12/03 18:25:49, Isaac wrote: > > Ignore this comment, it's stale. > > Confirming that you're okay with the deletion approach, guarded by the "prompt > if not --force"? Yes. With a prompt this is fine. https://codereview.chromium.org/61623008/diff/320001/gclient_scm.py File gclient_scm.py (right): https://codereview.chromium.org/61623008/diff/320001/gclient_scm.py#newcode392 gclient_scm.py:392: if (current_url != url and I'm confused how your logic interacts with this section
https://codereview.chromium.org/61623008/diff/320001/gclient_scm.py File gclient_scm.py (right): https://codereview.chromium.org/61623008/diff/320001/gclient_scm.py#newcode392 gclient_scm.py:392: if (current_url != url and On 2013/12/04 03:29:19, Isaac wrote: > I'm confused how your logic interacts with this section Ah yes, I can see how this is dangerous. I guess as-is, my change would cause checkouts with modified remote urls to be deleted and resynced from the default remote. Can the same thing be accomplished with "git config url.xyz.insteadOf abc" ? Doing it that way does not alter remote.origin.url. I'm open to alternatives for how to determine whether a checkout should be deleted and re-synced, since the current approach seems problematic.
Uploaded patch sets 6 and 7 to delete the URL rewriting logic. Isaac tells me that the same could be accomplished in the future using pre-DEPS hooks. https://codereview.chromium.org/61623008/diff/320001/gclient_scm.py File gclient_scm.py (right): https://codereview.chromium.org/61623008/diff/320001/gclient_scm.py#newcode392 gclient_scm.py:392: if (current_url != url and On 2013/12/04 16:58:42, borenet wrote: > On 2013/12/04 03:29:19, Isaac wrote: > > I'm confused how your logic interacts with this section > > Ah yes, I can see how this is dangerous. I guess as-is, my change would cause > checkouts with modified remote urls to be deleted and resynced from the default > remote. Can the same thing be accomplished with "git config url.xyz.insteadOf > abc" ? Doing it that way does not alter remote.origin.url. > > I'm open to alternatives for how to determine whether a checkout should be > deleted and re-synced, since the current approach seems problematic. Per discussion on chat, removed this section.
https://chromiumcodereview.appspot.com/61623008/diff/360001/gclient.py File gclient.py (right): https://chromiumcodereview.appspot.com/61623008/diff/360001/gclient.py#newcod... gclient.py:650: url, _ = gclient_utils.SplitUrlRevision(parsed_url) make sure this has substitions applied and is normalized for '/' https://chromiumcodereview.appspot.com/61623008/diff/360001/gclient.py#newcod... gclient.py:653: should_delete = options.force Let's also use if env contains 'CHROME_HEADLESS'
A couple of notes: For svn, syncing backwards across the Skia DEPS change only works with --force. Otherwise, third_party/skia has contents which confuse svn, resulting in a "no ancestry information" error. The deletion prompt sometimes gets partially hidden when running with --jobs > 1. Is there any reasonable way around that? If you can't see the prompt (eg, there's lots of spew from other DEPS syncing at the same time), it looks like your sync is hung. Now that I've added a list of hostnames which should run the new code, my test no longer passes (unless it runs on one of those machines). Should I tweak the test to make it pass? https://codereview.chromium.org/61623008/diff/360001/gclient.py File gclient.py (right): https://codereview.chromium.org/61623008/diff/360001/gclient.py#newcode650 gclient.py:650: url, _ = gclient_utils.SplitUrlRevision(parsed_url) On 2013/12/09 21:45:59, Isaac wrote: > make sure this has substitions applied and is normalized for '/' I tried overriding googlecode_url in the .gclient file for an existing SVN checkout, and the corresponding DEPS were deleted. So, parsed_url (and by extension url) has substitutions applied. As for trailing '/', I used custom_deps to override the acid3 dep. The trailing slash did matter, so I added a line to strip it. https://codereview.chromium.org/61623008/diff/360001/gclient.py#newcode653 gclient.py:653: should_delete = options.force On 2013/12/09 21:45:59, Isaac wrote: > Let's also use if env contains 'CHROME_HEADLESS' Done. https://codereview.chromium.org/61623008/diff/400001/gclient.py File gclient.py (right): https://codereview.chromium.org/61623008/diff/400001/gclient.py#newcode653 gclient.py:653: 'enabled.') Per Isaac's suggestion, using the hostname to enable on only a few bots at first. https://codereview.chromium.org/61623008/diff/400001/gclient_scm.py File gclient_scm.py (right): https://codereview.chromium.org/61623008/diff/400001/gclient_scm.py#newcode1195 gclient_scm.py:1195: I missed this section before - Isaac, this URL rewriting needs to go, right?
> For svn, syncing backwards across the Skia DEPS change only works with --force. > Otherwise, third_party/skia has contents which confuse svn, resulting in a "no > ancestry information" error. This is probably fine. Most people use git -- and most bots use --force. This LGTM https://codereview.chromium.org/61623008/diff/400001/gclient.py File gclient.py (right): https://codereview.chromium.org/61623008/diff/400001/gclient.py#newcode653 gclient.py:653: 'enabled.') On 2013/12/10 18:48:27, borenet wrote: > Per Isaac's suggestion, using the hostname to enable on only a few bots at > first. This looks good to me. To be extra safe, maybe also guard with os.environ.get('CHROME_HEADLESS') https://codereview.chromium.org/61623008/diff/400001/gclient_scm.py File gclient_scm.py (right): https://codereview.chromium.org/61623008/diff/400001/gclient_scm.py#newcode1195 gclient_scm.py:1195: On 2013/12/10 18:48:27, borenet wrote: > I missed this section before - Isaac, this URL rewriting needs to go, right? I'm not very familiar with this, having essentially only used gclient for git. If it doesn't conflict with your patch we could keep it. I suggested removing the git rewrite section because it had known flaws in addition to being confusing. Maybe Robbie knows more? If we don't hear from him I'd suggest keeping it.
Uploaded patch sets 12-14. Robbie, can you comment on the svn url rewriting? https://codereview.chromium.org/61623008/diff/400001/gclient.py File gclient.py (right): https://codereview.chromium.org/61623008/diff/400001/gclient.py#newcode653 gclient.py:653: 'enabled.') On 2013/12/11 03:17:33, Isaac wrote: > On 2013/12/10 18:48:27, borenet wrote: > > Per Isaac's suggestion, using the hostname to enable on only a few bots at > > first. > > This looks good to me. To be extra safe, maybe also guard with > os.environ.get('CHROME_HEADLESS') Done. https://codereview.chromium.org/61623008/diff/400001/gclient_scm.py File gclient_scm.py (right): https://codereview.chromium.org/61623008/diff/400001/gclient_scm.py#newcode1195 gclient_scm.py:1195: On 2013/12/11 03:17:33, Isaac wrote: > On 2013/12/10 18:48:27, borenet wrote: > > I missed this section before - Isaac, this URL rewriting needs to go, right? > > I'm not very familiar with this, having essentially only used gclient for git. > If it doesn't conflict with your patch we could keep it. I suggested removing > the git rewrite section because it had known flaws in addition to being > confusing. > > Maybe Robbie knows more? If we don't hear from him I'd suggest keeping it. It does conflict. When I overrode the googlecode_url in an existing checkout using custom_vars, the checkouts were migrated using this code rather than deleted.
Friendly ping. Robbie, any comments on the svn URL rewriting?
ping
omg, sorry I thought I had published this weeks ago :( lgtm https://chromiumcodereview.appspot.com/61623008/diff/400001/gclient_scm.py File gclient_scm.py (right): https://chromiumcodereview.appspot.com/61623008/diff/400001/gclient_scm.py#ne... gclient_scm.py:1195: On 2013/12/11 20:42:19, borenet wrote: > On 2013/12/11 03:17:33, Isaac wrote: > > On 2013/12/10 18:48:27, borenet wrote: > > > I missed this section before - Isaac, this URL rewriting needs to go, right? > > > > I'm not very familiar with this, having essentially only used gclient for git. > > > If it doesn't conflict with your patch we could keep it. I suggested removing > > the git rewrite section because it had known flaws in addition to being > > confusing. > > > > Maybe Robbie knows more? If we don't hear from him I'd suggest keeping it. > > It does conflict. When I overrode the googlecode_url in an existing checkout > using custom_vars, the checkouts were migrated using this code rather than > deleted. So IIUC, the new behavior will simply nuke the conflicting directory instead of trying to rewrite/preserve it (only if --force, or if the user picks 'yes'). This sounds fine to me... in the git-future, we should be relying on a global object cache anyway, so this url relocation will be unnecessary.
Thanks Robbie. Uploaded patch set 15. Marc-Antoine, this change has a couple of conflicts with your changes in https://codereview.chromium.org/85473007/. Would you mind verifying that this is okay? https://codereview.chromium.org/61623008/diff/400001/gclient_scm.py File gclient_scm.py (right): https://codereview.chromium.org/61623008/diff/400001/gclient_scm.py#newcode1195 gclient_scm.py:1195: On 2014/01/07 22:23:28, iannucci wrote: > On 2013/12/11 20:42:19, borenet wrote: > > On 2013/12/11 03:17:33, Isaac wrote: > > > On 2013/12/10 18:48:27, borenet wrote: > > > > I missed this section before - Isaac, this URL rewriting needs to go, > right? > > > > > > I'm not very familiar with this, having essentially only used gclient for > git. > > > > > If it doesn't conflict with your patch we could keep it. I suggested > removing > > > the git rewrite section because it had known flaws in addition to being > > > confusing. > > > > > > Maybe Robbie knows more? If we don't hear from him I'd suggest keeping it. > > > > It does conflict. When I overrode the googlecode_url in an existing checkout > > using custom_vars, the checkouts were migrated using this code rather than > > deleted. > > So IIUC, the new behavior will simply nuke the conflicting directory instead of > trying to rewrite/preserve it (only if --force, or if the user picks 'yes'). > Exactly. > This sounds fine to me... in the git-future, we should be relying on a global > object cache anyway, so this url relocation will be unnecessary.
https://codereview.chromium.org/61623008/diff/500001/gclient.py File gclient.py (right): https://codereview.chromium.org/61623008/diff/500001/gclient.py#newcode678 gclient.py:678: socket.gethostname() in ('vm859-m1', 'build1-m1', 'vm630-m1') and Extract this in a function. I highly dislike this, you should rely on flags or an environment variable. Add an ETA to remove this hack if you want to keep it. https://codereview.chromium.org/61623008/diff/500001/gclient.py#newcode688 gclient.py:688: if sys.__stdout__.isatty() and not should_delete: Move the check/prompt into a function. This shouldn't prompt when --force is used. As a matter of fact, I don't think the prompt is necessary. There is --reset for this logic. https://codereview.chromium.org/61623008/diff/500001/gclient_scm.py File gclient_scm.py (right): https://codereview.chromium.org/61623008/diff/500001/gclient_scm.py#newcode1099 gclient_scm.py:1099: # Only update if git or hg is not controlling the directory. Do you want to keep these? From what I understand, it is becoming a no-op since the check is done *before* this function is being called. https://codereview.chromium.org/61623008/diff/500001/gclient_scm.py#newcode1105 gclient_scm.py:1105: hg_path = os.path.join(self.checkout_path, '.hg') https://codereview.chromium.org/3601007 is opaque to me. I don't know if is it still desired. https://codereview.chromium.org/61623008/diff/500001/gclient_scm.py#newcode1137 gclient_scm.py:1137: from_info = scm.SVN.CaptureLocalInfo( From what I understand, this code is not necessary anymore?
Uploaded patch set 17. https://codereview.chromium.org/61623008/diff/500001/gclient.py File gclient.py (right): https://codereview.chromium.org/61623008/diff/500001/gclient.py#newcode678 gclient.py:678: socket.gethostname() in ('vm859-m1', 'build1-m1', 'vm630-m1') and On 2014/01/10 16:41:47, M-A Ruel wrote: > Extract this in a function. I highly dislike this, you should rely on flags or > an environment variable. Add an ETA to remove this hack if you want to keep it. Done. When this is submitted, I will submit a try on one or all of these bots which plays with the DEPS file. If nothing terrible happens, I'll remove the guard. https://codereview.chromium.org/61623008/diff/500001/gclient.py#newcode688 gclient.py:688: if sys.__stdout__.isatty() and not should_delete: On 2014/01/10 16:41:47, M-A Ruel wrote: > Move the check/prompt into a function. > This shouldn't prompt when --force is used. > As a matter of fact, I don't think the prompt is necessary. There is --reset for > this logic. Done. The prompt is not displayed with --force. I can remove the prompt and just fail instead? That would be simpler and should only make a difference for humans. I don't know how --reset interacts with this. https://codereview.chromium.org/61623008/diff/500001/gclient_scm.py File gclient_scm.py (right): https://codereview.chromium.org/61623008/diff/500001/gclient_scm.py#newcode1099 gclient_scm.py:1099: # Only update if git or hg is not controlling the directory. On 2014/01/10 16:41:47, M-A Ruel wrote: > Do you want to keep these? From what I understand, it is becoming a no-op since > the check is done *before* this function is being called. It seems that these checks shouldn't be needed, but I'm a little scared to touch much more of this. https://codereview.chromium.org/61623008/diff/500001/gclient_scm.py#newcode1105 gclient_scm.py:1105: hg_path = os.path.join(self.checkout_path, '.hg') On 2014/01/10 16:41:47, M-A Ruel wrote: > https://codereview.chromium.org/3601007 is opaque to me. I don't know if is it > still desired. I defer to someone else's judgment. https://codereview.chromium.org/61623008/diff/500001/gclient_scm.py#newcode1137 gclient_scm.py:1137: from_info = scm.SVN.CaptureLocalInfo( On 2014/01/10 16:41:47, M-A Ruel wrote: > From what I understand, this code is not necessary anymore? I guess I'm not familiar enough with this code to know, but it looks like this is used quite a bit below...
+ Morrita for why mercurial checkout should be ignored. I don't know why. https://codereview.chromium.org/61623008/diff/500001/gclient.py File gclient.py (right): https://codereview.chromium.org/61623008/diff/500001/gclient.py#newcode688 gclient.py:688: if sys.__stdout__.isatty() and not should_delete: On 2014/01/10 19:27:23, borenet wrote: > Done. The prompt is not displayed with --force. I can remove the prompt and > just fail instead? That would be simpler and should only make a difference for > humans. I don't know how --reset interacts with this. Yes, I prefer failing to prompting. Prompting the user in the middle of a sync was a Very Bad Idea (tm). https://codereview.chromium.org/61623008/diff/500001/gclient_scm.py File gclient_scm.py (right): https://codereview.chromium.org/61623008/diff/500001/gclient_scm.py#newcode1099 gclient_scm.py:1099: # Only update if git or hg is not controlling the directory. On 2014/01/10 19:27:23, borenet wrote: > On 2014/01/10 16:41:47, M-A Ruel wrote: > > Do you want to keep these? From what I understand, it is becoming a no-op > since > > the check is done *before* this function is being called. > > It seems that these checks shouldn't be needed, but I'm a little scared to touch > much more of this. I'd recommend to explicitly test it. Create a dummy svn .gclient, checkout a git repository instead and gclient sync to see what happens. If this code is not triggered, remove it. Technically, this should be a proper test but I'll defer if this is a requirement to Robbie. https://codereview.chromium.org/61623008/diff/500001/gclient_scm.py#newcode1137 gclient_scm.py:1137: from_info = scm.SVN.CaptureLocalInfo( On 2014/01/10 19:27:23, borenet wrote: > On 2014/01/10 16:41:47, M-A Ruel wrote: > > From what I understand, this code is not necessary anymore? > > I guess I'm not familiar enough with this code to know, but it looks like this > is used quite a bit below... Why not make an assert at the top to ensure the checkout is in a predetermined state before even calling this function? https://codereview.chromium.org/61623008/diff/500001/gclient_scm.py#newcode1222 gclient_scm.py:1222: raise gclient_utils.Error( I also assume this cannot happen anymore. https://codereview.chromium.org/61623008/diff/630001/gclient.py File gclient.py (right): https://codereview.chromium.org/61623008/diff/630001/gclient.py#newcode673 gclient.py:673: """This function determines whether the new behavior of deleting """Determines .. https://codereview.chromium.org/61623008/diff/630001/gclient.py#newcode674 gclient.py:674: checkouts which conflict with those in DEPS should be enabled. It Empty line between one liner summary and detailed explanation. https://codereview.chromium.org/61623008/diff/630001/gclient.py#newcode678 gclient.py:678: import socket I'd prefer the import at the top of the file. Even if it is a "temporary hack". https://codereview.chromium.org/61623008/diff/630001/gclient.py#newcode680 gclient.py:680: and os.environ.get('CHROME_HEADLESS')) You should inverse the check. Checking for os.environ is much faster than calling socket.gethostname(). https://codereview.chromium.org/61623008/diff/630001/gclient.py#newcode683 gclient.py:683: """Determine whether a checkout which conflicts with the DEPS entry imperactive https://codereview.chromium.org/61623008/diff/630001/gclient.py#newcode684 gclient.py:684: should be deleted. Returns True if running with --force or when empty line https://codereview.chromium.org/61623008/diff/630001/gclient.py#newcode686 gclient.py:686: should_delete = options.force or os.environ.get('CHROME_HEADLESS') Do not use an environment variable to determine this, only use flags. Otherwise it's totally black magic. https://codereview.chromium.org/61623008/diff/630001/gclient.py#newcode700 gclient.py:700: if (command == 'update' and enable_deletion_of_conflicting_checkouts()): () are not needed
Uploaded patch set 18. There are some failing tests which I will address if the changes in this patch set are acceptable. https://codereview.chromium.org/61623008/diff/500001/gclient.py File gclient.py (right): https://codereview.chromium.org/61623008/diff/500001/gclient.py#newcode688 gclient.py:688: if sys.__stdout__.isatty() and not should_delete: On 2014/01/13 17:31:38, M-A Ruel wrote: > On 2014/01/10 19:27:23, borenet wrote: > > Done. The prompt is not displayed with --force. I can remove the prompt and > > just fail instead? That would be simpler and should only make a difference > for > > humans. I don't know how --reset interacts with this. > > Yes, I prefer failing to prompting. > > Prompting the user in the middle of a sync was a Very Bad Idea (tm). Done. https://codereview.chromium.org/61623008/diff/500001/gclient_scm.py File gclient_scm.py (right): https://codereview.chromium.org/61623008/diff/500001/gclient_scm.py#newcode1099 gclient_scm.py:1099: # Only update if git or hg is not controlling the directory. On 2014/01/13 17:31:38, M-A Ruel wrote: > On 2014/01/10 19:27:23, borenet wrote: > > On 2014/01/10 16:41:47, M-A Ruel wrote: > > > Do you want to keep these? From what I understand, it is becoming a no-op > > since > > > the check is done *before* this function is being called. > > > > It seems that these checks shouldn't be needed, but I'm a little scared to > touch > > much more of this. > > I'd recommend to explicitly test it. > > Create a dummy svn .gclient, checkout a git repository instead and gclient sync > to see what happens. If this code is not triggered, remove it. > > Technically, this should be a proper test but I'll defer if this is a > requirement to Robbie. I tested this out locally, and as expected the checkout was either removed (using --force) or an error was thrown before we get to this point. Removed these sections in the latest patch set. https://codereview.chromium.org/61623008/diff/630001/gclient.py File gclient.py (right): https://codereview.chromium.org/61623008/diff/630001/gclient.py#newcode673 gclient.py:673: """This function determines whether the new behavior of deleting On 2014/01/13 17:31:39, M-A Ruel wrote: > """Determines .. Done. https://codereview.chromium.org/61623008/diff/630001/gclient.py#newcode674 gclient.py:674: checkouts which conflict with those in DEPS should be enabled. It On 2014/01/13 17:31:39, M-A Ruel wrote: > Empty line between one liner summary and detailed explanation. Done. https://codereview.chromium.org/61623008/diff/630001/gclient.py#newcode678 gclient.py:678: import socket On 2014/01/13 17:31:39, M-A Ruel wrote: > I'd prefer the import at the top of the file. Even if it is a "temporary hack". Done. https://codereview.chromium.org/61623008/diff/630001/gclient.py#newcode680 gclient.py:680: and os.environ.get('CHROME_HEADLESS')) On 2014/01/13 17:31:39, M-A Ruel wrote: > You should inverse the check. Checking for os.environ is much faster than > calling socket.gethostname(). Done. https://codereview.chromium.org/61623008/diff/630001/gclient.py#newcode700 gclient.py:700: if (command == 'update' and enable_deletion_of_conflicting_checkouts()): On 2014/01/13 17:31:39, M-A Ruel wrote: > () are not needed Done.
lgtm but please try to delete enable_deletion_of_conflicting_checkouts() ASAP, likely in less than 72 hours.
On 2014/01/14 20:09:49, M-A Ruel wrote: > lgtm but please try to delete enable_deletion_of_conflicting_checkouts() ASAP, > likely in less than 72 hours. Thanks. As I mentioned in chat, I have one remaining concern: this CL deletes a bunch of code which is no longer needed with the new behavior. However, the new behavior will only show up on the whitelisted bots. Therefore, the remainder of the bots (and humans) may be affected by the removal of this code in some unexpected way. I'm wondering whether it's a better idea to remove the guard altogether and commit very carefully over a weekend or something. Thoughts?
Uploaded patch set 19 to fix the failing tests. They failed because: - We're no longer performing the check for .git and .hg directories - The smoketest doesn't work unless the guard is removed. PTAL https://codereview.chromium.org/61623008/diff/940001/gclient_scm.py File gclient_scm.py (left): https://codereview.chromium.org/61623008/diff/940001/gclient_scm.py#oldcode1405 gclient_scm.py:1405: if os.path.isdir(os.path.join(self.checkout_path, '.git')): Seems like this needs to go, too. https://codereview.chromium.org/61623008/diff/940001/tests/gclient_scm_test.py File tests/gclient_scm_test.py (left): https://codereview.chromium.org/61623008/diff/940001/tests/gclient_scm_test.p... tests/gclient_scm_test.py:648: def testUpdateGit(self): I just removed these two tests, since making them pass would defeat their purpose. I think the new smoketest I added should cover this though. https://codereview.chromium.org/61623008/diff/940001/tests/gclient_smoketest.py File tests/gclient_smoketest.py (right): https://codereview.chromium.org/61623008/diff/940001/tests/gclient_smoketest.... tests/gclient_smoketest.py:1350: socket.gethostname() in ('vm859-m1', 'build1-m1', 'vm630-m1')): This is very, very stupid, but since there's a guard in gclient, we can't run this test and expect it to have reasonable results until the guard is gone.
https://codereview.chromium.org/61623008/diff/940001/tests/gclient_smoketest.py File tests/gclient_smoketest.py (right): https://codereview.chromium.org/61623008/diff/940001/tests/gclient_smoketest.... tests/gclient_smoketest.py:1350: socket.gethostname() in ('vm859-m1', 'build1-m1', 'vm630-m1')): On 2014/01/14 21:49:45, borenet wrote: > This is very, very stupid, but since there's a guard in gclient, we can't run > this test and expect it to have reasonable results until the guard is gone. What about mocking socket.gethostname to return 'vm859-m1' for the duration of this test?
lgtm https://codereview.chromium.org/61623008/diff/940001/gclient.py File gclient.py (right): https://codereview.chromium.org/61623008/diff/940001/gclient.py#newcode697 gclient.py:697: ' %s' % (dest_dir, url, dest_dir)) BTW in general we use: logging.warning('%s does not contain a checkout of %s. Removing %s', dest_dir, url, dest_dir) e.g no need to % format. https://codereview.chromium.org/61623008/diff/940001/tests/gclient_smoketest.py File tests/gclient_smoketest.py (right): https://codereview.chromium.org/61623008/diff/940001/tests/gclient_smoketest.... tests/gclient_smoketest.py:1350: socket.gethostname() in ('vm859-m1', 'build1-m1', 'vm630-m1')): On 2014/01/14 22:40:02, iannucci wrote: > On 2014/01/14 21:49:45, borenet wrote: > > This is very, very stupid, but since there's a guard in gclient, we can't run > > this test and expect it to have reasonable results until the guard is gone. > > What about mocking socket.gethostname to return 'vm859-m1' for the duration of > this test? Mock enable_deletion_of_conflicting_checkouts instead. https://codereview.chromium.org/61623008/diff/940001/tests/gclient_smoketest.... tests/gclient_smoketest.py:1393: 'src/repo2: %srepo_2@%s' % (self.git_base, self.githash('repo_2', 1)[:7]), 80 cols
https://codereview.chromium.org/61623008/diff/940001/tests/gclient_smoketest.py File tests/gclient_smoketest.py (right): https://codereview.chromium.org/61623008/diff/940001/tests/gclient_smoketest.... tests/gclient_smoketest.py:1350: socket.gethostname() in ('vm859-m1', 'build1-m1', 'vm630-m1')): On 2014/01/14 22:46:09, M-A Ruel wrote: > On 2014/01/14 22:40:02, iannucci wrote: > > On 2014/01/14 21:49:45, borenet wrote: > > > This is very, very stupid, but since there's a guard in gclient, we can't > run > > > this test and expect it to have reasonable results until the guard is gone. > > > > What about mocking socket.gethostname to return 'vm859-m1' for the duration of > > this test? > > Mock enable_deletion_of_conflicting_checkouts instead. These are much better solutions. Unfortunately, since we shell out to gclient rather than importing it directly, I can't mock anything directly. The best I could do is play with environment variables. I could modify enable_deletion_of_conflicting_checkouts to be something like this: return os.environ.get('FORCE_ENABLE_GCLIENT_AUTODELETE') or (os.environ.get('CHROME_HEADLESS') and socket......) And then stuff FORCE_ENABLE_GCLIENT_AUTODELETE into the environment for the duration of the test, but that seems even more hacky. I almost prefer the current solution for its simplicity.
Yeah, ok, LGTM and then when it's confirmed to work, we'll remove the hacks
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/borenet@google.com/61623008/940001
Presubmit check for 61623008-940001 failed and returned exit status 1. Running presubmit commit checks ... Checking out rietveld... Running save-description-on-failure.sh Running push-basic.sh Running upstream.sh Running submit-from-new-dir.sh Running abandon.sh Running submodule-merge-test.sh Running upload-local-tracking-branch.sh Running hooks.sh Running post-dcommit-hook-test.sh Running upload-stale.sh Running patch.sh Running basic.sh ** Presubmit ERRORS ** Pylint (81 files) (47.93s) failed ************* Module gclient_smoketest C0301:1393,0: Line too long (82/80) Presubmit checks took 110.7s to calculate.
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/borenet@google.com/61623008/1060001
Message was sent while issue was closed.
Change committed as 245404
Message was sent while issue was closed.
A revert of this CL has been created in https://codereview.chromium.org/141633005/ by thakis@chromium.org. The reason for reverting is: This breaks `gclient sync` for me. Before this patch: ________ found .git directory; skipping src After this patch: Error: 1> Can't update/checkout /Volumes/MacintoshHD2/src/chrome-git/src if an unversioned directory is present. Delete the directory and try again. .
Message was sent while issue was closed.
On 2014/01/17 16:55:11, Nico wrote: > A revert of this CL has been created in > https://codereview.chromium.org/141633005/ by mailto:thakis@chromium.org. > > The reason for reverting is: This breaks `gclient sync` for me. Before this > patch: > ________ found .git directory; skipping src > > After this patch: > Error: 1> Can't update/checkout /Volumes/MacintoshHD2/src/chrome-git/src if an > unversioned directory is present. Delete the directory and try again. > . Hm, interesting... src isn't in your .gclient? Hunh? How did you set up your checkout?
Message was sent while issue was closed.
On 2014/01/17 19:30:45, iannucci wrote: > On 2014/01/17 16:55:11, Nico wrote: > > A revert of this CL has been created in > > https://codereview.chromium.org/141633005/ by mailto:thakis@chromium.org. > > > > The reason for reverting is: This breaks `gclient sync` for me. Before this > > patch: > > ________ found .git directory; skipping src > > > > After this patch: > > Error: 1> Can't update/checkout /Volumes/MacintoshHD2/src/chrome-git/src if an > > unversioned directory is present. Delete the directory and try again. > > . > > Hm, interesting... src isn't in your .gclient? Hunh? > > How did you set up your checkout? This looks to be a problem with all git-svn checkouts. GetRemoteURL returns None in these cases, and we're no longer returning early when we find an unexpected .git directory, so gclient thinks the directory is unversioned. I'm working on a fix now.
Message was sent while issue was closed.
On 2014/01/17 19:33:11, borenet wrote: > On 2014/01/17 19:30:45, iannucci wrote: > > On 2014/01/17 16:55:11, Nico wrote: > > > A revert of this CL has been created in > > > https://codereview.chromium.org/141633005/ by mailto:thakis@chromium.org. > > > > > > The reason for reverting is: This breaks `gclient sync` for me. Before this > > > patch: > > > ________ found .git directory; skipping src > > > > > > After this patch: > > > Error: 1> Can't update/checkout /Volumes/MacintoshHD2/src/chrome-git/src if > an > > > unversioned directory is present. Delete the directory and try again. > > > . > > > > Hm, interesting... src isn't in your .gclient? Hunh? > > > > How did you set up your checkout? > > This looks to be a problem with all git-svn checkouts. GetRemoteURL returns > None in these cases, and we're no longer returning early when we find an > unexpected .git directory, so gclient thinks the directory is unversioned. I'm > working on a fix now. Uploaded CL to re-land + fix: https://codereview.chromium.org/131743022/ |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
