-
Notifications
You must be signed in to change notification settings - Fork 2.2k
Makefile: replace many scripts with make targets #3144
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
Conversation
In [1] and [2], Git LFS learned how to run its tests under prove(1) and replaced many of its script/*'s with Makefile-based targets. The platform-specific instructions for building `*.deb`'s and `*.rpm`'s must update accordingly so as not to refer to outdated (and now broken invocations) such as script/test. For example, this particular invocation--script/test--should be replaced with 'make test' instead. Apply the necessary changes to avoid bitrot and keep the Git LFS script/target invocations up-to-date. [1]: git-lfs/git-lfs#3125 [2]: git-lfs/git-lfs#3144
PastelMobileSuit
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This looks great! I tested all the make targets on OSX and everything worked for me aside from building the windows executables. However, those worked for me on Windows this morning so I think it's just an issue with my setup.
I did have a couple of minor pieces of feedback on the docs that I'd like to see addressed before we merge this in, but aside from that I'm 👍 !
Makefile
Outdated
| $(call BUILD,$(GOOS),$(GOARCH),) | ||
|
|
||
| all : bin/git-lfs | ||
| # bin/git-lfs targets the default output of Git LFS on Windows systems, and |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should be bin/git-lfs.exe here
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks, got this one in 0177b07 (Makefile: correct typo in bin/git-lfs.exe documentation, 2018-07-25).
Makefile
Outdated
| $(call BUILD,$(GOOS),$(GOARCH),.exe) | ||
|
|
||
| include $(MAKEFILE_GEN) | ||
| # resource.syso install the 'goversioninfo' command and uses it in order to |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
s/install/installs
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks! Got this one in 553a320 (Makefile: correct typo in the resource.syso documentation, 2018-07-25).
| # resource.syso install the 'goversioninfo' command and uses it in order to | ||
| # generate a binary that has information included necessary to create the | ||
| # Windows installer. | ||
| resource.syso: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I just tried running make resource.syso and make all on OSX and am getting
git-lfs.go:1: running "goversioninfo": exec: "goversioninfo": executable file not found in $PATH
Is this expected? From my reading of this comment it seems like running make resource.syso should already take care of that. I don't think I'm setup to cross-compile for Windows on OSX, so that may be the source of my issue.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🤔, hmm -- I'm not sure why this is happening! I cleared out $GOPATH/bin, and was able to build this successfully:
$ make -B resource.syso
go get github.com/josephspurrier/goversioninfo/cmd/goversioninfo
go generate
$ echo $?
0Do you want to debug this a little further offline together some later time? My hunch is that this is an environment-specific problem, since we were both able to watch it work on Windows this morning, and the output is from my machine (running macOS) this evening.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I actually just figured out the problem! $GOPATH/bin wasn't in my $PATH, adding it fixed the issue
| @@ -1,52 +1,386 @@ | |||
| GOC ?= gccgo | |||
| AR ?= ar | |||
| # GIT_LFS_SHA is the '--short'-form SHA1 of the current revision of Git LFS. | |||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The documentation in this file is great! ✨
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yay, thanks!
| release : $(RELEASE_TARGETS) | ||
| shasum -a 256 $(RELEASE_TARGETS) | ||
|
|
||
| # bin/releases/git-lfs-%-$(VERSION).tar.gz generates a gzip-compressed TAR of |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It might be worth putting an example here, like make bin/releases/git-lfs-darwin-amd64-v2.4.0-354-g27ff38c6.tar.gz. I that might make it a bit clearer how to use this make target
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good call, I added some documentation to this effect in edfcac9 (Makefile: add some extra documentation for building release artifacts, 2018-07-25). I also added a note on how to build Git LFS versions with a custom VERSION name, which you can do as described in that commit.
PastelMobileSuit
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Found one typo in a new commit, but aside from that ![]()
edfcac9 to
ac9f719
Compare
In commit c4874df of PR git-lfs#971 the script/cibuild script was revised to run the Go test suite just for our "git" Go package in order to confirm that we do not leak the GIT_TRACE environment variable to Git itself when it is set. Later, in commit 2235198 of PR git-lfs#3144, our Makefile was updated to start by running some Git commands ("git describe" and "git rev-parse") to determine the current tagged version of the project. When GIT_TRACE=1 is set in second run of the Go test suite, these initial Git command output Git trace log lines, which does not affect the validity of the second Go test of our "git" package but does add some noise to the output of the script/cibuild script. We therefore just unset the GIT_TRACE environment variable for these two Git commands in our Makefile.
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.
In commit 4c422b4 of PR git-lfs#113 the script/release script was added, which runs the script/release.go program, which uploaded release binaries to GitHub via a POST request, based on the values in a bin/releases/build_matrix.json file. This file is no longer created because the script/build.go file and accompanying script/bootstrap script were removed in commit 2235198 of PR git-lfs#3144. As we do not use the script/release or script/release.go files anymore as part of our release process, we can delete them now.
In commit 4c422b4 of PR git-lfs#113 the script/release script was added, which runs the script/release.go program to upload release binaries to GitHub via a POST request based on the values in a bin/releases/build_matrix.json file. This file is no longer created because the script/build.go file and accompanying script/bootstrap script were removed in commit 2235198 of PR git-lfs#3144. As we do not use the script/release or script/release.go files anymore as part of our release process, we can delete them now.
In commit 4c422b4 of PR git-lfs#113 the script/release script was added, which builds and runs a Go program from the script/script.go and script/release.go files. The mainRelease() function in that program uploads release binaries to GitHub via a POST request based on the values in a bin/releases/build_matrix.json file. However, that JSON file is no longer created because the script/build.go file and accompanying script/bootstrap script were removed in commit 2235198 of PR git-lfs#3144. As we do not use the script/release or remaining script/*.go files anymore as part of our release process, we can delete them now.
In commit 4c422b4 of PR git-lfs#113 the script/release script was added, which builds and runs a Go program from the script/script.go and script/release.go files. The mainRelease() function in that program uploads release binaries to GitHub via a POST request based on the values in a bin/releases/build_matrix.json file. However, that JSON file is no longer created because the script/build.go file and accompanying script/bootstrap script were removed in commit 2235198 of PR git-lfs#3144. As we do not use the script/release or remaining script/*.go files anymore as part of our release process, we can delete them now.
Since commit 9b0adeb of PR git-lfs#3125, when we converted our test suite to use the "prove" command, we have run up to nine test jobs in parallel in our primary test builds. However, as of commit 7fd5aa6 in PR git-lfs#3144, we only use a maximum of four parallel test jobs when building our Linux RPM packages. This has the effect of slowing down our CI suite, because, ever since commit 212c051 in PR git-lfs#3932, we build a range of Linux packages in our GitHub Actions CI workflow. (Note that for historical reasons, we only run the full "make test" Makefile target in our "t" test directory when building RPM packages; in our Debian packages, we just run the Go language tests. While this discrepency is worth addressing, for the moment we simply aim to improve the runtime of our RPM package builds.)
Since commit 9b0adeb of PR git-lfs#3125, when we converted our test suite to use the "prove" command, we have run up to nine test jobs in parallel in our primary test builds. However, as of commit 7fd5aa6 in PR git-lfs#3144, we only use a maximum of four parallel test jobs when building our Linux RPM packages. This has the effect of slowing down our CI suite, because, ever since commit 212c051 in PR git-lfs#3932, we build a range of Linux packages in our GitHub Actions CI workflow. (Note that for historical reasons, we only run the full "make test" Makefile target in our "t" test directory when building RPM packages; in our Debian packages, we just run the Go language tests. While this discrepency is worth addressing, for the moment we simply aim to improve the runtime of our RPM package builds.)
Since commit 9b0adeb of PR git-lfs#3125, when we converted our test suite to use the "prove" command, we have run up to nine test jobs in parallel in our primary test builds. However, as of commit 7fd5aa6 in PR git-lfs#3144, we only use a maximum of four parallel test jobs when building our Linux RPM packages. This has the effect of slowing down our CI suite, because, ever since commit 212c051 in PR git-lfs#3932, we build a range of Linux packages in our GitHub Actions CI workflow. (Note that for historical reasons, we only run the full "make test" Makefile target in our "t" test directory when building RPM packages; in our Debian packages, we just run the Go language tests. While this discrepency is worth addressing, for the moment we simply aim to improve the runtime of our RPM package builds.)
Since commit 9b0adeb of PR git-lfs#3125, when we converted our test suite to use the "prove" command, we have run up to nine test jobs in parallel in our primary test builds. However, as of commit 7fd5aa6 in PR git-lfs#3144, we only use a maximum of four parallel test jobs when building our Linux RPM packages. This has the effect of slightly slowing down our CI suite, because, ever since commit 212c051 in PR git-lfs#3932, we build a range of Linux packages in our GitHub Actions CI workflow. (Note that for historical reasons, we only run the full "make test" Makefile target in our "t" test directory when building RPM packages; in our Debian packages, we just run the Go language tests. While this discrepancy is worth addressing, for the moment we simply aim to improve the runtime of our RPM package builds.)
This pull request replaces many of the bash scripts in the
scriptdirectory with aMakefilein the project root.Here's a list of old scripts and their new Makefile-based replacements:
script/bootstrap➡️make, ormake allscript/fmt➡️make fmtscript/lint➡️make lintscript/man➡️make manscript/test➡️make testscript/vendor➡️make vendorThe aim in replacing many of our home-grown bash scripts with Makefiles is motivated by the structure and syntax of Makefiles, themselves. Makefiles provide a self-documenting way to describe how targets are built, what other files they depend upon, and how to build each step. In addition, Makefiles are familiar to developers used to working on other open-source projects. In particular, Git uses a Makefile to similar effect, and this brings Git LFS closer to feeling close and more familiar to developers used to working on upstream Git.
The
make fmt,make lint,make man,make test, andmake vendortargets above are documented extensively in the Makefile itself. However, the defaultmaketarget (and in particular,make alll)--though documented--I feel warrant special discussion here.make allreplacesscript/boostrap --all, which was formerly responsible for generating the.tar.gzand.ziprelease artifacts that are uploaded to each tagged release of Git LFS on GitHub. These were the hardest to port into a Makefile, but have been done so successfully, and runningmake allis equivalent to runningscript/bootstrap --all.make allhas a couple of stages. First, it builds binaries (stored inbin/git-lfs-$(GOOS)-$(GOARCH)-$(VERSION)) for all valid, supported combinations ofGOOSandGOARCH. Then, it builds.tar.gzand.zipfiles (on non-Windows and WindowsGOOS's, respectively), which are used to upload to GitHub for a release.The first stage is implemented by a
BUILDmacro, which takes as argumentsGOOS,GOARCH, and an optional build suffix, used to append.exefor Windows builds. The second stage is implemented by rules matching.tar.gzand.zip, which includes the appropriate binary, the README.md, CHANGELOG.md, and script/install` and then packages the contents up as a compressed directory.This is the most complicated example of a target that was ported from a bash script to a Makefile. Though the above explanation is a little lengthy, I think that overall the cognitive load for Makefiles is overall lower than the cognitive load for Bash scripts+Go, so I think that this is an improvement for this domain.
Other noticeable replacements involve several commonly used scripts, like
script/testandscript/integration.script/testhas been replaced bymake test(along withmake test-race,make test-verbose, andmake test-bench, to run the test suite in race-detection, verbose, and benchmarking mode(s), respectively).All Makefile targets take an optional
PKGSargument, which specifies which packages should be worked on when compiling or otherwise running a recipe. For example, if you want only to run tests in packagelfsapi, you can do so by runningmake PKGS=lfsapi test, or similar for other targets.A few scripts cannot be removed at the time of writing, and they are listed below:
script/backport-pr: This is a bash script that is useful in backporting PRs, and not suited for being ported into a Makefile.script/changelog: This is a bash script that is used to generate release changelogs, and is also not well-suited for being ported into a Makefile.script/cibuild: This bash script is a shorthand for runningmake test && make integration, but sets up third-party CI-specific environment settings, such as the path togoimports, so is left alone as to not pollute the Makefile with these sorts of environment-level configurations.script/compile-win-installer-unsigned.bat: This bash script is used by InnoSetup to build the Windows installer, so must remain a bash script.script/install-git-source: This bash script is used by a few third-party CI providers to install particular versions of the Git source, so remains thusly.script/install.sh.example: This is used to package in release tarballs, and is therefore left alone.script/packagecloud.rb: This Ruby script is used to upload *.deb and *.rpm release artifacts to PackageCloud, and thus is not suited for porting into a Makefile.script/release{,.go}: This is used to upload the release artifacts to GitHub, and is therefore not well-suited for turning into a Makefile, and is left alone thusly.script/windows-installer: This kicks off the InnoSetup process, so is left thusly.Lastly, here are a few scripts that were removed while pruning the
scriptdirectory:script/build.go: used to powerscript/bootstrap, and is now no longer needed.script/genmakefile: used as a shorthand to rungo generateto make the old GCC-based Makefile, which is no longer needed.script/run: used as shorthand forgo run, but no longer in use.script/update-version: used to update the version information in various places throughout the source code, but is no longer in use./cc @git-lfs/core