Conversation
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>
f3ef3f2 to
9c6e6ab
Compare
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>
535fc5d to
f497e10
Compare
Member
|
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. |
bk2204
approved these changes
Aug 29, 2018
Member
bk2204
left a comment
There was a problem hiding this comment.
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.
fd582d2 to
f396977
Compare
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.
f396977 to
79f8e7c
Compare
Contributor
Author
|
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.
Thanks for testing, and I agree with your assessment. I think that we
share the perspective that it's OK that the vendor tree is used instead
of go.mod, since it means that:
1. We can compile out of the box, and
2. If someone wants to build a package on that platform, they won't
use either, anyway.
Thanks for your help pairing on this! :-).
|
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.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
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.gofiles (and their friends) have been removed, sincego moddoes not keep_test.gofiles in the vendor tree, unlikeglide, which did.We chose to keep
$GO111MODULEunset, to indicate that thego modcan 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$GO111MODULEtoon(the later will break in a subsequent version of Go).Keeping with this theme, since we use
dh_golangto build on Debian, and it does not yet have support for Go modules, let's explicitly enable$GO111MODULEto 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