search

tools / godep

dependency tool for go
http://godoc.org/github.com/tools/godep
BSD 3-Clause "New" or "Revised" License
5.54k stars 454 forks source link

rewrite branch issues #81

Closed maddyblue closed 10 years ago

maddyblue commented 10 years ago

Using github.com/StackExchange/scollector as a test program, I ran godep save -r in the scollector directory.

  1. The program is already gofmt'd, so I expect no style changes. However, there's a two line whitespace change around line 115 that appears to be incorrect.
  2. The standard library import paths are being rewritten without corresponding packages being copied. I assume that they shouldn't be rewritten at all, so the lack of copying isn't a real problem.
  3. When run on mac, it misses the _windows.go files, and thus misses one of the includes (code.google.com/p/winsvc is imported in service_windows.go).
  4. It is not run on any subdirectories of scollector. If that is the default behavior, then we may just disagree or I don't understand something important.
kr commented 10 years ago

Thanks! This is what I get for rushing a patch out for testing before getting on an airplane. :sweat_smile:

kr commented 10 years ago

I had some trouble installing scollector, but I was able to reproduce what you describe with github.com/heroku/hk.

  1. There are incorrect whitespace changes. Maybe this has something to do with how I'm using package go/printer.
  2. Rewriting import paths that aren't part of the dependency manifest was a bug. I've pushed an updated commit to the branch and now hk with rewriting compiles cleanly for me.
  3. I'll look into the build constraints. Does it miss out putting the dependency in the list, or just rewriting the path?
  4. To get packages in subdirectories, use godep save -r ./.... Godep follows the convention established by the go tool: for commands that take a list of packages, no argument means the same as just dot (.).
kr commented 10 years ago

The whitespace formatting problem should be fixed now. Can you confirm @mjibson?

I'll work on the build tags next.

maddyblue commented 10 years ago

Is the updated code pushed to the branch? I'm not seeing it.

kr commented 10 years ago

Yes, it's pushed. Github doesn't do such a hot job of indicating revisions to a patch. You can see the newest code on https://github.com/tools/godep/pull/82/files if you search for "Config".

kr commented 10 years ago

FYI the commit id is 550fdb6a9c9a78bf17c6771a17ceadd88d39e1cb.

maddyblue commented 10 years ago

Yes, everything is fixed except for the third point. From a linux machine, running godep save -r ./... in the scollector directory ignores all *_windows.go files. They are not rewritten or added to Godeps.json.

kr commented 10 years ago

Ok, then the build tag problem is a separate bug. It would happen even if you didn't use -r. I'll open an issue for that and track it separately from the rewriting pull request. Probably fix it after merging this.

kr commented 10 years ago

Opened #84 to track build constraints.

With that, I think this issue is resolved and I'm almost ready to merge #82.

kr commented 10 years ago

Thanks for reporting all these issues!