Skip to content

Conversation

@ttaylorr
Copy link
Contributor

This pull request replaces many of the bash scripts in the script directory with a Makefile in the project root.

Here's a list of old scripts and their new Makefile-based replacements:

  • script/bootstrap ➡️ make, or make all

  • script/fmt ➡️ make fmt

  • script/lint ➡️ make lint

  • script/man ➡️ make man

  • script/test ➡️ make test

  • script/vendor ➡️ make vendor

The 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, and make vendor targets above are documented extensively in the Makefile itself. However, the default make target (and in particular, make alll)--though documented--I feel warrant special discussion here.

make all replaces script/boostrap --all, which was formerly responsible for generating the .tar.gz and .zip release 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 running make all is equivalent to running script/bootstrap --all.

make all has a couple of stages. First, it builds binaries (stored in bin/git-lfs-$(GOOS)-$(GOARCH)-$(VERSION)) for all valid, supported combinations of GOOS and GOARCH. Then, it builds .tar.gz and .zip files (on non-Windows and Windows GOOS's, respectively), which are used to upload to GitHub for a release.

The first stage is implemented by a BUILD macro, which takes as arguments GOOS, GOARCH, and an optional build suffix, used to append .exe for Windows builds. The second stage is implemented by rules matching .tar.gz and .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/test and script/integration. script/test has been replaced by make test (along with make test-race, make test-verbose, and make test-bench, to run the test suite in race-detection, verbose, and benchmarking mode(s), respectively).

All Makefile targets take an optional PKGS argument, 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 package lfsapi, you can do so by running make 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 running make test && make integration, but sets up third-party CI-specific environment settings, such as the path to goimports, 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 script directory:

  • script/build.go: used to power script/bootstrap, and is now no longer needed.

  • script/genmakefile: used as a shorthand to run go generate to make the old GCC-based Makefile, which is no longer needed.

  • script/run: used as shorthand for go 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

@ttaylorr ttaylorr added this to the v2.5.0 milestone Jul 24, 2018
ttaylorr added a commit to git-lfs/build-dockers that referenced this pull request Jul 25, 2018
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
Copy link
Contributor

@PastelMobileSuit PastelMobileSuit left a 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
Copy link
Contributor

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

Copy link
Contributor Author

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
Copy link
Contributor

Choose a reason for hiding this comment

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

s/install/installs

Copy link
Contributor Author

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:
Copy link
Contributor

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.

Copy link
Contributor Author

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 $?
0

Do 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.

Copy link
Contributor

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.
Copy link
Contributor

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! ✨

Copy link
Contributor Author

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
Copy link
Contributor

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

Copy link
Contributor Author

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.

Copy link
Contributor

@PastelMobileSuit PastelMobileSuit left a 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 :shipit:

@ttaylorr ttaylorr force-pushed the ttaylorr/makefile branch from edfcac9 to ac9f719 Compare July 26, 2018 00:02
@ttaylorr ttaylorr merged commit 3f3faa9 into master Jul 26, 2018
@ttaylorr ttaylorr deleted the ttaylorr/makefile branch July 26, 2018 01:23
chrisd8088 added a commit to chrisd8088/git-lfs that referenced this pull request Dec 26, 2022
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.
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.
chrisd8088 added a commit to chrisd8088/git-lfs that referenced this pull request Sep 8, 2023
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.
chrisd8088 added a commit to chrisd8088/git-lfs that referenced this pull request Sep 8, 2023
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.
chrisd8088 added a commit to chrisd8088/git-lfs that referenced this pull request Sep 8, 2023
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.
chrisd8088 added a commit to chrisd8088/git-lfs that referenced this pull request Sep 8, 2023
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.
chrisd8088 added a commit to chrisd8088/git-lfs that referenced this pull request Sep 6, 2024
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.)
chrisd8088 added a commit to chrisd8088/git-lfs that referenced this pull request Sep 8, 2024
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.)
chrisd8088 added a commit to chrisd8088/git-lfs that referenced this pull request Sep 15, 2024
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.)
chrisd8088 added a commit to chrisd8088/git-lfs that referenced this pull request Oct 5, 2024
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.)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants