Chromium Code Reviews
|
|
Chromium Code Reviews|
Created:
8 years, 10 months ago by Steve Block Modified:
8 years, 10 months ago Reviewers:
Peter Mayo, Peter Mayo (wrong one), M-A Ruel CC:
chromium-reviews, Dirk Pranke, M-A Ruel, Peter Mayo, davidjames, sosa, Chris Masone, Ian Vollick Visibility:
Public. |
DescriptionIf both -f and -D are specified when updating, remove all untracked directories
This is required to avoid the need to clobber the bots when moving a directory
to deps/. Currently, the directory in question is likely to remain in the
working copy, despite having been removed, due to the presence of untracked
files. This causes the checkout from deps/ to fail.
With this change, when both --force and --delete_unversioned_trees are
specified, the the directory in question will be removed from the working copy,
thereby allowing the copy in deps/ to be checked out correctly.
Note that untracked directories which are explicitly ignored (ie in .gitignore
or svn:ignore) will not be removed.
BUG=112887, chromium-os:20759
Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=121986
Patch Set 1 #Patch Set 2 : '' #
Total comments: 5
Patch Set 3 : '' #
Total comments: 1
Patch Set 4 : '' #Patch Set 5 : '' #
Total comments: 1
Patch Set 6 : '' #Patch Set 7 : If both -f and -D are specified when updating, remove all untracked directories #
Created: 8 years, 10 months ago
Messages
Total messages: 23 (0 generated)
http://chromiumcodereview.appspot.com/9348054/diff/1004/gclient_scm.py File gclient_scm.py (right): http://chromiumcodereview.appspot.com/9348054/diff/1004/gclient_scm.py#newcod... gclient_scm.py:451: ['ls-files', '--directory', '--others', '--exclude-standard'], Note that this will be overzealous if .gitignore is not updated properly. See the comment at line 958 for an explanation about what's preferable. http://crbug.com/109763 is about filtering dependencies that are know when doing a "status", which would be helpful here and below. http://chromiumcodereview.appspot.com/9348054/diff/1004/gclient_scm.py#newcod... gclient_scm.py:933: if status[0][0] != '?': Eh, that was bad. http://chromiumcodereview.appspot.com/9348054/diff/1004/gclient_scm.py#newcod... gclient_scm.py:958: # If --force is specified, remove any untracked files and directories. You are basically doing a revert only for directories. What I wanted if to scan file_list that was generated at line 956, look for 'D' operations and then process these. Otherwise, it's going to be constantly deleting directories that aren't svn:ignored properly.
http://chromiumcodereview.appspot.com/9348054/diff/1004/gclient_scm.py File gclient_scm.py (right): http://chromiumcodereview.appspot.com/9348054/diff/1004/gclient_scm.py#newcod... gclient_scm.py:451: ['ls-files', '--directory', '--others', '--exclude-standard'], > http://crbug.com/109763 is about filtering dependencies that are know when doing > a "status", which would be helpful here and below. I think that in this case, we want to delete untracked directories which have just been removed from the repository, even if they are ignored or are a dependency. This will allow us to use a single change to move a directory to deps/ and set svn:ignore for that directory. http://chromiumcodereview.appspot.com/9348054/diff/1004/gclient_scm.py#newcod... gclient_scm.py:958: # If --force is specified, remove any untracked files and directories. > What I wanted if to scan file_list that was generated at line 956, > look for 'D' I considered this approach, but it seems less robust, as if you ever end up with untracked directories in your working copy (for example, as the result of 'gclient sync' without '--force'), then there's no way to remove them without manual intervention. So if a later invocation of 'gclient sync' tries to put a DEPS directory in that location, it will fail, even if you use '--force'. I guess though that it's sufficient for the bots, which can always use '--force'. I'll update the patch to try this approach. > Otherwise, it's going to be constantly deleting directories that > aren't svn:ignored properly. Indeed, but they'll immediately be replaced from DEPS. Isn't there an expectation that svn:ignore is set correctly?
Updated patch is a work in progress
+some cros people that have been impacted by the previous condition in gclient and might be interested in this
On 2012/02/09 17:55:04, Steve Block wrote: > > Otherwise, it's going to be constantly deleting directories that > > aren't svn:ignored properly. > Indeed, but they'll immediately be replaced from DEPS. Isn't there an > expectation that svn:ignore is set correctly? I wish! You convinced me now, I'm fine with the idea. Sorry for the delay in this review cycle, I had to think about it a little. Why have you removed both git support and the test? http://chromiumcodereview.appspot.com/9348054/diff/5001/gclient_scm.py File gclient_scm.py (right): http://chromiumcodereview.appspot.com/9348054/diff/5001/gclient_scm.py#newcod... gclient_scm.py:944: if options.force: I'd prefer to start with: if options.force and options.self._options.delete_unversioned_trees:
Could you add ,chromium-os:20759 to the BUG line. It looks like this will go a long way to adressing that too.
Updated patch to check --delete_unversioned_trees Note that I'm seeing failures in ManagedGitWrapperTestCase.testUpdateCheckout() and UnmanagedGitWrapperTestCase.testUpdateCheckout() when I test locally, both before and after my change. Is this a known issue?
On 2012/02/10 11:22:20, Steve Block wrote: > Updated patch to check --delete_unversioned_trees > > Note that I'm seeing failures in ManagedGitWrapperTestCase.testUpdateCheckout() > and UnmanagedGitWrapperTestCase.testUpdateCheckout() when I test locally, both > before and after my change. Is this a known issue? ManagedGitWrapper tests aren't stable and have race conditions. :/ As long as they pass on the CQ, it's fine with me. lgtm with optional nit. http://chromiumcodereview.appspot.com/9348054/diff/10002/gclient_scm.py File gclient_scm.py (right): http://chromiumcodereview.appspot.com/9348054/diff/10002/gclient_scm.py#newco... gclient_scm.py:454: if paths: nit: Not necessary to do "if paths:", just this will work right away. You can keep the "if" on its own line if you prefer than using a generator. I don't mind either way. for path in (p for p in paths.splitlines() if p.endswith('/')):
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/steveblock@chromium.org/9348054/13002
Presubmit check for 9348054-13002 failed and returned exit status 1. warning: code.google.com certificate with fingerprint ce:3e:bd:64:4d:e5:66:1c:e8:63:eb:99:3d:7e:75:36:a0:86:0e:03 not verified (check hostfingerprints or web.cacerts config setting) Running presubmit commit checks ... Checking out rietveld... Running push-basic.sh Running save-description-on-failure.sh Running basic.sh Running upload-stale.sh Running upload-local-tracking-branch.sh Running patch.sh Running submit-from-new-dir.sh Running post-dcommit-hook-test.sh Running hooks.sh Running abandon.sh Running upstream.sh ** Presubmit Messages ** You should install python 2.5 and run ln -s $(which python2.5) python. A great place to put this symlink is in depot_tools. Otherwise, you break depot_tools on python 2.5, you get to keep the pieces. ** Presubmit ERRORS ** tests/gclient_smoketest.py failed! Command tests/gclient_smoketest.py returned non-zero exit status 1 in /mnt/data/b/commit-queue/workdir/tools/depot_tools ............................F........... ====================================================================== FAIL: testRevertAndStatus (__main__.GClientSmokeSVN) ---------------------------------------------------------------------- Traceback (most recent call last): File "tests/gclient_smoketest.py", line 568, in testRevertAndStatus self.assertEquals(10, len(out)) AssertionError: 10 != 4 ---------------------------------------------------------------------- Ran 40 tests in 158.250s FAILED (failures=1) Presubmit checks took 351.3s to calculate.
lgtm
On 2012/02/11 10:31:50, I haz the power (commit-bot) wrote: > FAIL: testRevertAndStatus (__main__.GClientSmokeSVN) > ---------------------------------------------------------------------- > Traceback (most recent call last): > File "tests/gclient_smoketest.py", line 568, in testRevertAndStatus > self.assertEquals(10, len(out)) > AssertionError: 10 != 4 BTW, you can ./tests/gclient_smoketest.py -v GClientSmokeSVN.testRevertAndStatus to figure out the problem.
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/steveblock@chromium.org/9348054/19001
Change committed as 121986
It seems this may have negatively impacted the chrome in chromiumos builders at least. chromeos-chrome-19.0.1043.0_alpha-r1: 1>_____ removing unversioned directory c chromeos-chrome-19.0.1043.0_alpha-r1: Syncing projects: 40% ( 2/ 5) src chromeos-chrome-19.0.1043.0_alpha-r1: Error: Called rmtree(/var/cache/chromeos-chrome/chrome-src/src/c) in non-directory I'll look more closely in a few minutes to try to figure out what "c" was. If anyone else has an idea for what "in a non-directory" means, that could be valuable to add.
On 2012/02/15 16:13:11, Peter Mayo wrote:
> It seems this may have negatively impacted the chrome in chromiumos builders
at
> least.
>
>
> chromeos-chrome-19.0.1043.0_alpha-r1: 1>_____ removing unversioned directory c
> chromeos-chrome-19.0.1043.0_alpha-r1: Syncing projects: 40% ( 2/ 5) src
> chromeos-chrome-19.0.1043.0_alpha-r1: Error: Called
> rmtree(/var/cache/chromeos-chrome/chrome-src/src/c) in non-directory
>
>
> I'll look more closely in a few minutes to try to figure out what "c" was. If
> anyone else has an idea for what "in a non-directory" means, that could be
> valuable to add.
For what a non-directory means, from gclient_utils.py:
if os.path.islink(path) or not os.path.isdir(path):
raise Error('Called rmtree(%s) in non-directory' % path)
Investigating
I'll revert in the meantime. Le 15 février 2012 08:26, Steve Block <steveblock@chromium.org> a écrit : > Investigating >
Note that it's the change to gclient that needs reverting, not the change to the buildbot config. See http://build.chromium.org/p/chromium.chromiumos/builders/ChromiumOS%20%28x86%...
Sure but it's faster and safer to revert this now. Le 15 février 2012 08:36, Steve Block <steveblock@chromium.org> a écrit : > Note that it's the change to gclient that needs reverting, not the > change to the buildbot config. See > > http://build.chromium.org/p/chromium.chromiumos/builders/ChromiumOS%20%28x86%... >
Which "this" is that? gclient -> ebuild -> boom! No buildbot config will help without neutering the affected builder completely. On Wed, Feb 15, 2012 at 11:37 AM, Marc-Antoine Ruel <maruel@chromium.org>wrote: > Sure but it's faster and safer to revert this now. > > Le 15 février 2012 08:36, Steve Block <steveblock@chromium.org> a écrit : > > Note that it's the change to gclient that needs reverting, not the >> change to the buildbot config. See >> >> http://build.chromium.org/p/chromium.chromiumos/builders/ChromiumOS%20%28x86%... >> > > -- Peter Mayo | Waterloo | petermayo@google.com | 519-880-3439
I also reverted the gclient change. You were cc'ed on the CL. Le 15 février 2012 09:45, Peter Mayo <petermayo@google.com> a écrit : > Which "this" is that? > > gclient -> ebuild -> boom! > > No buildbot config will help without neutering the affected builder > completely. > > > On Wed, Feb 15, 2012 at 11:37 AM, Marc-Antoine Ruel <maruel@chromium.org>wrote: > >> Sure but it's faster and safer to revert this now. >> >> Le 15 février 2012 08:36, Steve Block <steveblock@chromium.org> a écrit : >> >> Note that it's the change to gclient that needs reverting, not the >>> change to the buildbot config. See >>> >>> http://build.chromium.org/p/chromium.chromiumos/builders/ChromiumOS%20%28x86%... >>> >> >> > > > -- > > Peter Mayo | Waterloo | petermayo@google.com | 519-880-3439 > > |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
