Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
52663008:
Add a new gclient-new-workdir script which clones an existing gclient working directory much like g… (Closed)
|
Created:
7 years, 1 month ago by atreat Modified:
7 years, 1 month ago CC:
chromium-reviews, Dirk Pranke, cmp-cc_chromium.org, iannucci+depot_tools_chromium.org, ilevy-cc_chromium.org, lgombos, Inactive Base URL:
https://chromium.googlesource.com/chromium/tools/depot_tools.git@master Visibility:
Public. |
DescriptionAdd a new gclient-new-workdir script which clones an existing gclient working directory much like git-new-workdir, but takes into account all sub projects as well.
BUG=none
Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=232992
Patch Set 1 #
Total comments: 4
Patch Set 2 : Fix according to comments #
Total comments: 13
Patch Set 3 : More cleanup #Patch Set 4 : Further fixes #
Total comments: 2
Patch Set 5 : Fixed up again #Patch Set 6 : Fix for PyLint in unused variable 'files' in os.walk #Messages
Total messages: 20 (0 generated)
Please have a look. My python is a bit rusty, but this works for me where git-new-workdir failed.
Hm. If there is interest in this sort of thing, I would highly recommend extending the git cache_dir implementation in gclient to be more developer-friendly. Right now it's mainly for saving space/time on the bots. Basic premise is: * Clone full mirrors of all repos in gclient project to /some/path/to/cache_dir * Create 'local' clones from these mirror repos (i.e. don't clone any objects, just do a checkout) When you commit, the new objects go into the local clone. When you fetch, gclient fetch'es the mirrors, and then does a pull in the local clone. The size of the local clone is just the size of the checkout without any history (i.e. about half the size of a SVN checkout).
On 2013/11/04 20:10:12, iannucci wrote: > Hm. If there is interest in this sort of thing, I would highly recommend > extending the git cache_dir implementation in gclient to be more > developer-friendly. Right now it's mainly for saving space/time on the bots. > > Basic premise is: > * Clone full mirrors of all repos in gclient project to > /some/path/to/cache_dir > * Create 'local' clones from these mirror repos (i.e. don't clone any objects, > just do a checkout) > > When you commit, the new objects go into the local clone. When you fetch, > gclient fetch'es the mirrors, and then does a pull in the local clone. > > The size of the local clone is just the size of the checkout without any history > (i.e. about half the size of a SVN checkout). When I do a commit in one of my working directories I want it to update my local 'global' git database so I can see that object/branch in my other working directories. I want the same functionality as git-new-workdir but for gclient. Thought perhaps others would like the same...
On 2013/11/04 20:13:37, atreat wrote: > On 2013/11/04 20:10:12, iannucci wrote: > > Hm. If there is interest in this sort of thing, I would highly recommend > > extending the git cache_dir implementation in gclient to be more > > developer-friendly. Right now it's mainly for saving space/time on the bots. > > > > Basic premise is: > > * Clone full mirrors of all repos in gclient project to > > /some/path/to/cache_dir > > * Create 'local' clones from these mirror repos (i.e. don't clone any > objects, > > just do a checkout) > > > > When you commit, the new objects go into the local clone. When you fetch, > > gclient fetch'es the mirrors, and then does a pull in the local clone. > > > > The size of the local clone is just the size of the checkout without any > history > > (i.e. about half the size of a SVN checkout). > > When I do a commit in one of my working directories I want it to update my local > 'global' git database so I can see that object/branch in my other working > directories. > > I want the same functionality as git-new-workdir but for gclient. Thought > perhaps others would like the same... Ah got it. Cache dir would imply that you'd need to push your ref up to the mirror, which would be inconvenient. Nevermind me then :)
I know next to nothing to gclient cache dir, I'd recommend Robbie to do the review.
https://codereview.chromium.org/52663008/diff/1/gclient-new-workdir.py File gclient-new-workdir.py (right): https://codereview.chromium.org/52663008/diff/1/gclient-new-workdir.py#newcode15 gclient-new-workdir.py:15: if len(argv) < 3: also check if not enough parameters? Shouldn't it be len(argv) != 3? We should also assert that this script doesn't run on windows. https://codereview.chromium.org/52663008/diff/1/gclient-new-workdir.py#newcode28 gclient-new-workdir.py:28: new_workdir = argv[2] Let's move these above the checks, so we don't have to index argv 6 times. Also, all of the above should be in a function parse_options(argv) -> (repository, new_workdir) https://codereview.chromium.org/52663008/diff/1/gclient-new-workdir.py#newcode30 gclient-new-workdir.py:30: gclient = repository + os.sep + ".gclient" os.path.join(repository, '.gclient') https://codereview.chromium.org/52663008/diff/1/gclient-new-workdir.py#newcode68 gclient-new-workdir.py:68: os.system("git checkout -f") let's just use subprocess.check_call with the cwd parameter instead of chdir'ing
On 2013/11/04 21:40:29, iannucci wrote: Thanks for the review! Here is an updated patch with all issues addressed I believe. Cheers, Adam
https://codereview.chromium.org/52663008/diff/100001/gclient-new-workdir.py File gclient-new-workdir.py (right): https://codereview.chromium.org/52663008/diff/100001/gclient-new-workdir.py#n... gclient-new-workdir.py:15: def parse_options(argv): style nit: 2 lines between top-level statements https://codereview.chromium.org/52663008/diff/100001/gclient-new-workdir.py#n... gclient-new-workdir.py:18: return False, "", "" I think sys.exit(1) is clearer, instead of passing back an explicit 'success'. https://codereview.chromium.org/52663008/diff/100001/gclient-new-workdir.py#n... gclient-new-workdir.py:35: assert not sys.platform.startswith("win") nit: I would move this into parse_options, but up to you. https://codereview.chromium.org/52663008/diff/100001/gclient-new-workdir.py#n... gclient-new-workdir.py:55: if name == ".git": Why not just: if '.git' in dirs: # do stuff with '.git' ? https://codereview.chromium.org/52663008/diff/100001/gclient-new-workdir.py#n... gclient-new-workdir.py:57: make_workdir(os.path.join(root, name), os.path.join(workdir, name)) What happens if something breaks half-way through? Is there any appropriate cleanup we should do? https://codereview.chromium.org/52663008/diff/100001/gclient-new-workdir.py#n... gclient-new-workdir.py:67: os.makedirs(new_workdir + os.sep + "logs") os.path.join https://codereview.chromium.org/52663008/diff/100001/gclient-new-workdir.py#n... gclient-new-workdir.py:75: make_symlink(repository, new_workdir, "svn") I wonder if a blacklist would be more correct than this whitelist? Also, I would probably put all of these strings into an actual list, and then loop over them with make_symlink. https://codereview.chromium.org/52663008/diff/100001/gclient-new-workdir.py#n... gclient-new-workdir.py:76: shutil.copy2(repository + os.sep + "HEAD", new_workdir + os.sep + "HEAD") os.path.join https://codereview.chromium.org/52663008/diff/100001/gclient-new-workdir.py#n... gclient-new-workdir.py:85: print("usage: gclient-new-workdir.py <repository> <new_workdir>") Maybe since this is used in exactly 1 place, just put the print inline?
https://codereview.chromium.org/52663008/diff/100001/gclient-new-workdir.py File gclient-new-workdir.py (right): https://codereview.chromium.org/52663008/diff/100001/gclient-new-workdir.py#n... gclient-new-workdir.py:57: make_workdir(os.path.join(root, name), os.path.join(workdir, name)) On 2013/11/04 22:30:43, iannucci wrote: > What happens if something breaks half-way through? Is there any appropriate > cleanup we should do? We aren't touching the original workdir. The only cleanup to do would be to remove the new workdir, but the script should give an error and then the user can decide what to do with the new workdir. This is how git-new-workdir works at least. https://codereview.chromium.org/52663008/diff/100001/gclient-new-workdir.py#n... gclient-new-workdir.py:75: make_symlink(repository, new_workdir, "svn") On 2013/11/04 22:30:43, iannucci wrote: > I wonder if a blacklist would be more correct than this whitelist? > > Also, I would probably put all of these strings into an actual list, and then > loop over them with make_symlink. The whitelist was explicitly taken from git-new-workdir which is authored by the git folks. https://codereview.chromium.org/52663008/diff/100001/gclient-new-workdir.py#n... gclient-new-workdir.py:75: make_symlink(repository, new_workdir, "svn") On 2013/11/04 22:30:43, iannucci wrote: > I wonder if a blacklist would be more correct than this whitelist? > > Also, I would probably put all of these strings into an actual list, and then > loop over them with make_symlink. Hmm, I am not sure that a list would make it any more readable given the 80 char limit. I can do it if you insist though.
Here is a newer version addressing comments. Thanks!
https://codereview.chromium.org/52663008/diff/100001/gclient-new-workdir.py File gclient-new-workdir.py (right): https://codereview.chromium.org/52663008/diff/100001/gclient-new-workdir.py#n... gclient-new-workdir.py:75: make_symlink(repository, new_workdir, "svn") On 2013/11/04 22:51:35, atreat wrote: > On 2013/11/04 22:30:43, iannucci wrote: > > I wonder if a blacklist would be more correct than this whitelist? > > > > Also, I would probably put all of these strings into an actual list, and then > > loop over them with make_symlink. > > Hmm, I am not sure that a list would make it any more readable given the 80 char > limit. I can do it if you insist though. I'm thinking like: GIT_DIRECTORY_WHITELIST = ['config', 'refs', ...] ... for entry in GIT_DIRECTORY_WHITELIST: make_symlink(repository, new_workdir, entry) Just cuts out a lot of copy/paste repetition in this function
On 2013/11/04 22:54:50, iannucci wrote: > https://codereview.chromium.org/52663008/diff/100001/gclient-new-workdir.py > File gclient-new-workdir.py (right): > > https://codereview.chromium.org/52663008/diff/100001/gclient-new-workdir.py#n... > gclient-new-workdir.py:75: make_symlink(repository, new_workdir, "svn") > On 2013/11/04 22:51:35, atreat wrote: > > On 2013/11/04 22:30:43, iannucci wrote: > > > I wonder if a blacklist would be more correct than this whitelist? > > > > > > Also, I would probably put all of these strings into an actual list, and > then > > > loop over them with make_symlink. > > > > Hmm, I am not sure that a list would make it any more readable given the 80 > char > > limit. I can do it if you insist though. > > I'm thinking like: > > GIT_DIRECTORY_WHITELIST = ['config', 'refs', ...] > > > ... > > for entry in GIT_DIRECTORY_WHITELIST: > make_symlink(repository, new_workdir, entry) > > Just cuts out a lot of copy/paste repetition in this function Is it ok if GIT_DIRECTORY_WHITELIST goes over the eighty char limit per line?
On 2013/11/04 22:56:37, atreat wrote: > On 2013/11/04 22:54:50, iannucci wrote: > > https://codereview.chromium.org/52663008/diff/100001/gclient-new-workdir.py > > File gclient-new-workdir.py (right): > > > > > https://codereview.chromium.org/52663008/diff/100001/gclient-new-workdir.py#n... > > gclient-new-workdir.py:75: make_symlink(repository, new_workdir, "svn") > > On 2013/11/04 22:51:35, atreat wrote: > > > On 2013/11/04 22:30:43, iannucci wrote: > > > > I wonder if a blacklist would be more correct than this whitelist? > > > > > > > > Also, I would probably put all of these strings into an actual list, and > > then > > > > loop over them with make_symlink. > > > > > > Hmm, I am not sure that a list would make it any more readable given the 80 > > char > > > limit. I can do it if you insist though. > > > > I'm thinking like: > > > > GIT_DIRECTORY_WHITELIST = ['config', 'refs', ...] > > > > > > ... > > > > for entry in GIT_DIRECTORY_WHITELIST: > > make_symlink(repository, new_workdir, entry) > > > > Just cuts out a lot of copy/paste repetition in this function > > Is it ok if GIT_DIRECTORY_WHITELIST goes over the eighty char limit per line? Just do one line per entry GIT_DIRECTORY_WHITELIST = [ 'config', 'refs', 'thing', 'other', ..., ] Make sure the list is sorted too :)
On 2013/11/04 22:59:42, iannucci wrote: > Just do one line per entry > > GIT_DIRECTORY_WHITELIST = [ > 'config', > 'refs', > 'thing', > 'other', > ..., > ] > > Make sure the list is sorted too :) Done.
lgtm % 2nd comment https://codereview.chromium.org/52663008/diff/220001/gclient-new-workdir.py File gclient-new-workdir.py (right): https://codereview.chromium.org/52663008/diff/220001/gclient-new-workdir.py#n... gclient-new-workdir.py:56: os.path.join(workdir, ".git")) This loop is probably good for a V1, but it would be worth parallelizing it. Each workdir should be completely independent, as far as git is concerned. https://codereview.chromium.org/52663008/diff/220001/gclient-new-workdir.py#n... gclient-new-workdir.py:77: os.makedirs(os.path.join(new_workdir, "logs")) I would make this the responsibility of make_symlink. Do an os.makedirs of os.path.dirpath(link) or something along those lines.
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/adam.treat@samsung.com/52663008/240001
Presubmit check for 52663008-240001 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 (67 files) failed ************* Module gclient-new-workdir W0612: 52,18:main: Unused variable 'files' Presubmit checks took 87.9s to calculate.
On 2013/11/05 15:10:00, I haz the power (commit-bot) wrote: > ** Presubmit ERRORS ** > Pylint (67 files) failed > ************* Module gclient-new-workdir > W0612: 52,18:main: Unused variable 'files' > > > Presubmit checks took 87.9s to calculate. Anyone know how I can eliminate the unused variable in python from the os.walk?
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/adam.treat@samsung.com/52663008/290001
Message was sent while issue was closed.
Change committed as 232992 |
