Skip to content

all: use Go modules instead of Glide#3208

Merged
ttaylorr merged 4 commits intomasterfrom
ttaylorr/go-mod
Aug 29, 2018
Merged

all: use Go modules instead of Glide#3208
ttaylorr merged 4 commits intomasterfrom
ttaylorr/go-mod

Conversation

@ttaylorr
Copy link
Contributor

This pull request leverages #3203 and packages Git LFS as a Go 1.11 Go Module, thereby no longer using Glide.

The changes herein are described in detail in each commit, but a few notable things stick out that are worth mentioning at the top-level:

  • 1819e67, & f8ffbb5: these are both preparatory commits, the first fixing a broken Makefile recipe, and the second removing a long-unused environment setting from the rules file in Debian builds.

  • cd81c77: this is where the majority of the change happened. In particular:

    • _test.go files (and their friends) have been removed, since go mod does not keep _test.go files in the vendor tree, unlike glide, which did.

    • We chose to keep $GO111MODULE unset, to indicate that the go mod can automatically detect that this repository does contain a Go module, and detect it as such. This requires the repository to be checked out outside of your system $GOPATH, or otherwise you must set $GO111MODULE to on (the later will break in a subsequent version of Go).

    • Keeping with this theme, since we use dh_golang to build on Debian, and it does not yet have support for Go modules, let's explicitly enable $GO111MODULE to force Go on Debian builds to treat the repository as a Go module, even though the checkout takes place in a $GOPATH.

/cc @git-lfs/core

ttaylorr and others added 2 commits August 28, 2018 17:11
Since [1], we have caused 'make lint' (or any recipes that list 'lint'
as a prerequisite) to fail if Git LFS imports non-standard packages
outside of its vendor tree.

However, this approach is deficient in that it does not correctly report
whether there are improperly vendored dependencies, since the pipeline
ends with "|| exit 0".

To prepare for a change in the invocation of "go list", let's make sure
that a final "grep ." fails (i.e., that the output of the pipeline up
until that point is empty) by adding a final "grep ." and negating the
exit code of the entire pipeline.

[1]: a1305a8 (Makefile: replace script/{fmt,lint} with '{fmt,lint}'
target, 2018-07-16)

Co-authored-by: brian m. carlson <bk2204@github.com>
This option was introduced historically in order to build Go packages in
Go 1.5 with vendored dependencies in the vendor tree.

GO15VENDOREXPERIMENT has long since been deprecated, and is no longer
required in modern versions of Go in order to instruct to the 'go' tool
to look in this directory.

Co-authored-by: brian m. carlson <bk2204@github.com>
@ttaylorr ttaylorr added this to the v2.6.0 milestone Aug 29, 2018
@ttaylorr ttaylorr force-pushed the ttaylorr/go-mod branch 7 times, most recently from f3ef3f2 to 9c6e6ab Compare August 29, 2018 17:19
Since we are now building on Go 1.11 (as of 074a2d4 (all: use Go 1.11
in CI, 2018-08-28)) and Go 1.11 supports Go Modules [1], let's stop
using Glide, and begin using Go Modules.

This involves a few things:

  * Teach the Makefile how to build go.sum files instead of glide.lock
    files.

  * Teach continuous integration services to build Git LFS in a
    non-$GOPATH environment, since (without setting GO111MODULE=on
    explicitly, which we choose not to do), this will break compiling
    Git LFS, because Go 1.11 will ignore modules present in a Go
    checkout beneath $GOPATH.

  * In order to do the above, let's also make sure that we are
    un-setting $GOCACHE in the environment, as this causes Go to work
    without modules support [2].

  * Because we're no longer building in a `$GOPATH`-based location,
    let's instruct the CircleCI base image to archive the new location,
    too.

  * Similarly, teach the RPM spec to build in a non-$GOPATH location.

  * By contrast, since we use dh_golang to build git-lfs binaries on
    Debian, let's wait until the upstream dh_golang package is released
    with support for Go 1.11 module support explicitly. Therefore, force
    GO111MODULE to be on so that we can build a copy of Git LFS whose
    checkout is within a $GOPATH.

Although the go.mod versions match the glide.yaml ones, the diff
attached is large because Go Modules do not vendor `_test.go` files,
whereas Glide does.

