constabulary / gb

gb, the project based build tool for Go

https://getgb.io/

MIT License

2.15k stars 150 forks source link

vendor: respect repository field in restore and update #730

Open cryptix opened 6 years ago

cryptix commented 6 years ago

Somehow the repository field from the manifest isn't used.

With this I can replace packages with forks.

davecheney commented 6 years ago

I deeply regret adding restore to gb vendor, thanks for fixing this, I’ll take a look soon.

On 11 Jan 2018, at 05:45, Henry notifications@github.com wrote:

Somehow the repository field from the manifest isn't used.

With this I can replace packages with forks.

You can view, comment on, or merge this pull request online at:

https://github.com/constabulary/gb/pull/730

Commit Summary

respect repository field in restore and update File Changes

M cmd/gb-vendor/restore.go (8) M cmd/gb-vendor/update.go (2) Patch Links:

https://github.com/constabulary/gb/pull/730.patch https://github.com/constabulary/gb/pull/730.diff — You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub, or mute the thread.

cryptix commented 6 years ago

I deeply regret adding restore to gb vendor, ...

oh why is that?

I might not be best style but I tend to keep small patches inside my tree until I move them to PRs/external forks. It reduces the overhead of dealing with a lot more repos for me and the restore command is pretty helpful to get back the upstream state.

davecheney commented 6 years ago

It has been the biggest source of bugs, especially when people became so reliant on it that parallel downloading of artifacts had to be implemented.

On 11 Jan 2018, at 19:02, Henry notifications@github.com wrote:

I deeply regret adding restore to gb vendor.

oh why is that?

I might not be best style but I tend to keep small patches inside my tree until I move them to PRs/external forks. It reduces the overhead of dealing with a lot more repos for me and the restore command is pretty helpful to get back the upstream state.

— You are receiving this because you commented. Reply to this email directly, view it on GitHub, or mute the thread.

cryptix commented 5 years ago

~~I just noticed an issue with my approach. Consider these two entries in manifest, they retrieve two packages from the same repository.~~

~~Since I just replaced importPath with repository, it now clones the root of the repositories into the two improtpath locations, leading to a broken vendor state.~~

Disregard the previous comment. Somehow my vendor/manifest didn't have "path": "..." entries to build the correct copy.

codecov-io commented 5 years ago

Codecov Report

Merging #730 into master will increase coverage by 0.18%. The diff coverage is n/a.

@@            Coverage Diff            @@
##           master    #730      +/-   ##
=========================================
+ Coverage   50.52%   50.7%   +0.18%     
=========================================
  Files          42      41       -1     
  Lines        3119    3102      -17     
=========================================
- Hits         1576    1573       -3     
+ Misses       1405    1386      -19     
- Partials      138     143       +5

Impacted Files	Coverage Δ
build.go
cmd/gb/build.go	`71.34% <0%> (+66.47%)`	:arrow_up:

Continue to review full report at Codecov.

Legend - Click here to learn more Δ = absolute <relative> (impact), ø = not affected, ? = missing data Powered by Codecov. Last update f11784c...0153a33. Read the comment docs.

cryptix commented 5 years ago

Testing this some more, I noticed that I broke vanity imports.

I pushed a really dirty hack in 0153a33 to make golang.org/x/... work again but it's not good since it only works for go.googlesource.com/....