Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

proposal: cmd/go: allow 'go get -u' to upgrade the targets of replacements #32721

Open
dfang opened this issue Jun 21, 2019 · 12 comments
Open

proposal: cmd/go: allow 'go get -u' to upgrade the targets of replacements #32721

dfang opened this issue Jun 21, 2019 · 12 comments

Comments

@dfang
Copy link

@dfang dfang commented Jun 21, 2019

What version of Go are you using (go version)?

$ go version
go version go1.12.5 darwin/amd64

Does this issue reproduce with the latest release?

yes

What operating system and processor architecture are you using (go env)?

go env Output
$ go env
go env
GOARCH="amd64"
GOBIN=""
GOCACHE="/Users/mj/Library/Caches/go-build"
GOEXE=""
GOFLAGS=""
GOHOSTARCH="amd64"
GOHOSTOS="darwin"
GOOS="darwin"
GOPATH="/Users/mj/go"
GOPROXY=""
GORACE=""
GOROOT="/usr/local/Cellar/go/1.12.5/libexec"
GOTMPDIR=""
GOTOOLDIR="/usr/local/Cellar/go/1.12.5/libexec/pkg/tool/darwin_amd64"
GCCGO="gccgo"
CC="clang"
CXX="clang++"
CGO_ENABLED="1"
GOMOD=""
CGO_CFLAGS="-g -O2"
CGO_CPPFLAGS=""
CGO_CXXFLAGS="-g -O2"
CGO_FFLAGS="-g -O2"
CGO_LDFLAGS="-g -O2"
PKG_CONFIG="pkg-config"
GOGCCFLAGS="-fPIC -m64 -pthread -fno-caret-diagnostics -Qunused-arguments -fmessage-length=0 -fdebug-prefix-map=/var/folders/xb/dsd0_2b92x7bsl92xlzj6fbm0000gn/T/go-build803671561=/tmp/go-build -gno-record-gcc-switches -fno-common"

What did you do?

go get -u github.com/dfang/auth

What did you expect to see?

replace github.com/qor/auth => github.com/dfang/auth v0.0.0-20190621054412-96c274f7c597 

just update/replace the hash with updated one in go.mod

What did you see instead?

the hash didn't change, but add one dependency on

require(
    ....
   github.com/dfang/auth v-0.0.0-<hash>
   .....
)

for now i have to copy hash in require, delete the line, update the hash in replace

@bcmills

This comment has been minimized.

Copy link
Member

@bcmills bcmills commented Jun 21, 2019

What did you do?

go get -u github.com/dfang/auth

That's not enough information for us to reproduce this error, let alone to verify a fix for it. Please provide complete steps to reproduce the problem.

@dfang

This comment has been minimized.

Copy link
Author

@dfang dfang commented Jun 23, 2019

@bcmills ok, simple files

main.go

package main

import (
	"fmt"
	"test/faker"
)

func main() {
	fmt.Println(faker.App().Version())
}

faker/app.go

package faker

type FakeApp interface {
	Version() string // => "2.6.0"
}

type fakeApp struct{}

func App() FakeApp {
	return fakeApp{}
}

func (a fakeApp) Version() string {
	return "0.1"
}

go.mod

module github.com/user/test

go 1.12

replace github.com/user/test/faker => syreclabs.com/go/faker v1.0.0

just create these files, then run go mod tidy, now it's ready.

if syreclabs.com/go/faker updates, i want to keep up, i will run go get -u syreclabs.com/go/faker@v1.1.0

now go.mod became

module github.com/user/test

go 1.12

replace github.com/user/test/faker => syreclabs.com/go/faker v1.0.0

require syreclabs.com/go/faker v1.1.0 // indirect

what i expected is

go.mod

module github.com/user/test

go 1.12

replace github.com/user/test/faker => syreclabs.com/go/faker v1.1.0

let me know if there is anything unclear ....

thanks

@dfang dfang changed the title go get -u fails to update the hash of the module by go mod --replace go get -u fails to update the hash/version of the module by go mod --replace Jun 23, 2019
@bcmills bcmills removed the WaitingForInfo label Jun 27, 2019
@bcmills bcmills changed the title go get -u fails to update the hash/version of the module by go mod --replace cmd/go: allow 'go get -u' to upgrade the targets of replacements Jun 27, 2019
@bcmills

This comment has been minimized.

Copy link
Member

@bcmills bcmills commented Jun 27, 2019

This seems closely related to #28176, in that both want to track updates in replacements.

But note that the argument to go get -u is in general a package import path, and the target of a replace directive is not importable (see #26904). The replace directive today replaces source code, not import paths.

If/when we go support path replacements (as in #26904) instead of source code replacements, then you could do something like:

module github.com/user/test

go 1.14

replace github.com/user/test/faker => syreclabs.com/go/faker

require (
	github.com/user/test/faker v0.0.0-00000101000000-000000000000
	syrelabs.com/go/faker v1.0.0
)

And then go get -u would do, I suspect, exactly what you want.

@bcmills bcmills added this to the Unplanned milestone Jun 27, 2019
@dfang

This comment has been minimized.

Copy link
Author

@dfang dfang commented Jun 28, 2019

This is a feature request or proposal, i'll change the title first ...

@dfang dfang changed the title cmd/go: allow 'go get -u' to upgrade the targets of replacements proposal: cmd/go: allow 'go get -u' to upgrade the targets of replacements Jun 28, 2019
@rsc

This comment has been minimized.

Copy link
Contributor

@rsc rsc commented Jul 2, 2019

I think it would be a mistake to do this.
Replace is the developer's all-purpose override mechanism, opting out of all the usual logic.
It should remain so.
If we make "go get -u" apply to replace right-hand-sides, then we will need "go get -u -no-replace" as well.

@dfang

This comment has been minimized.

Copy link
Author

@dfang dfang commented Jul 3, 2019

@rsc if it is a mistake to do go mod replacement. why go mod edit --replace exists ?

there are scenarios when you want to update right-hand-sides.

eg. a real example from me:

i'm forking this repo to do my work, this relies on qor/auth, but there is bug on qor/auth. i forked it. and use go mod edit --replace to redirect to forked one. everytime i update the forked qor/auth, i have to go get and the edit go.mod.
it's not so much work if there is only qor/auth to update, unfortunately i've added go mod support to all dependencies, so i have to edit go.mod on every update.

btw, forks will not merged because no active maintaining on this repo.

i heard google use a monorepo so this feature may not be very useful. it was hard for forking go code and pull request because of the path, now it's better thanks to go mod edit --replace, why not make it better ?

I think it would be a mistake to do this.
Replace is the developer's all-purpose override mechanism, opting out of all the usual logic.
It should remain so.
If we make "go get -u" apply to replace right-hand-sides, then we will need "go get -u -no-replace" as well.

no-replace as default behavior, just add --replace flag ?

if you want to the right-hand-sides, just go get -u --replace replacement/forked/whatever

@dmitris

This comment has been minimized.

Copy link
Contributor

@dmitris dmitris commented Sep 11, 2019

replace github.com/user/test/faker => syreclabs.com/go/faker v1.0.0

require syreclabs.com/go/faker v1.1.0 // indirect

FWIW, we had to write a custom tool (modfix) exactly to address this issue - the replacement version getting out of sync with require ones all the time on updates with go get <module>, leading to a rather nasty situation when developers think they use the new version because they ran go get <module> and it's in the require stanza - while in reality they still use the old one because of the "sticky" version in the replace part that got out of sync. (If there's interest, I could try to publish the parts of the modfix that are not internal specific. We use the replacements like replace github.com/BurntSushi/toml v0.3.1 => git.ourcompany.com/mirror-github/BurntSushi--toml v0.3.1 for all the third-party / opensource modules.)

It is still far from perfect since now developers have to remember to run modfix every time when they update any dependencies (changes maybe in the transitive parts of the dependency graph), but still it's better than having to do it manually...

@rsc

This comment has been minimized.

Copy link
Contributor

@rsc rsc commented Sep 12, 2019

/cc (&ping) @bcmills @jayconrod

@bcmills

This comment has been minimized.

Copy link
Member

@bcmills bcmills commented Sep 12, 2019

I think we should see where we land with #26904, and possibly revisit this proposal at that point.

(We need a more solid foundation for replace before we consider building fancy things on top of it.)

@josharian

This comment has been minimized.

Copy link
Contributor

@josharian josharian commented Sep 14, 2019

I just hit this as well.

Replace is the developer's all-purpose override mechanism, opting out of all the usual logic. It should remain so.

I'm not so sure. Experimenting with and then maintaining ongoing private patches atop public repos is commonplace. (This is for a variety of reasons: abandoned repos, slow upstream review times, slow upstream release cycles, patch is not appropriate to upstream.) Thus using replace is also commonplace. It seems that "opting out of all the usual logic" means in practice "lacking tooling to accomplish basic tasks". And lacking tooling to accomplish basic tasks in commonplace usage is not ideal.

It's worth saying: Maybe I'm not holding it right. I'm honestly still really struggling to figure out how to make modules play nicely with all the different stages of hacking on public code. Maybe #33848 would change things. I'm not sure.

@bcmills

This comment has been minimized.

Copy link
Member

@bcmills bcmills commented Sep 16, 2019

@josharian, I would generally expect a long-running, actively-maintained fork to have its own import path and its own separate versioning.

For example, if your fork and the upstream module both start out at v1.0.0, you may need to issue v1.0.1 of your fork in order to fix a bug introduced within the patch itself. Then if upstream fixes a bug in their own code, they'll presumably release a v1.0.1 of their own, and your fork will have to pick that up as v1.0.2 (because v1.0.1 is already taken).

Since the versions can't necessarily remain in lock-step anyway, #26904 seems like a more appropriate solution.

@dfang

This comment has been minimized.

Copy link
Author

@dfang dfang commented Sep 17, 2019

@bcmills like @josharian said there are many reasons we may want to use replacement feature.

you mean the appropriate solution is "rewrite the import path" ?

@andybons andybons added the Proposal label Oct 6, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked pull requests

Successfully merging a pull request may close this issue.

None yet
6 participants
You can’t perform that action at this time.