[1]: https://golang.org/doc/go1.11#modules
[2]: `GOCACHE=on` will be deprecated in Go 1.12, so this change makes
     sense for that reason, too.

Co-authored-by: brian m. carlson <bk2204@github.com>
@ttaylorr ttaylorr force-pushed the ttaylorr/go-mod branch 5 times, most recently from 535fc5d to f497e10 Compare August 29, 2018 17:53
@bk2204
Copy link
Member

bk2204 commented Aug 29, 2018

I did some testing on this on an Ubuntu 18.04 (bionic) Docker container. My testing shows that this change does break building there, since Go 1.11 isn't yet available, and consequently Go module support is missing. I don't think that's fatal, especially since Debian and Ubuntu don't use our vendored dependencies, but I thought I'd point it out anyway.

Copy link
Member

@bk2204 bk2204 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've looked at this and it looks sane to me.

My only concern is the one I pointed out in an earlier comment. I'm fine with merging this as it stands.

@ttaylorr ttaylorr force-pushed the ttaylorr/go-mod branch 3 times, most recently from fd582d2 to f396977 Compare August 29, 2018 19:05
In 2235198 (Makefile: replace script/bootstrap with 'make', 'make all',
2018-07-19), the Makefile learned how to make binaries of git-lfs,
including invoking 'go generate' in order to generate the resource.syso,
necessary for building on Windows.

In the commit mentioned above, we taught the Makefile recipe for
'resource.syso' to 'go get' the necessary 'goversioninfo' binary before
building, if callers do not have it.

In Go 1.10.3 and earlier, this caused no change to the vendor directory
or glide.yaml file, and instead installed the project in '$GOPATH', and
built the binary in '$GOPATH/bin'.

Since we are on Go 1.11, now, and running 'go get' from inside a module
directory causes the module's go.mod (and thusly, the go.sum, too) to
change, let's make it the caller's responsibility to provide a valid
'goversioninfo' binary in '$PATH'.

This allows us to avoid the burden of cleaning up after ourselves when
running 'go get' from within a directory that is itself a Go module.
@ttaylorr
Copy link
Contributor Author

ttaylorr commented Aug 29, 2018 via email

@ttaylorr ttaylorr merged commit 35fe301 into master Aug 29, 2018
@ttaylorr ttaylorr deleted the ttaylorr/go-mod branch August 29, 2018 20:59
ttaylorr added a commit that referenced this pull request Oct 17, 2018
Since the advent of [1], and [2], we use Go 1.11 with modules enabled.
This relieves us of the requirement of having Git LFS checked out within
the appropriate location beneath the caller's $GOPATH.

So, let's remove the guideline, and recommend away from using `go get`,
since this command behaves differently based on whether or not its CWD
is itself a Go repository.

Instead, recommend that we use `git clone`, and do not specify a
location to perform the clone in, since Git LFS can be checked out from
anywhere.

[1]: dfc0f2f (Merge pull request #3298 from git-lfs/ttaylorr/go-1.11.1,
     2018-10-08)
[2]: 35fe301 (Merge pull request #3208 from git-lfs/ttaylorr/go-mod,
     2018-08-29)
chrisd8088 added a commit to chrisd8088/git-lfs that referenced this pull request Aug 10, 2021
When our go.mod file was introduced in commit
114e85c in PR git-lfs#3208, the module
path chosen did not include a trailing /v2 component.  However,
the Go modules specification now advises that module paths must
have a "major version suffix" which matches the release version.

We therefore add a /v2 suffix to our module path and all its
instances in import paths.

See also https://golang.org/ref/mod#major-version-suffixes for
details regarding the Go module system's major version suffix rule.
chrisd8088 added a commit to chrisd8088/git-lfs that referenced this pull request Sep 8, 2023
In PR git-lfs#4903 we removed use of a "vendor" directory for Go modules,
so we can now drop several references to that directory from our
CI build script and Makefile, and our license documentation.

Note that the "lint" Makefile target was introduced in PR git-lfs#3144
(and then modified in PR git-lfs#3208 when the project adopted Go modules),
and existed to check whether all dependencies outside those from
the Go standard library existed in the "vendor" directory.  At
present, though, the command now finds no dependencies that are
neither in the standard library nor are Go modules, and so is a
no-op which we can simply remove.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants