Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1375553003:
go: Roll Goopfile, remove two no longer used packages. (Closed)
Descriptiongo: Roll Goopfile, remove two no longer used packages.
Also add a script that detects what packages can be removed, plus instructions
how to use it.
Removed packages:
github.com/bradfitz/http2 - have been merged into golang.org/x/net (yay!)
github.com/howeyc/fsnotify - have been replaced by gopkg.in/fsnotify.v0
R=nodir@chromium.org, tandrii@chromium.org
BUG=
Committed: https://chromium.googlesource.com/infra/infra/+/bee88f60dcfb792f873d2bffda7244d80f635f63
Patch Set 1 #
Total comments: 26
Patch Set 2 : #
Messages
Total messages: 20 (6 generated)
https://codereview.chromium.org/1375553003/diff/1/go/roll_goop.py File go/roll_goop.py (left): https://codereview.chromium.org/1375553003/diff/1/go/roll_goop.py#oldcode28 go/roll_goop.py:28: 'github.com/luci/gkvlite': 'refs/heads/32bitKeys', oh, it's no longer exceptions, Robbie fixed refs/heads/master of github.com/luci/gkvlite to point to same commit as refs/heads/32bitKeys.
The CQ bit was checked by vadimsh@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1375553003/1 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1375553003/1
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
lgtm % if you add caching and convince me otherwise. https://codereview.chromium.org/1375553003/diff/1/go/clean_goop.py File go/clean_goop.py (right): https://codereview.chromium.org/1375553003/diff/1/go/clean_goop.py#newcode35 go/clean_goop.py:35: def get_deps(pkg_glob): Add caching of the result, and it'll be so much faster :) https://codereview.chromium.org/1375553003/diff/1/go/clean_goop.py#newcode43 go/clean_goop.py:43: def get_all_goop(): and maybe cache this too? https://codereview.chromium.org/1375553003/diff/1/go/clean_goop.py#newcode46 go/clean_goop.py:46: goop = f.read() nit: return [l.split()[0] for l in f] https://codereview.chromium.org/1375553003/diff/1/go/clean_goop.py#newcode95 go/clean_goop.py:95: print 'Consider removing from Goopfile following packages:' nit: s/following/the following https://codereview.chromium.org/1375553003/diff/1/go/clean_goop.py#newcode98 go/clean_goop.py:98: print 'Hooray! All Goopfile packages seems to be in use.' s/seems/seem https://codereview.chromium.org/1375553003/diff/1/go/roll_goop.py File go/roll_goop.py (left): https://codereview.chromium.org/1375553003/diff/1/go/roll_goop.py#oldcode28 go/roll_goop.py:28: 'github.com/luci/gkvlite': 'refs/heads/32bitKeys', On 2015/09/28 22:27:48, Vadim Sh. wrote: > oh, it's no longer exceptions, Robbie fixed refs/heads/master of > github.com/luci/gkvlite to point to same commit as refs/heads/32bitKeys. Oh, great.
https://codereview.chromium.org/1375553003/diff/1/go/clean_goop.py File go/clean_goop.py (right): https://codereview.chromium.org/1375553003/diff/1/go/clean_goop.py#newcode23 go/clean_goop.py:23: GO_DIR = os.path.dirname(os.path.abspath(__file__)) You can check that $GOROOT equals this and suggest the user to run this in the activated go env https://codereview.chromium.org/1375553003/diff/1/go/clean_goop.py#newcode37 go/clean_goop.py:37: cmd = ['go', 'list', '-f', '{{ join .Deps "\\n" }}', pkg_glob] this must be ran with GO_DIR as current dir, because you pass a rel path in pkg_glob alternatively, just run GOPATH=$GOROOT go list all when want to ignore vendored packages. (I think it is simpler) https://codereview.chromium.org/1375553003/diff/1/go/clean_goop.py#newcode45 go/clean_goop.py:45: with open(os.path.join(GO_DIR, 'Goopfile.lock'), 'rt') as f: 't' mode is unnecessary to specify since it is default https://codereview.chromium.org/1375553003/diff/1/go/clean_goop.py#newcode56 go/clean_goop.py:56: if dep.startswith(pkg): if dep == pkg or dep.startswith(pkg+'/'): https://codereview.chromium.org/1375553003/diff/1/go/clean_goop.py#newcode66 go/clean_goop.py:66: # packages in GOPAHT, since we don't want to enumerate vendored packages). GOPATH https://codereview.chromium.org/1375553003/diff/1/go/clean_goop.py#newcode69 go/clean_goop.py:69: go_dir_rel = os.path.relpath(os.path.join(GO_DIR)) os.path.join(GO_DIR) == GO_DIR ? Am I missing something? https://codereview.chromium.org/1375553003/diff/1/go/clean_goop.py#newcode84 go/clean_goop.py:84: for pkg in goop_deps: goop_deps is set() in the first iteration. Then it is modified only in line 89. Maybe move this loop to line 90 to make more readable?
also rerolled deps, they have already changed https://codereview.chromium.org/1375553003/diff/1/go/clean_goop.py File go/clean_goop.py (right): https://codereview.chromium.org/1375553003/diff/1/go/clean_goop.py#newcode23 go/clean_goop.py:23: GO_DIR = os.path.dirname(os.path.abspath(__file__)) On 2015/09/29 16:46:05, nodir wrote: > You can check that $GOROOT equals this and suggest the user to run this in the > activated go env GOROOT points to goland SDK. GOPATH have multiple entries. It is even more complicated in infra_internal. I prefer not to bother. I don't present this script to be overly user friendly. https://codereview.chromium.org/1375553003/diff/1/go/clean_goop.py#newcode35 go/clean_goop.py:35: def get_deps(pkg_glob): On 2015/09/29 10:17:27, tandrii(chromium) wrote: > Add caching of the result, and it'll be so much faster :) Added cache, it doesn't make much difference. https://codereview.chromium.org/1375553003/diff/1/go/clean_goop.py#newcode37 go/clean_goop.py:37: cmd = ['go', 'list', '-f', '{{ join .Deps "\\n" }}', pkg_glob] On 2015/09/29 16:46:05, nodir wrote: > this must be ran with GO_DIR as current dir, because you pass a rel path in > pkg_glob > relpath(...) below builds correct path regardless of current directory. It can become incorrect only if roll_goop.py scrip itself changed cwd, and it doesn't. > alternatively, just run > > GOPATH=$GOROOT go list all > > when want to ignore vendored packages. (I think it is simpler) GOROOT is goland SDK, not src/... https://codereview.chromium.org/1375553003/diff/1/go/clean_goop.py#newcode43 go/clean_goop.py:43: def get_all_goop(): On 2015/09/29 10:17:27, tandrii(chromium) wrote: > and maybe cache this too? Done. https://codereview.chromium.org/1375553003/diff/1/go/clean_goop.py#newcode45 go/clean_goop.py:45: with open(os.path.join(GO_DIR, 'Goopfile.lock'), 'rt') as f: On 2015/09/29 16:46:05, nodir wrote: > 't' mode is unnecessary to specify since it is default r is too for that matter https://codereview.chromium.org/1375553003/diff/1/go/clean_goop.py#newcode46 go/clean_goop.py:46: goop = f.read() On 2015/09/29 10:17:27, tandrii(chromium) wrote: > nit: return [l.split()[0] for l in f] Done. https://codereview.chromium.org/1375553003/diff/1/go/clean_goop.py#newcode56 go/clean_goop.py:56: if dep.startswith(pkg): On 2015/09/29 16:46:05, nodir wrote: > if dep == pkg or dep.startswith(pkg+'/'): Done. https://codereview.chromium.org/1375553003/diff/1/go/clean_goop.py#newcode69 go/clean_goop.py:69: go_dir_rel = os.path.relpath(os.path.join(GO_DIR)) On 2015/09/29 16:46:05, nodir wrote: > os.path.join(GO_DIR) == GO_DIR ? Am I missing something? There used to be something else there. https://codereview.chromium.org/1375553003/diff/1/go/clean_goop.py#newcode84 go/clean_goop.py:84: for pkg in goop_deps: On 2015/09/29 16:46:05, nodir wrote: > goop_deps is set() in the first iteration. Then it is modified only in line 89. > Maybe move this loop to line 90 to make more readable? Done. https://codereview.chromium.org/1375553003/diff/1/go/clean_goop.py#newcode95 go/clean_goop.py:95: print 'Consider removing from Goopfile following packages:' On 2015/09/29 10:17:27, tandrii(chromium) wrote: > nit: s/following/the following Done. https://codereview.chromium.org/1375553003/diff/1/go/clean_goop.py#newcode98 go/clean_goop.py:98: print 'Hooray! All Goopfile packages seems to be in use.' On 2015/09/29 10:17:28, tandrii(chromium) wrote: > s/seems/seem Done.
The CQ bit was checked by vadimsh@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1375553003/20001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1375553003/20001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: Infra Win Tester on tryserver.infra (JOB_FAILED, https://build.chromium.org/p/tryserver.infra/builders/Infra%20Win%20Tester/bu...)
The CQ bit was checked by vadimsh@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from tandrii@chromium.org Link to the patchset: https://codereview.chromium.org/1375553003/#ps20001 (title: " ")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1375553003/20001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1375553003/20001
Message was sent while issue was closed.
Committed patchset #2 (id:20001) as https://chromium.googlesource.com/infra/infra/+/bee88f60dcfb792f873d2bffda724...
Message was sent while issue was closed.
https://codereview.chromium.org/1375553003/diff/1/go/clean_goop.py File go/clean_goop.py (right): https://codereview.chromium.org/1375553003/diff/1/go/clean_goop.py#newcode37 go/clean_goop.py:37: cmd = ['go', 'list', '-f', '{{ join .Deps "\\n" }}', pkg_glob] On 2015/09/29 19:49:34, Vadim Sh. wrote: > On 2015/09/29 16:46:05, nodir wrote: > > this must be ran with GO_DIR as current dir, because you pass a rel path in > > pkg_glob > > > relpath(...) below builds correct path regardless of current directory. It can > become incorrect only if roll_goop.py scrip itself changed cwd, and it doesn't. > > > alternatively, just run > > > > GOPATH=$GOROOT go list all > > > > when want to ignore vendored packages. (I think it is simpler) > > GOROOT is goland SDK, not src/... I just ran go/clean_goop.py from root and it errors out: Traceback (most recent call last): File "go/clean_goop.py", line 116, in <module> sys.exit(main()) File "go/clean_goop.py", line 82, in main assert go_dir_rel.startswith('.') AssertionError because go_dir_rel == `go`. I mentioned cwd because go.relpath with one parameter ultimately depends on cwd
Message was sent while issue was closed.
On 2015/09/29 20:39:13, nodir wrote: > https://codereview.chromium.org/1375553003/diff/1/go/clean_goop.py > File go/clean_goop.py (right): > > https://codereview.chromium.org/1375553003/diff/1/go/clean_goop.py#newcode37 > go/clean_goop.py:37: cmd = ['go', 'list', '-f', '{{ join .Deps "\\n" }}', > pkg_glob] > On 2015/09/29 19:49:34, Vadim Sh. wrote: > > On 2015/09/29 16:46:05, nodir wrote: > > > this must be ran with GO_DIR as current dir, because you pass a rel path in > > > pkg_glob > > > > > relpath(...) below builds correct path regardless of current directory. It can > > become incorrect only if roll_goop.py scrip itself changed cwd, and it > doesn't. > > > > > alternatively, just run > > > > > > GOPATH=$GOROOT go list all > > > > > > when want to ignore vendored packages. (I think it is simpler) > > > > GOROOT is goland SDK, not src/... > > I just ran go/clean_goop.py from root and it errors out: > > Traceback (most recent call last): > File "go/clean_goop.py", line 116, in <module> > sys.exit(main()) > File "go/clean_goop.py", line 82, in main > assert go_dir_rel.startswith('.') > AssertionError > > because go_dir_rel == `go`. > > I mentioned cwd because go.relpath with one parameter ultimately depends on cwd I meant os.path.relpath
Message was sent while issue was closed.
On 2015/09/29 20:39:44, nodir wrote: > On 2015/09/29 20:39:13, nodir wrote: > > https://codereview.chromium.org/1375553003/diff/1/go/clean_goop.py > > File go/clean_goop.py (right): > > > > https://codereview.chromium.org/1375553003/diff/1/go/clean_goop.py#newcode37 > > go/clean_goop.py:37: cmd = ['go', 'list', '-f', '{{ join .Deps "\\n" }}', > > pkg_glob] > > On 2015/09/29 19:49:34, Vadim Sh. wrote: > > > On 2015/09/29 16:46:05, nodir wrote: > > > > this must be ran with GO_DIR as current dir, because you pass a rel path > in > > > > pkg_glob > > > > > > > relpath(...) below builds correct path regardless of current directory. It > can > > > become incorrect only if roll_goop.py scrip itself changed cwd, and it > > doesn't. > > > > > > > alternatively, just run > > > > > > > > GOPATH=$GOROOT go list all > > > > > > > > when want to ignore vendored packages. (I think it is simpler) > > > > > > GOROOT is goland SDK, not src/... Right, I meant GO_DIR > > > > I just ran go/clean_goop.py from root and it errors out: > > > > Traceback (most recent call last): > > File "go/clean_goop.py", line 116, in <module> > > sys.exit(main()) > > File "go/clean_goop.py", line 82, in main > > assert go_dir_rel.startswith('.') > > AssertionError > > > > because go_dir_rel == `go`. > > > > I mentioned cwd because go.relpath with one parameter ultimately depends on > cwd > > I meant os.path.relpath |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
