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/build: migrate to using Go modules #26872

Open
bcmills opened this Issue Aug 8, 2018 · 9 comments

Comments

Projects
None yet
5 participants
@bcmills
Member

bcmills commented Aug 8, 2018

On https://golang.org/cl/128636, @bradfitz notes:

I'd want to understand what we do & don't use [modules] for […].

For example, all of our "make deploy-prod" etc scripts right now build in a Docker container with an entirely different mechanism.

Initially, I think we should start with exactly what we're doing today, but with the ability to make changes in x/build with a known-reasonable set of dependencies. That's https://golang.org/cl/128636.

There are certainly more interesting steps we can take down the road: for example, we might be able to use go mod download in place of cmd/gitlock to wire in dependencies.

This issue is for tracking those steps.

@gopherbot gopherbot added this to the Unreleased milestone Aug 8, 2018

@gopherbot gopherbot added the Builders label Aug 8, 2018

@bcmills bcmills added NeedsInvestigation and removed Builders labels Aug 8, 2018

@gopherbot gopherbot added the Builders label Aug 8, 2018

@gopherbot

This comment has been minimized.

gopherbot commented Aug 8, 2018

Change https://golang.org/cl/128636 mentions this issue: all: add module definitions

gopherbot pushed a commit to golang/build that referenced this issue Aug 8, 2018

all: add module definitions
I computed these using a build of the Go toolchain from a working copy at
https://golang.org/cl/128136, patchset 12.

The versions selected by 'go mod tidy' needed two tweaks to pass tests:
go get golang.org/x/text@master
go get github.com/russross/blackfriday@v1

The x/text version bump is because the last tagged version (v0.3.0) is very old
(last December).

The blackfriday bump is because the repository made a breaking change (v2.0.0)
without changing the import path.

With those changes, 'go test ./...' passes at the repo root.
('go test all' fails, possibly due to golang/go#26279.)

Updates golang/go#26279.
Updates golang/go#26872.

Change-Id: Ieac5327e4bddd2b78b981d7683beb98608708a3a
Reviewed-on: https://go-review.googlesource.com/128636
Reviewed-by: Brad Fitzpatrick <bradfitz@golang.org>
@bcmills

This comment has been minimized.

Member

bcmills commented Aug 21, 2018

@FiloSottile notes: “there’s stuff in vendor/ that disagrees with the go.mod, we should fix that if we’re going all in on modules”

@bcmills

This comment has been minimized.

Member

bcmills commented Aug 21, 2018

(I'm guessing he's referring to the stuff in vendor/github.com/tarm/serial.)

@dmitshur

This comment has been minimized.

Member

dmitshur commented Aug 21, 2018

(I'm guessing he's referring to the stuff in vendor/github.com/tarm/serial.)

There's also https://golang.org/cl/129075, those 2 commands must be out of sync too by now.

Edit: Whoops, that's not related to stuff in vendor/, but rather a Dockerfile. Sorry.

@bradfitz bradfitz self-assigned this Nov 8, 2018

@bradfitz

This comment has been minimized.

Member

bradfitz commented Nov 8, 2018

I think it's time to delete gitlock.

I've been using modules for my own projects for some time and prefer it over gitlock. (which was always meant as a medium-term solution)

/cc @dmitshur

@dmitshur

This comment has been minimized.

Member

dmitshur commented Nov 8, 2018

Well, given this is x/build and its policy (which I can't find for some reason right now), we can experiment with modules as long as they work for our needs. See https://go-review.googlesource.com/c/build/+/128636/#message-82cc0747061a265a95ab44a9ef7debe6798a4392.

I'm in favor of using modules to build commands reproducibly for deployment (aka, replace what gitlock is used for now). 👍

As long as that doesn't interfere with keeping the libraries in this repo (e.g., maintner) working in the GOPATH world.

@bradfitz

This comment has been minimized.

Member

bradfitz commented Nov 8, 2018

First speed bump:

bradfitz@gdev:~/src/golang.org/x/build/cmd/gopherbot$ GO111MODULE=on go mod init

bradfitz@gdev:~/src/golang.org/x/build/cmd/gopherbot$ GO111MODULE=on go list -json .
.....
can't load package: package golang.org/x/build/cmd/gopherbot: unknown import path "golang.org/x/build/cmd/gopherbot": ambiguous import: found golang.org/x/build/cmd/gopherbot in multiple modules:
        golang.org/x/build/cmd/gopherbot (/home/bradfitz/src/golang.org/x/build/cmd/gopherbot)
        golang.org/x/build v0.0.0-20181108190521-5df336cd7fdc (/home/bradfitz/pkg/mod/golang.org/x/build@v0.0.0-20181108190521-5df336cd7fdc/cmd/gopherbot)

I suppose we could have one go.mod for all things in x/build but I'd prefer that each package main tool have its own, so we can bump dependencies independently as we deploy things.

Otherwise we'll be in a state where source code doesn't match deployed reality and we we'll bump go.mod deps affecting a dozen binaries and only deploy one of them, and have a few that haven't been deployed for years with untested deps.

What do I need to do to get past that error above, @bcmills?

@bcmills

This comment has been minimized.

Member

bcmills commented Nov 8, 2018

If you're splitting out a submodule, the new submodule needs to depend on a version of the parent that does not contain the newly-split packages.

(For strict go get -u compatibility it should be a cycle as described in #27858 (comment), but since nobody should be importing cmd/gopherbot from outside it probably doesn't matter in this case.)

If you want to actually test that change, you'll probably need a replace directive. (In general, testing changes to multiple modules in the same repo is difficult; see #27542.)

@myitcv

This comment has been minimized.

Member

myitcv commented Nov 8, 2018

To add to @bcmills's comment, whilst the replace method works (gist, results), you can also do this via separate commits.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment