Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
189913020:
gclient: print a warning if a dep would get deleted or moved in the future (Closed)
|
Created:
6 years, 9 months ago by borenet Modified:
6 years, 8 months ago CC:
chromium-reviews, Dirk Pranke, cmp-cc_chromium.org, M-A Ruel, iannucci+depot_tools_chromium.org, ilevy-cc_chromium.org, skiabot_google.com Visibility:
Public. |
Descriptiongclient: print a warning if a dep would get deleted or moved in the future
BUG=skia:1638
Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=262761
Patch Set 1 : Re-land https://codereview.chromium.org/192323006/ #Patch Set 2 : Fix for unmanaged git(-svn?) checkouts #Patch Set 3 : Instead of erroring out on a conflicting checkout, move it to a _bad_scm dir #Patch Set 4 : Refactor delete/move logic, add random number to move dest #
Total comments: 2
Patch Set 5 : Use tempfile #Patch Set 6 : Fix creation of _bad_scm dir #Patch Set 7 : Fix makedirs again ( rebase) #Patch Set 8 : Don't actually move or delete anything; just print an urgent warning. #Patch Set 9 : Fix warning #
Total comments: 4
Patch Set 10 : Remove commented lines #Patch Set 11 : rebase+fix #
Messages
Total messages: 32 (0 generated)
Nico, can you apply this locally and see if it works well with the tweaked .gclient?
cd ~/src/depot_tools; curl https://codereview.chromium.org/download/issue189913020_20001.diff | patch -p1; cd - gclient sync Error: 1> Can't update/checkout /Volumes/MacintoshHD2/src/chrome-git/src if an unversioned directory is present. Delete the directory and try again. # edit ../.gclient to say "unmanaged": "false" (this should be a psa to chromium-dev if it's needed) $ gclient sync Error: 1> Can't update/checkout /Volumes/MacintoshHD2/src/chrome-git/src if an unversioned directory is present. Delete the directory and try again. hummer:src thakis$ mvim ../.gclient hummer:src thakis$ gclient sync # I didn't update src, so I'd expect this to do nothing ________ unmanaged solution; skipping src Syncing projects: 12% (11/87) src/third_party/bidichecker _____ src/third_party/WebKit/LayoutTests/w3c/csswg-test at 8c415e3215a203fa3a22dbdd1799279fdf44c81e Syncing projects: 16% (14/87) src/testing/gmock _____ src/third_party/WebKit/LayoutTests/w3c/web-platform-tests at ac4322a338be82b3d8b722917d6d3d057c0a3f6a _____ src/chrome/browser/resources/pdf/html_office at 9f76cc282c471ae4ff77415384db039fcab2faa8 Syncing projects: 20% (18/87) src/third_party/cld_2/src _____ src/third_party/brotli/src at 7f848593bd2ec83f4537b6d494a5bf55b9bd4456 Syncing projects: 40% (35/87) src/sdch/open-vcdiff _____ src/third_party/angle : Attempting rebase onto fa63e947cb3eccf463648d21a05d5002c9b8adfa... First, rewinding head to replay your work on top of it... Fast-forwarded master to fa63e947cb3eccf463648d21a05d5002c9b8adfa. Error: 43> Can't update/checkout /Volumes/MacintoshHD2/src/chrome-git/src/third_party/libc++abi/trunk if an unversioned directory is present. Delete the directory and try again. hummer:src thakis$ So it fails somewhere else. Not sure if this is my client being in a weird state, but things work without the patch. On Mon, Mar 10, 2014 at 1:28 PM, <iannucci@chromium.org> wrote: > Nico, can you apply this locally and see if it works well with the tweaked > .gclient? > > https://codereview.chromium.org/189913020/ > To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
On 2014/03/10 20:33:47, Nico wrote: > cd ~/src/depot_tools; curl > https://codereview.chromium.org/download/issue189913020_20001.diff | patch > -p1; cd - > gclient sync > Error: 1> Can't update/checkout /Volumes/MacintoshHD2/src/chrome-git/src if > an unversioned directory is present. Delete the directory and try again. > # edit ../.gclient to say "unmanaged": "false" (this should be a psa to > chromium-dev if it's needed) > > $ gclient sync > Error: 1> Can't update/checkout /Volumes/MacintoshHD2/src/chrome-git/src if > an unversioned directory is present. Delete the directory and try again. > hummer:src thakis$ mvim ../.gclient > hummer:src thakis$ gclient sync # I didn't update src, so I'd expect this > to do nothing > ________ unmanaged solution; skipping src > Syncing projects: 12% (11/87) src/third_party/bidichecker > _____ src/third_party/WebKit/LayoutTests/w3c/csswg-test at > 8c415e3215a203fa3a22dbdd1799279fdf44c81e > Syncing projects: 16% (14/87) src/testing/gmock > _____ src/third_party/WebKit/LayoutTests/w3c/web-platform-tests at > ac4322a338be82b3d8b722917d6d3d057c0a3f6a > > _____ src/chrome/browser/resources/pdf/html_office at > 9f76cc282c471ae4ff77415384db039fcab2faa8 > Syncing projects: 20% (18/87) src/third_party/cld_2/src > > _____ src/third_party/brotli/src at 7f848593bd2ec83f4537b6d494a5bf55b9bd4456 > Syncing projects: 40% (35/87) src/sdch/open-vcdiff > _____ src/third_party/angle : Attempting rebase onto > fa63e947cb3eccf463648d21a05d5002c9b8adfa... > First, rewinding head to replay your work on top of it... > Fast-forwarded master to fa63e947cb3eccf463648d21a05d5002c9b8adfa. > > Error: 43> Can't update/checkout > /Volumes/MacintoshHD2/src/chrome-git/src/third_party/libc++abi/trunk if an > unversioned directory is present. Delete the directory and try again. > hummer:src thakis$ > > > So it fails somewhere else. Not sure if this is my client being in a weird > state, but things work without the patch. > > > > > On Mon, Mar 10, 2014 at 1:28 PM, <mailto:iannucci@chromium.org> wrote: > > > Nico, can you apply this locally and see if it works well with the tweaked > > .gclient? > > > > https://codereview.chromium.org/189913020/ > > > > To unsubscribe from this group and stop receiving emails from it, send an email > to mailto:chromium-reviews+unsubscribe@chromium.org. I think that dependency was changed or moved recently (last night)?
libc++ recently switched from SVN -> GIT (which would have been handled by Eric's patch :) ) On Mon, Mar 10, 2014 at 1:35 PM, <borenet@google.com> wrote: > On 2014/03/10 20:33:47, Nico wrote: > >> cd ~/src/depot_tools; curl >> https://codereview.chromium.org/download/issue189913020_20001.diff | >> patch >> -p1; cd - >> gclient sync >> Error: 1> Can't update/checkout /Volumes/MacintoshHD2/src/chrome-git/src >> if >> an unversioned directory is present. Delete the directory and try again. >> # edit ../.gclient to say "unmanaged": "false" (this should be a psa to >> chromium-dev if it's needed) >> > > $ gclient sync >> Error: 1> Can't update/checkout /Volumes/MacintoshHD2/src/chrome-git/src >> if >> an unversioned directory is present. Delete the directory and try again. >> hummer:src thakis$ mvim ../.gclient >> hummer:src thakis$ gclient sync # I didn't update src, so I'd expect this >> to do nothing >> ________ unmanaged solution; skipping src >> Syncing projects: 12% (11/87) src/third_party/bidichecker >> _____ src/third_party/WebKit/LayoutTests/w3c/csswg-test at >> 8c415e3215a203fa3a22dbdd1799279fdf44c81e >> Syncing projects: 16% (14/87) src/testing/gmock >> _____ src/third_party/WebKit/LayoutTests/w3c/web-platform-tests at >> ac4322a338be82b3d8b722917d6d3d057c0a3f6a >> > > _____ src/chrome/browser/resources/pdf/html_office at >> 9f76cc282c471ae4ff77415384db039fcab2faa8 >> Syncing projects: 20% (18/87) src/third_party/cld_2/src >> > > _____ src/third_party/brotli/src at 7f848593bd2ec83f4537b6d494a5bf >> 55b9bd4456 >> Syncing projects: 40% (35/87) src/sdch/open-vcdiff >> _____ src/third_party/angle : Attempting rebase onto >> fa63e947cb3eccf463648d21a05d5002c9b8adfa... >> First, rewinding head to replay your work on top of it... >> Fast-forwarded master to fa63e947cb3eccf463648d21a05d5002c9b8adfa. >> > > Error: 43> Can't update/checkout >> /Volumes/MacintoshHD2/src/chrome-git/src/third_party/libc++abi/trunk if >> an >> unversioned directory is present. Delete the directory and try again. >> hummer:src thakis$ >> > > > So it fails somewhere else. Not sure if this is my client being in a weird >> state, but things work without the patch. >> > > > > > On Mon, Mar 10, 2014 at 1:28 PM, <mailto:iannucci@chromium.org> wrote: >> > > > Nico, can you apply this locally and see if it works well with the >> tweaked >> > .gclient? >> > >> > https://codereview.chromium.org/189913020/ >> > >> > > To unsubscribe from this group and stop receiving emails from it, send an >> > email > >> to mailto:chromium-reviews+unsubscribe@chromium.org. >> > > I think that dependency was changed or moved recently (last night)? > > https://codereview.chromium.org/189913020/ > To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
Not that recently, almost a week ago: http://src.chromium.org/viewvc/chrome?revision=254959&view=revision To reiterate, `gclient sync` works fine without this patch but errors with this patch, so it seems it isn't handled? On Mon, Mar 10, 2014 at 1:36 PM, Robert Iannucci <iannucci@chromium.org>wrote: > libc++ recently switched from SVN -> GIT (which would have been handled by > Eric's patch :) ) > > > On Mon, Mar 10, 2014 at 1:35 PM, <borenet@google.com> wrote: > >> On 2014/03/10 20:33:47, Nico wrote: >> >>> cd ~/src/depot_tools; curl >>> https://codereview.chromium.org/download/issue189913020_20001.diff | >>> patch >>> -p1; cd - >>> gclient sync >>> Error: 1> Can't update/checkout /Volumes/MacintoshHD2/src/chrome-git/src >>> if >>> an unversioned directory is present. Delete the directory and try again. >>> # edit ../.gclient to say "unmanaged": "false" (this should be a psa to >>> chromium-dev if it's needed) >>> >> >> $ gclient sync >>> Error: 1> Can't update/checkout /Volumes/MacintoshHD2/src/chrome-git/src >>> if >>> an unversioned directory is present. Delete the directory and try again. >>> hummer:src thakis$ mvim ../.gclient >>> hummer:src thakis$ gclient sync # I didn't update src, so I'd expect >>> this >>> to do nothing >>> ________ unmanaged solution; skipping src >>> Syncing projects: 12% (11/87) src/third_party/bidichecker >>> _____ src/third_party/WebKit/LayoutTests/w3c/csswg-test at >>> 8c415e3215a203fa3a22dbdd1799279fdf44c81e >>> Syncing projects: 16% (14/87) src/testing/gmock >>> _____ src/third_party/WebKit/LayoutTests/w3c/web-platform-tests at >>> ac4322a338be82b3d8b722917d6d3d057c0a3f6a >>> >> >> _____ src/chrome/browser/resources/pdf/html_office at >>> 9f76cc282c471ae4ff77415384db039fcab2faa8 >>> Syncing projects: 20% (18/87) src/third_party/cld_2/src >>> >> >> _____ src/third_party/brotli/src at 7f848593bd2ec83f4537b6d494a5bf >>> 55b9bd4456 >>> Syncing projects: 40% (35/87) src/sdch/open-vcdiff >>> _____ src/third_party/angle : Attempting rebase onto >>> fa63e947cb3eccf463648d21a05d5002c9b8adfa... >>> First, rewinding head to replay your work on top of it... >>> Fast-forwarded master to fa63e947cb3eccf463648d21a05d5002c9b8adfa. >>> >> >> Error: 43> Can't update/checkout >>> /Volumes/MacintoshHD2/src/chrome-git/src/third_party/libc++abi/trunk if >>> an >>> unversioned directory is present. Delete the directory and try again. >>> hummer:src thakis$ >>> >> >> >> So it fails somewhere else. Not sure if this is my client being in a >>> weird >>> state, but things work without the patch. >>> >> >> >> >> >> On Mon, Mar 10, 2014 at 1:28 PM, <mailto:iannucci@chromium.org> wrote: >>> >> >> > Nico, can you apply this locally and see if it works well with the >>> tweaked >>> > .gclient? >>> > >>> > https://codereview.chromium.org/189913020/ >>> > >>> >> >> To unsubscribe from this group and stop receiving emails from it, send an >>> >> email >> >>> to mailto:chromium-reviews+unsubscribe@chromium.org. >>> >> >> I think that dependency was changed or moved recently (last night)? >> >> https://codereview.chromium.org/189913020/ >> > > To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
For debugging's sake, can you list the contents of src/third_party/libc++abi/trunk? Notably, if there's an svn checkout there, and DEPS has a git checkout listed, this is expected behavior without --force.
It's the other way round, DEPS lists svn, third_party/libc++ is git: $ ls -a third_party/libc++/trunk/ . .. .arcconfig .git CMakeLists.txt CREDITS.TXT LICENSE.TXT Makefile cmake include lib src test www I've never run `gclient sync` with --force I think; most others might not either. Everyone who synced in the last few months will have a .git libc++ checkout in their third_party/libc++. (Does the current gclient just silently not update these directories?) On Mon, Mar 10, 2014 at 1:50 PM, <borenet@google.com> wrote: > For debugging's sake, can you list the contents of > src/third_party/libc++abi/trunk? Notably, if there's an svn checkout > there, and > DEPS has a git checkout listed, this is expected behavior without --force. > > https://codereview.chromium.org/189913020/ > To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
Ah, so it looks like they actually switched from git to svn. In this case, I think gclient was incorrectly skipping that directory due to the presence of the .git directory. On 2014/03/10 20:57:08, Nico wrote: > It's the other way round, DEPS lists svn, third_party/libc++ is git: > > $ ls -a third_party/libc++/trunk/ > . .. .arcconfig .git CMakeLists.txt CREDITS.TXT LICENSE.TXT Makefile cmake > include lib src test www > > I've never run `gclient sync` with --force I think; most others might not > either. Everyone who synced in the last few months will have a .git libc++ > checkout in their third_party/libc++. (Does the current gclient just > silently not update these directories?) > > > On Mon, Mar 10, 2014 at 1:50 PM, <mailto:borenet@google.com> wrote: > > > For debugging's sake, can you list the contents of > > src/third_party/libc++abi/trunk? Notably, if there's an svn checkout > > there, and > > DEPS has a git checkout listed, this is expected behavior without --force. > > > > https://codereview.chromium.org/189913020/ > > > > To unsubscribe from this group and stop receiving emails from it, send an email > to mailto:chromium-reviews+unsubscribe@chromium.org.
Fixing this sounds great :-) Maybe there could be short-term special case code that does "if libc++ or libc++abi, pretend --force was passed" in gclient for a week or two, since almost everyone will have this problem? On Mon, Mar 10, 2014 at 2:13 PM, <borenet@google.com> wrote: > Ah, so it looks like they actually switched from git to svn. In this > case, I > think gclient was incorrectly skipping that directory due to the presence > of the > .git directory. > > > On 2014/03/10 20:57:08, Nico wrote: > >> It's the other way round, DEPS lists svn, third_party/libc++ is git: >> > > $ ls -a third_party/libc++/trunk/ >> . .. .arcconfig .git CMakeLists.txt CREDITS.TXT LICENSE.TXT Makefile cmake >> include lib src test www >> > > I've never run `gclient sync` with --force I think; most others might not >> either. Everyone who synced in the last few months will have a .git libc++ >> checkout in their third_party/libc++. (Does the current gclient just >> silently not update these directories?) >> > > > On Mon, Mar 10, 2014 at 1:50 PM, <mailto:borenet@google.com> wrote: >> > > > For debugging's sake, can you list the contents of >> > src/third_party/libc++abi/trunk? Notably, if there's an svn checkout >> > there, and >> > DEPS has a git checkout listed, this is expected behavior without >> --force. >> > >> > https://codereview.chromium.org/189913020/ >> > >> > > To unsubscribe from this group and stop receiving emails from it, send an >> > email > >> to mailto:chromium-reviews+unsubscribe@chromium.org. >> > > > > https://codereview.chromium.org/189913020/ > To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
Personally, I'd like to find a general solution, especially since this type of DEPS change seems pretty common lately. As for automatically detecting and fixing problems with .gclient or checked-out DEPS, my opinion (which, based on the code already in gclient, is probably not shared by chrome-infra) is that trying to do too much magic only sets us up for code maintenance problems or subtle problems like the above. I think we'll benefit from being strict and either aggressively deleting things to get us into a good state (eg. on the bots) or just failing without trying to do any magic (with a hint, eg. "You should delete /some/problematic/path"). I think requiring a small amount of manual action is okay assuming that a warning is sent out beforehand and there's a helpful error message. Of course, I'm happy to take whatever approach is deemed best. On 2014/03/10 21:45:07, Nico wrote: > Fixing this sounds great :-) > > Maybe there could be short-term special case code that does "if libc++ or > libc++abi, pretend --force was passed" in gclient for a week or two, since > almost everyone will have this problem? > > > On Mon, Mar 10, 2014 at 2:13 PM, <mailto:borenet@google.com> wrote: > > > Ah, so it looks like they actually switched from git to svn. In this > > case, I > > think gclient was incorrectly skipping that directory due to the presence > > of the > > .git directory. > > > > > > On 2014/03/10 20:57:08, Nico wrote: > > > >> It's the other way round, DEPS lists svn, third_party/libc++ is git: > >> > > > > $ ls -a third_party/libc++/trunk/ > >> . .. .arcconfig .git CMakeLists.txt CREDITS.TXT LICENSE.TXT Makefile cmake > >> include lib src test www > >> > > > > I've never run `gclient sync` with --force I think; most others might not > >> either. Everyone who synced in the last few months will have a .git libc++ > >> checkout in their third_party/libc++. (Does the current gclient just > >> silently not update these directories?) > >> > > > > > > On Mon, Mar 10, 2014 at 1:50 PM, <mailto:borenet@google.com> wrote: > >> > > > > > For debugging's sake, can you list the contents of > >> > src/third_party/libc++abi/trunk? Notably, if there's an svn checkout > >> > there, and > >> > DEPS has a git checkout listed, this is expected behavior without > >> --force. > >> > > >> > https://codereview.chromium.org/189913020/ > >> > > >> > > > > To unsubscribe from this group and stop receiving emails from it, send an > >> > > email > > > >> to mailto:chromium-reviews+unsubscribe@chromium.org. > >> > > > > > > > > https://codereview.chromium.org/189913020/ > > > > To unsubscribe from this group and stop receiving emails from it, send an email > to mailto:chromium-reviews+unsubscribe@chromium.org.
On Tue, Mar 11, 2014 at 6:18 AM, <borenet@google.com> wrote: > Personally, I'd like to find a general solution, especially since this > type of > DEPS change seems pretty common lately. > > As for automatically detecting and fixing problems with .gclient or > checked-out > DEPS, my opinion (which, based on the code already in gclient, is probably > not > shared by chrome-infra) is that trying to do too much magic only sets us > up for > code maintenance problems or subtle problems like the above. I think we'll > benefit from being strict and either aggressively deleting things to get > us into > a good state (eg. on the bots) or just failing without trying to do any > magic > (with a hint, eg. "You should delete /some/problematic/path"). I think > requiring a small amount of manual action is okay assuming that a warning > is > sent out beforehand and there's a helpful error message. > I agree with the general sentiment of not having magic, but short-lived magic (2 weeks) that saves hundreds of people some work is fine imho. > > Of course, I'm happy to take whatever approach is deemed best. > > > On 2014/03/10 21:45:07, Nico wrote: > >> Fixing this sounds great :-) >> > > Maybe there could be short-term special case code that does "if libc++ or >> libc++abi, pretend --force was passed" in gclient for a week or two, since >> almost everyone will have this problem? >> > > > On Mon, Mar 10, 2014 at 2:13 PM, <mailto:borenet@google.com> wrote: >> > > > Ah, so it looks like they actually switched from git to svn. In this >> > case, I >> > think gclient was incorrectly skipping that directory due to the >> presence >> > of the >> > .git directory. >> > >> > >> > On 2014/03/10 20:57:08, Nico wrote: >> > >> >> It's the other way round, DEPS lists svn, third_party/libc++ is git: >> >> >> > >> > $ ls -a third_party/libc++/trunk/ >> >> . .. .arcconfig .git CMakeLists.txt CREDITS.TXT LICENSE.TXT Makefile >> cmake >> >> include lib src test www >> >> >> > >> > I've never run `gclient sync` with --force I think; most others might >> not >> >> either. Everyone who synced in the last few months will have a .git >> libc++ >> >> checkout in their third_party/libc++. (Does the current gclient just >> >> silently not update these directories?) >> >> >> > >> > >> > On Mon, Mar 10, 2014 at 1:50 PM, <mailto:borenet@google.com> wrote: >> >> >> > >> > > For debugging's sake, can you list the contents of >> >> > src/third_party/libc++abi/trunk? Notably, if there's an svn >> checkout >> >> > there, and >> >> > DEPS has a git checkout listed, this is expected behavior without >> >> --force. >> >> > >> >> > https://codereview.chromium.org/189913020/ >> >> > >> >> >> > >> > To unsubscribe from this group and stop receiving emails from it, send >> an >> >> >> > email >> > >> >> to mailto:chromium-reviews+unsubscribe@chromium.org. >> >> >> > >> > >> > >> > https://codereview.chromium.org/189913020/ >> > >> > > To unsubscribe from this group and stop receiving emails from it, send an >> > email > >> to mailto:chromium-reviews+unsubscribe@chromium.org. >> > > > > https://codereview.chromium.org/189913020/ > To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
At this point I'm OK with either the general solution or the specific magic hack (though I know for certain that there are other projects which want to do this switch immediately after skia). Gclient is so ill-defined in its responsibilities at this point that it's likely to break /something/, and everyone has their own 'workflow', most of which aren't tested. What if we just moved the offending repo to the side instead of deleting it? So: .gclient src/path/to/dep turns into: .gclient src/path/to/dep _bad_scm/path/to/dep.<random> If it's --force, we can just remove it. Otherwise we print out an alert that their repo moved to the other place. If they had changes in that repo, then they can salvage them however they want. Otherwise they can just delete _bad_scm. WDYT? R On Tue, Mar 11, 2014 at 9:54 AM, Nico Weber <thakis@chromium.org> wrote: > On Tue, Mar 11, 2014 at 6:18 AM, <borenet@google.com> wrote: > >> Personally, I'd like to find a general solution, especially since this >> type of >> DEPS change seems pretty common lately. >> >> As for automatically detecting and fixing problems with .gclient or >> checked-out >> DEPS, my opinion (which, based on the code already in gclient, is >> probably not >> shared by chrome-infra) is that trying to do too much magic only sets us >> up for >> code maintenance problems or subtle problems like the above. I think >> we'll >> benefit from being strict and either aggressively deleting things to get >> us into >> a good state (eg. on the bots) or just failing without trying to do any >> magic >> (with a hint, eg. "You should delete /some/problematic/path"). I think >> requiring a small amount of manual action is okay assuming that a warning >> is >> sent out beforehand and there's a helpful error message. >> > > I agree with the general sentiment of not having magic, but short-lived > magic (2 weeks) that saves hundreds of people some work is fine imho. > > >> >> Of course, I'm happy to take whatever approach is deemed best. >> >> >> On 2014/03/10 21:45:07, Nico wrote: >> >>> Fixing this sounds great :-) >>> >> >> Maybe there could be short-term special case code that does "if libc++ or >>> libc++abi, pretend --force was passed" in gclient for a week or two, >>> since >>> almost everyone will have this problem? >>> >> >> >> On Mon, Mar 10, 2014 at 2:13 PM, <mailto:borenet@google.com> wrote: >>> >> >> > Ah, so it looks like they actually switched from git to svn. In this >>> > case, I >>> > think gclient was incorrectly skipping that directory due to the >>> presence >>> > of the >>> > .git directory. >>> > >>> > >>> > On 2014/03/10 20:57:08, Nico wrote: >>> > >>> >> It's the other way round, DEPS lists svn, third_party/libc++ is git: >>> >> >>> > >>> > $ ls -a third_party/libc++/trunk/ >>> >> . .. .arcconfig .git CMakeLists.txt CREDITS.TXT LICENSE.TXT Makefile >>> cmake >>> >> include lib src test www >>> >> >>> > >>> > I've never run `gclient sync` with --force I think; most others might >>> not >>> >> either. Everyone who synced in the last few months will have a .git >>> libc++ >>> >> checkout in their third_party/libc++. (Does the current gclient just >>> >> silently not update these directories?) >>> >> >>> > >>> > >>> > On Mon, Mar 10, 2014 at 1:50 PM, <mailto:borenet@google.com> wrote: >>> >> >>> > >>> > > For debugging's sake, can you list the contents of >>> >> > src/third_party/libc++abi/trunk? Notably, if there's an svn >>> checkout >>> >> > there, and >>> >> > DEPS has a git checkout listed, this is expected behavior without >>> >> --force. >>> >> > >>> >> > https://codereview.chromium.org/189913020/ >>> >> > >>> >> >>> > >>> > To unsubscribe from this group and stop receiving emails from it, >>> send an >>> >> >>> > email >>> > >>> >> to mailto:chromium-reviews+unsubscribe@chromium.org. >>> >> >>> > >>> > >>> > >>> > https://codereview.chromium.org/189913020/ >>> > >>> >> >> To unsubscribe from this group and stop receiving emails from it, send an >>> >> email >> >>> to mailto:chromium-reviews+unsubscribe@chromium.org. >>> >> >> >> >> https://codereview.chromium.org/189913020/ >> > > To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
Uploaded patch set 4, which moves the offending directory to a _bad_scm.#### directory. I'm still worried about developers with broken .gclient files. If they're using managed checkouts with a URL that doesn't match what's checked out, as of this patch set gclient will happily move their entire checkout to the side and download from scratch. Do we know of a good way to prevent this? I'm looking into detecting this and erroring out immediately.
On 2014/03/11 21:18:42, borenet wrote: > Uploaded patch set 4, which moves the offending directory to a _bad_scm.#### > directory. I'm still worried about developers with broken .gclient files. If > they're using managed checkouts with a URL that doesn't match what's checked > out, as of this patch set gclient will happily move their entire checkout to the > side and download from scratch. Do we know of a good way to prevent this? I'm > looking into detecting this and erroring out immediately. I'm experimenting with erroring out if the .gclient looks wrong here: https://codereview.chromium.org/195913002/
On Tue, Mar 11, 2014 at 2:18 PM, <borenet@google.com> wrote: > Uploaded patch set 4, which moves the offending directory to a > _bad_scm.#### > directory. I'm still worried about developers with broken .gclient files. > If > they're using managed checkouts with a URL that doesn't match what's > checked > out, as of this patch set gclient will happily move their entire checkout > to the > side and download from scratch. Do we know of a good way to prevent this? > I'm > looking into detecting this and erroring out immediately. > Don't do this if the dir name ends in "src" and contains subfolders called "chrome", "content", and "v8"? > > https://codereview.chromium.org/189913020/ > To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
This lgtm. The flows I think it covers are:
* .gclient says 'svn', but it's really git-svn: do nothing
* Should we emit a warning?
* DEPS says 'svn', but it's really git: delete or move
* DEPS says 'git', but it's really svn: delete or move
Are there others I missed?
https://codereview.chromium.org/189913020/diff/60001/gclient_scm.py
File gclient_scm.py (right):
https://codereview.chromium.org/189913020/diff/60001/gclient_scm.py#newcode158
gclient_scm.py:158: % (self.checkout_path, dest_path))
I would probably use
http://docs.python.org/2/library/tempfile.html#tempfile.mkdtemp with
prefix=basename(relpath), dir=join(root_dir, '_bad_scm', dirname(relpath)).
More precisely: * .gclient says 'svn' AND "git config --get svn-remote.svn.url" is the expected URL: do nothing * DEPS says 'svn' but "svn info ." fails: delete or move * DEPS says 'git' but there's no .git directory: delete or move https://codereview.chromium.org/189913020/diff/60001/gclient_scm.py File gclient_scm.py (right): https://codereview.chromium.org/189913020/diff/60001/gclient_scm.py#newcode158 gclient_scm.py:158: % (self.checkout_path, dest_path)) On 2014/03/12 19:55:25, iannucci wrote: > I would probably use > http://docs.python.org/2/library/tempfile.html#tempfile.mkdtemp with > prefix=basename(relpath), dir=join(root_dir, '_bad_scm', dirname(relpath)). Done.
Uploaded patch set 7.
On 2014/04/03 18:57:18, borenet wrote: > Uploaded patch set 7. Do you plan to do a 'warning' version of this first?
Per the above comment, taking a slightly more careful approach: rather than delete/move, just print a warning at first. The warning should not occur unless the checkout is broken or the DEPS changed, so if anyone sees the warning it will most likely indicate a bug in my code.
lgtm, though if you could prepare the followup CL and mention it in this one, that would be The Best. If not, then I'm OK to land with the commented lines. https://codereview.chromium.org/189913020/diff/160001/gclient_scm.py File gclient_scm.py (right): https://codereview.chromium.org/189913020/diff/160001/gclient_scm.py#newcode8 gclient_scm.py:8: #import errno Probably should just leave these comments out and have them standing by in another CL. https://codereview.chromium.org/189913020/diff/160001/gclient_scm.py#newcode182 gclient_scm.py:182: gclient_utils.AddWarning('WARNING: Upcoming changes would cause %s to be ' Prep the cl to turn this feature on, and mention it in the warning message.
Uploaded patch set 10 and created followup CL: https://codereview.chromium.org/225403015/ ("gclient: Actually move or delete mismatched checkouts") https://codereview.chromium.org/189913020/diff/160001/gclient_scm.py File gclient_scm.py (right): https://codereview.chromium.org/189913020/diff/160001/gclient_scm.py#newcode8 gclient_scm.py:8: #import errno On 2014/04/04 00:49:54, iannucci wrote: > Probably should just leave these comments out and have them standing by in > another CL. Done. https://codereview.chromium.org/189913020/diff/160001/gclient_scm.py#newcode182 gclient_scm.py:182: gclient_utils.AddWarning('WARNING: Upcoming changes would cause %s to be ' On 2014/04/04 00:49:54, iannucci wrote: > Prep the cl to turn this feature on, and mention it in the warning message. Done.
lgtm
The CQ bit was checked by borenet@google.com
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/borenet@google.com/189913020/180001
The CQ bit was unchecked by commit-bot@chromium.org
Failed to apply patch for depot_tools/gclient_scm.py:
While running patch -p1 --forward --force --no-backup-if-mismatch;
patching file depot_tools/gclient_scm.py
Hunk #1 succeeded at 198 (offset 33 lines).
Hunk #2 succeeded at 376 with fuzz 2 (offset 34 lines).
Hunk #3 succeeded at 397 with fuzz 1 (offset 35 lines).
Hunk #4 succeeded at 406 (offset 35 lines).
Hunk #5 FAILED at 1035.
Hunk #6 FAILED at 1071.
Hunk #7 FAILED at 1165.
Hunk #8 succeeded at 1248 (offset 40 lines).
3 out of 8 hunks FAILED -- saving rejects to file
depot_tools/gclient_scm.py.rej
Patch: depot_tools/gclient_scm.py
Index: gclient_scm.py
diff --git depot_tools/gclient_scm.py depot_tools/gclient_scm.py
index
913d9a57e6746eba82bda263d430a11163696e08..afd5852e40eccdc2ae37748219ebe05c7d459f0d
100644
--- a/depot_tools/gclient_scm.py
+++ b/depot_tools/gclient_scm.py
@@ -165,6 +165,22 @@ class SCMWrapper(object):
# valid git or svn checkout.
return False
+ # TODO(borenet): Remove this once SCMWrapper._DeleteOrMove is enabled.
+ # pylint: disable=R0201
+ def _DeleteOrMove(self, force):
+ """Delete the checkout directory or move it out of the way.
+
+ Args:
+ force: bool; if True, delete the directory. Otherwise, just move it.
+ """
+ gclient_utils.AddWarning('WARNING: Upcoming change in '
+ 'https://codereview.chromium.org/225403015 would '
+ 'cause %s to be deleted or moved to the side. '
+ 'This is intended to ease changes to DEPS in the '
+ 'future. If you are seeing this warning and '
+ 'haven\'t changed the DEPS file, please contact '
+ 'borenet@ immediately.' % self.checkout_path)
+
class GitWrapper(SCMWrapper):
"""Wrapper for Git"""
@@ -326,6 +342,9 @@ class GitWrapper(SCMWrapper):
if (not os.path.exists(self.checkout_path) or
(os.path.isdir(self.checkout_path) and
not os.path.exists(os.path.join(self.checkout_path, '.git')))):
+ if (os.path.isdir(self.checkout_path) and
+ not os.path.exists(os.path.join(self.checkout_path, '.git'))):
+ self._DeleteOrMove(options.force)
self._Clone(revision, url, options)
self.UpdateSubmoduleConfig()
if file_list is not None:
@@ -343,14 +362,6 @@ class GitWrapper(SCMWrapper):
print ('________ unmanaged solution; skipping %s' % self.relpath)
return self._Capture(['rev-parse', '--verify', 'HEAD'])
- if not os.path.exists(os.path.join(self.checkout_path, '.git')):
- raise gclient_utils.Error('\n____ %s%s\n'
- '\tPath is not a git repo. No .git dir.\n'
- '\tTo resolve:\n'
- '\t\trm -rf %s\n'
- '\tAnd run gclient sync again\n'
- % (self.relpath, rev_str, self.relpath))
-
# See if the url has changed (the unittests use git://foo for the url, let
# that through).
current_url = self._Capture(['config', 'remote.%s.url' % self.remote])
@@ -360,7 +371,7 @@ class GitWrapper(SCMWrapper):
# Skip url auto-correction if remote.origin.gclient-auto-fix-url is set.
# This allows devs to use experimental repos which have a different url
# but whose branch(s) are the same as official repos.
- if (current_url != url and
+ if (current_url.rstrip('/') != url.rstrip('/') and
url != 'git://foo' and
subprocess2.capture(
['git', 'config', 'remote.%s.gclient-auto-fix-url' % self.remote],
@@ -1024,12 +1035,7 @@ class SVNWrapper(SCMWrapper):
Raises:
Error: if can't get URL for relative path.
"""
- # Only update if git or hg is not controlling the directory.
- git_path = os.path.join(self.checkout_path, '.git')
- if os.path.exists(git_path):
- print('________ found .git directory; skipping %s' % self.relpath)
- return
-
+ # Only update if hg is not controlling the directory.
hg_path = os.path.join(self.checkout_path, '.hg')
if os.path.exists(hg_path):
print('________ found .hg directory; skipping %s' % self.relpath)
@@ -1060,21 +1066,26 @@ class SVNWrapper(SCMWrapper):
forced_revision = False
rev_str = ''
- # Get the existing scm url and the revision number of the current checkout.
exists = os.path.exists(self.checkout_path)
if exists and managed:
+ # Git is only okay if it's a git-svn checkout of the right repo.
+ if scm.GIT.IsGitSvn(self.checkout_path):
+ remote_url = scm.GIT.Capture(['config', '--local', '--get',
+ 'svn-remote.svn.url'],
+ cwd=self.checkout_path).rstrip()
+ if remote_url.rstrip('/') == base_url.rstrip('/'):
+ print('\n_____ %s looks like a git-svn checkout. Skipping.'
+ % self.relpath)
+ return # TODO(borenet): Get the svn revision number?
+
+ # Get the existing scm url and the revision number of the current checkout.
+ if exists and managed:
try:
from_info = scm.SVN.CaptureLocalInfo(
[], os.path.join(self.checkout_path, '.'))
except (gclient_utils.Error, subprocess2.CalledProcessError):
- if options.reset and options.delete_unversioned_trees:
- print 'Removing troublesome path %s' % self.checkout_path
- gclient_utils.rmtree(self.checkout_path)
- exists = False
- else:
- msg = ('Can\'t update/checkout %s if an unversioned directory is '
- 'present. Delete the directory and try again.')
- raise gclient_utils.Error(msg % self.checkout_path)
+ self._DeleteOrMove(options.force)
+ exists = False
BASE_URLS = {
'/chrome/trunk/src': 'gs://chromium-svn-checkout/chrome/',
@@ -1154,7 +1165,9 @@ class SVNWrapper(SCMWrapper):
if not managed:
print ('________ unmanaged solution; skipping %s' % self.relpath)
- return self.Svnversion()
+ if os.path.exists(os.path.join(self.checkout_path, '.svn')):
+ return self.Svnversion()
+ return
if 'URL' not in from_info:
raise gclient_utils.Error(
@@ -1197,7 +1210,7 @@ class SVNWrapper(SCMWrapper):
revision = str(from_info_live['Revision'])
rev_str = ' at %s' % revision
- if from_info['URL'] != base_url:
+ if from_info['URL'].rstrip('/') != base_url.rstrip('/'):
# The repository url changed, need to switch.
try:
to_info = scm.SVN.CaptureRemoteInfo(url)
The CQ bit was checked by borenet@google.com
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/borenet@google.com/189913020/200001
Message was sent while issue was closed.
Change committed as 262761 |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
