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

x/vgo: should we allow replace for "old" code? #25739

Closed
myitcv opened this issue Jun 5, 2018 · 9 comments
Closed

x/vgo: should we allow replace for "old" code? #25739

myitcv opened this issue Jun 5, 2018 · 9 comments
Milestone

Comments

@myitcv
Copy link
Member

myitcv commented Jun 5, 2018

Please answer these questions before submitting your issue. Thanks!

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

go version go1.10.2 linux/amd64 vgo:2018-02-20.1
vgo commit: 416f375193bdc882469e470452be5d856646df76

Does this issue reproduce with the latest release?

Yes. Per above

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

GOARCH="amd64"
GOBIN=""
GOCACHE="/home/myitcv/.cache/go-build"
GOEXE=""
GOHOSTARCH="amd64"
GOHOSTOS="linux"
GOOS="linux"
GOPATH="/home/myitcv/vgo-by-example/.gopath"
GOPROXY=""
GORACE=""
GOROOT="/home/myitcv/gos"
GOTMPDIR=""
GOTOOLDIR="/home/myitcv/gos/pkg/tool/linux_amd64"
GCCGO="gccgo"
CC="gcc"
CXX="g++"
CGO_ENABLED="1"
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 -fmessage-length=0 -fdebug-prefix-map=/tmp/go-build361133570=/tmp/go-build -gno-record-gcc-switches"
VGOMODROOT=""

What did you do?

Create ourselves a simple module that depends on an "old" Go package (rsc.io/pdf at v0.1.1 is
"old", non-module code):

$ mkdir hello
$ cd hello
$ cat <<EOD >hello.go
package main // import "example.com/hello"

import (
        "fmt"

        "rsc.io/pdf"
)

func main() {
        fmt.Println(pdf.Point{})
}
EOD
$ echo >go.mod

Now we get that specific version of rsc.io/pdf that is known to be "old" Go code:

$ vgo get rsc.io/pdf@v0.1.1
vgo: finding rsc.io/pdf v0.1.1
vgo: downloading rsc.io/pdf v0.1.1
$ cat go.mod
module example.com/hello

require rsc.io/pdf v0.1.1

Now check our code builds and runs:

$ vgo build
$ ./hello
{0 0}

Create a local copy of rsc.io/pdf and add a replace directive to use it:

$ git clone https://github.com/rsc/pdf pdf
Cloning into 'pdf'...
$ cd pdf
$ git checkout v0.1.1
HEAD is now at 48d0402... pdf: add a Go import comment
$ cd ..
$ cat <<EOD >>go.mod
replace rsc.io/pdf v0.1.1 => ./pdf
EOD
$ cat go.mod
module example.com/hello

require rsc.io/pdf v0.1.1
replace rsc.io/pdf v0.1.1 => ./pdf

Now check our code still builds:

$ vgo build
vgo: open /go/hello/pdf/go.mod: no such file or directory

It doesn't; seems we need to mark that directory as a Go module with a go.mod:

$ cd pdf
$ cat <<EOD >go.mod
module rsc.io/pdf
EOD
$ cd ..

Now check our code builds and runs:

$ vgo build
$ ./hello
{0 0}

What did you expect to see?

It's unclear whether we can/want to allow replace directives to apply to "old" (where "old" is the opposite of "new"). Doing so would obviate the need for creating the go.mod file in the pdf directory; but I'm unclear what other "problems" this might introduce.

What did you see instead?

Per above.

@gopherbot gopherbot added this to the vgo milestone Jun 5, 2018
@kardianos
Copy link
Contributor

kardianos commented Jun 5, 2018

Doing so would obviate the need for creating the go.mod file in the pdf directory; but I'm unclear what other "problems" this might introduce.

Each package needs to be able to compile on its own. To do that it needs to know that the GOPATH looks like. Aside from a possible technical issue to overcome, mixing code without a module file (and thus without a understanding of where it is in GOPATH) and "new" code with a mod file seems like it would increase confusion. Probably replace rsc.io/pdf v0.1.1 => ./pdf would be able to made to work, but replace rsc.io/pdf v0.1.1 => ../pdf (parent directory) would fail when compiling the package.

I suspect this would be a bad idea.

As a counter proposal, maybe make it possible to reference "old code" by referencing the GOPATH like:
replace rsc.io/pdf v0.1.1 => $GOPATH/rsc.io/pdf .

edit: I'm not advancing the requirement of referencing old code, but if this does turn out to be an issue, I think the GOPATH ref might be a good alternative.

@bcmills
Copy link
Contributor

bcmills commented Jun 5, 2018

I'm curious: do you have the same problem if you use vgo get instead of git clone to create the local copy?

I could see a reasonable argument for vgo get writing a go.mod automatically on the way down, although that has the somewhat unpleasant side effect of making the downloaded code not a pristine copy of the upstream source.

@myitcv
Copy link
Member Author

myitcv commented Jun 5, 2018

Each package needs to be able to compile on its own.

This was my initial feeling. But given Russ' comments in #25662 (comment) I didn't know whether adding go.mod files to what really is "old" code breaks other invariants. After all, the code referenced in $GOPATH/src/v/... does not have a go.mod file.

As a counter proposal

Just to be clear, this is definitely a question, not a proposal. I'm just trying to understand how this "flow" is supported (unless others feel the use case is more an edge case?)

maybe make it possible to reference "old code" by referencing the GOPATH like:
replace rsc.io/pdf v0.1.1 => $GOPATH/rsc.io/pdf .

Interesting idea.

The reason for containing pdf within my module is as follows: I want it to be committed as part of the module code.

The alternative would of course be for me to fork the repo, but then I think we're caught short again here. Because imagine I fork the repo to github.com/myitcv/pdf, then I would want to be able to run a vgo get command to say (in effect) vgo get rsc.io/pdf from github.com/myitcv/pdf. This might well be something we also want to support, but I think it ultimately boils down to the same problem.

@myitcv
Copy link
Member Author

myitcv commented Jun 5, 2018

@bcmills

I'm curious: do you have the same problem if you use vgo get instead of git clone to create the local copy?

I'm probably missing something obvious here, but would vgo get not place the code in $GOPATH/src/v/... as opposed to the local directory?

@bcmills
Copy link
Contributor

bcmills commented Jun 5, 2018

It's more likely that I'm the one missing something obvious. 🙂

@kardianos
Copy link
Contributor

The reason for containing pdf within my module is as follows: I want it to be committed as part of the module code.

I think if you are committing the code to your repo, then you might as well put in an internal folder and git it a go.mod file

@rsc
Copy link
Contributor

rsc commented Jun 7, 2018

The replacement line must point at a module. If you are preparing a module in a local file system directory, then part of preparing it is having a go.mod. In the future it will be common for your clone of someone's repo to bring in a go.mod. Until then, you need to make it. It is important to know what the replacement module believes its import path base is (the module path) as well as what its requirements are (which may differ from the original, and which will not be consulted anyway). For both these reasons the go.mod must be present and cannot simply be inferred.

Now, it's true that if you said to replace some other thing => rsc.io/pdf v0.1.1 then that would work even though the code in github.com/rsc/pdf at tag v0.1.1 has no go.mod file. But that's because the combination "rsc.io/pdf v0.1.1" is going through a resolution process that does in fact construct a synthetic go.mod file as best it can from the sources. But a plain directory full of files does not carry the same context.

Re invariants, it is fine to put a module line that says what the code in that directory believes that directory's import path to be. That's what vgo get is doing on download.

As for the auto-generated go.mod not being in the $GOPATH/src/v/rsc.io/pdf@v0.1.1 directory, that's true, it's not there. It's in $GOPATH/src/v/cache/rsc.io/pdf/@v/v0.1.1.mod instead. The unpacked source directory is a pristine copy of exactly what is in the zip file ($GOPATH/src/v/cache/rsc.io/pdf/@v/v0.1.1.zip), so that we can compute its hash and expect that hash to be never-changing. In contrast, the details of auto-generating a go.mod from a file tree do change, as we fix bugs and make the conversion more capable. If go.mod lived in that source directory, the hash would change as vgo did, which would not break go.modverify (soon to be go.sum) verification. Instead the mod is kept in the cache directory, alongside the zip file. You'll also notice that it has a comment at the top with a version number:

$ cat v0.1.1.mod
//vgo 0.0.4

module rsc.io/pdf
$ 

That version number indicates the version of the converter used to generate the mod file. (Really a made-up number, it didn't need to look like semver.) Each time we make a substantial change to the converter and need to invalidate old conversions, we can bump the number in the vgo source code, and it will treat all those cached files as stale and regenerate them from the original source trees.

Bringing things back to the ../pdf directory, it should suffice to do

 echo rsc.io/pdf >go.mod
 vgo list ./...

which will page in whatever rsc.io/pdf needs (nothing, it turns out). I've been wondering about some kind of 'vgo initmod rsc.io/pdf' command but I'm relucant to put something there that will be under significant pressure to grow in scope without bound.

@rsc
Copy link
Contributor

rsc commented Jun 7, 2018

I think this answers the question in the issue title at least; closing.

@rsc rsc closed this as completed Jun 7, 2018
@myitcv
Copy link
Member Author

myitcv commented Jun 7, 2018

I think this answers the question in the issue title at least; closing.

It absolutely does, thank you.

@golang golang locked and limited conversation to collaborators Jun 7, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

No branches or pull requests

5 participants