golang / go

The Go programming language
https://go.dev
BSD 3-Clause "New" or "Revised" License
122.88k stars 17.52k forks source link

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

Open dfang opened 5 years ago

dfang commented 5 years ago

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 commented 5 years ago

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 commented 5 years ago

@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

bcmills commented 5 years ago

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.

dfang commented 5 years ago

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

rsc commented 5 years ago

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 commented 5 years ago

@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 commented 5 years ago

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 commented 5 years ago

/cc (&ping) @bcmills @jayconrod

bcmills commented 5 years ago

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 commented 5 years ago

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 commented 5 years ago

@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 commented 5 years ago

@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" ?

ianlancetaylor commented 3 years ago

The last comment was over a year ago, and things have changed. What is the status of this idea? Is this still relevant? Thanks.

bcmills commented 3 years ago

I still think that #26904 is the appropriate solution for this use-case, and I'm hoping to get to in this cycle (it's been over a year, but a long one 😅).

josharian commented 3 years ago

From a user’s point of view, this is definitely still relevant and an ongoing source of frustration. Not two weeks ago I wrote a script that calls uses dropreplace, calls go get -u, and restores the replace directive.