Remove MAKE_IN_DOCKER, and associated changes

[LukeShu]

Per the vote for #124, remove the MAKE_IN_DOCKER magic and instead run commands on the host system. Some of those commands are system-y commands (make, go, python3, docker) and must be installed ahead-of-time on the host system, as documented in DEVELOPER.md (just as Envoy Proxy needs you to install a C compiler). Others (linters, controller-gen) the Makefile builds locally and installs to ./tools/bin/ in order to pin very specific version (just as Envoy Proxy builds a local copy of protoc).

With this new assumption that the Makefile controls the golangci-lint and codespell versions, I did away with the golangci/golangci-lint-action@v2 and codespell-project/actions-codespell@master GitHub Actions, instead having the workflow simply call make -k lint. However, I was careful that doing so did not disrupt the linters' ability to post annotations to the GitHub diff page; indeed earlier draft versions of this PR intentionally introduced things for both of these linters to complain about in order for me to validate that this still works. You can see that version of things on the lukeshu/bork branch.

As you will soon learn is usual for me, I recommend a commit-by-commit review.

fixes #124

[codecov-commenter]

Codecov Report

Merging #129 (1f6e312) into main (4192e8d) will not change coverage.
The diff coverage is n/a.

@@           Coverage Diff           @@
##             main     #129   +/-   ##
=======================================
  Coverage   53.08%   53.08%           
=======================================
  Files          12       12           
  Lines         486      486           
=======================================
  Hits          258      258           
  Misses        210      210           
  Partials       18       18           

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 4192e8d...1f6e312. Read the comment docs.

[lizan]

This makes the pushed image to amd64 (CI host arch) from multi arch.

[youngnick]

I think that's fine for now - we need to have one thing working before we worry about having multi-arch. Currently, we have nothing.

[arkodg]

we also want to tag and push with the SHA of the commit, can you please revert this

[LukeShu]

The sha is already pushed by the earlier make push-multiarch (or, pre-this-pr: is already pushed by a different earlier docker/build-push-action@v3 call)

[arkodg]

ah thanks for pointing it out.
If we left this the way it is, it would be make the CI faster since its leveraging the same cache for the multiple build steps

[LukeShu]

IMO, since everything should be doable without GHA, the better solution would be to go the other direction and move the :latest pushing to be a target in the Makefile, but I didn't have the appetite to do that in this PR.

[arkodg]

would prefer if this GHA remains the way it is, since the current changes doesn't improve the existing workflow and keep things consistent

[kflynn]

FTR, I tend to strongly believe that it should be a requirement that you can cut a release even if GHA is down, so I also believe that the push-to-:latest bit should move into a Makefile target.

[arkodg]

@kflynn that sounds like a valid case, prefer if we handle ^ Makefile target in a separate PR once completed so it does not overload this PR which is addressing #124

[kflynn]

@arkodg Completely agree that it should be a separate PR, yes. 🙂

[arkodg]

make lint seems to be failing for me due to pip version incompatibility

Successfully installed pathspec-0.9.0 pyyaml-6.0 yamllint-1.24.2
WARNING: You are using pip version 21.0.1; however, version 22.1.2 is available.
You should consider upgrading via the '/Users/arkodebdasgupta/go-workspace/src/github.com/envoyproxy/luke/tools/bin/yamllint.d/venv/bin/python3.9 -m pip install --upgrade pip' command.
ln -srf tools/bin/yamllint.d/venv/bin/yamllint tools/bin/yamllint
ln: illegal option -- r
usage: ln [-Ffhinsv] source_file [target_file]
       ln [-Ffhinsv] source_file ... target_dir
       link source_file target_file
make[1]: *** [tools/bin/yamllint] Error 1
make: *** [_run] Error 2

[arkodg]

after upgrading pip I hit some more issues

tools/make/lint.mk:18: warning: undefined variable `GITHUB_ACTION'
Running Go linter ...
tools/bin/golangci-lint run  --build-tags=e2e --config=tools/linter/golangci-lint/.golangci.yml
WARN [linters context] bodyclose is disabled because of go1.18. You can track the evolution of the go1.18 support by following the https://github.com/golangci/golangci-lint/issues/2649. 
WARN [linters context] structcheck is disabled because of go1.18. You can track the evolution of the go1.18 support by following the https://github.com/golangci/golangci-lint/issues/2649. 
WARN [linters context] unparam is disabled because of go1.18. You can track the evolution of the go1.18 support by following the https://github.com/golangci/golangci-lint/issues/2649. 
ln -srf tools/bin/yamllint.d/venv/bin/yamllint tools/bin/yamllint
ln: illegal option -- r
usage: ln [-Ffhinsv] source_file [target_file]
       ln [-Ffhinsv] source_file ... target_dir
       link source_file target_file
make[1]: *** [tools/bin/yamllint] Error 1
make: *** [_run] Error 2

[arkodg]

any specific reason to rm the current way ?

[LukeShu]

Consistency. And this consistent approach has fewer cross-arch headaches as things like "the installer for such-and-such tool doesn't support darwin-arm64 yet".

[arkodg]

not sure how this is more consistent, since the tools here are go based, arch should not play a role. I prefer the existing approach since its less complex (lesser files to maintain)

[LukeShu]

Sorry, I had kustomize in particular in mind when I said that; which was using a install_kustomize.sh script rather than go install (and the script does download a pre-compiled binary; hence the concern about arch). For controller-gen and setup-envtest, which were using a simple go install, I guess I should elaborate on why this is better.

Consistency: It isn't possible to do a simple go install for kustomize. For the version that we currently use, you have to add some exclude directives to a go.mod. I'd have to double-check to be sure, but I'm pretty sure that some newer versions also require some resplace directives. So trying to build consistency around basic go install will always have inconsistency with Kustomize. Other tools often end up in similar situations (it hasn't for a while now, but in the past golangci-lint has required replace directives, and may resume doing so in the future). This solution is applicable to any Go program, not just the ones that don't require exclude or replace.

Seamless: Having the version in its own file (go.mod) allows Make to know when it needs to rebuild. Say we land a change that requires us bumping a tool's version; there will surely be several contributors who are confused that their checkout no longer works after a git pull, they wouldn't realize that they need to purge their current binary to get Make to update it. By moving the version number from a Make variable to a separate file, Make can seamlessly updated it for them, and they'll never see breakage.

Reproducibility: When a tool doesn't yet use go.mod or depends on libraries that don't yet use go.mod, the bare go install may end up choosing different indirect-dependency library versions when one of them publishes an update, and cause surprising breakage. This isn't a problem if the tool's go.mod is updated by Go 1.18, but can be a problem if the tool's maintainers are still using and older Go (1.15?).

Maintainability: On the down side, as you mention: there are more files to maintain, but mitigating that is that updating just means editing one line in the go.mod, and then running make go-mod-tidy. And on the upside, dependabot knows how to update go.mod but not how to update a Make variable, so dependabot can help keep our tools up to date.

[arkodg]

thanks for the detailed reasoning, im still not convinced that this is the better approach for this project, and the existing one that is similar to the kubebuilder approach is much simpler to read through and maintain.
apart from this change, the PR LGTM, thanks for the multiple rebases, smaller PRs for specific areas should hopefully reduce such conflicts

[LukeShu]

make lint seems to be failing for me due to pip version incompatibility

Successfully installed pathspec-0.9.0 pyyaml-6.0 yamllint-1.24.2
WARNING: You are using pip version 21.0.1; however, version 22.1.2 is available.
You should consider upgrading via the '/Users/arkodebdasgupta/go-workspace/src/github.com/envoyproxy/luke/tools/bin/yamllint.d/venv/bin/python3.9 -m pip install --upgrade pip' command.
ln -srf tools/bin/yamllint.d/venv/bin/yamllint tools/bin/yamllint
ln: illegal option -- r
usage: ln [-Ffhinsv] source_file [target_file]
       ln [-Ffhinsv] source_file ... target_dir
       link source_file target_file
make[1]: *** [tools/bin/yamllint] Error 1
make: *** [_run] Error 2

Oh dang, macOS ln doesn't support -r :(

[arkodg]

I find myself moving my head up and down the screen trying to compute the dependency list.
prefer the current way with the add on dependancy of installing the tool

[LukeShu]

That's fair. I don't have a real strong opinion here, but to make an argument for this way:

• Avoiding merge conflicts when 2 different people add 2 different linters
• When adding a linter, there are fewer places where it needs updated. Don't need to define the target in one place, add a lint dependency in a second place, and a lint-deps dependency in a third place. I don't think we should be adding bunches of linters, but as the list grows, this will be a bigger deal.

[arkodg]

this is purely a taste aspect, prefer the existing approach to simplify code maintenance but this comment is non blocking

[arkodg]

@LukeShu can confirm that make lint seems to work fine on MacOS now, thanks for making the changes

[kflynn]

This looks good to me. There are things here that look more complex, but (based on the burned fingers from Emissary) end up being wins in the long term.

[kflynn]

I am loving seeing these @s vanish. 🙂

[kflynn]

FTR, I tend to strongly believe that it should be a requirement that you can cut a release even if GHA is down, so I also believe that the push-to-:latest bit should move into a Makefile target.

[LukeShu]

v4:

• Rebased to resolve conflicts. The only thing of note is that chore: implement make target for multi arch image build #139 added some commands to image.mk with @s for 6ad7760 to remove.

Range-diff:
 1:  691373d =  1:  dd2a01e .gitignore: Begone with pointless **/
 2:  4cb8904 =  2:  2e59fcb build-sys: Turn off intermediate file removal
 3:  e953d5f !  3:  c68457c build-sys: Don't use recursive Make
    @@ tools/make/common.mk: ifeq (${BINS},)
     +include tools/make/lint.mk
     +include tools/make/kube.mk
     +
    - define USAGE_OPTIONS
    - 
    - Options:
    -@@ tools/make/common.mk: help:
    - 	@echo "Envoy Gateway is an open source project for managing Envoy Proxy as a standalone or Kubernetes-based application gateway\n"
    - 	@echo "Usage:\n  make \033[36m<Target>\033[0m \033[36m<Option>\033[0m\n\nTargets:"
    - 	@awk 'BEGIN {FS = ":.*##"; printf ""} /^[a-zA-Z_0-9-]+:.*?##/ { printf "  \033[36m%-15s\033[0m %s\n", $$1, $$2 } /^##@/ { printf "\n\033[1m%s\033[0m\n", substr($$0, 5) } ' $(MAKEFILE_LIST)
    --	@echo "$$USAGE_OPTIONS"
    - \ No newline at end of file
    -+	@echo "$$USAGE_OPTIONS"
    + # Log the running target
    + LOG_TARGET = echo "===========> Running $@..."
    + # Log debugging info
     
      ## tools/make/golang.mk ##
     @@ tools/make/golang.mk: go.build.%:
    @@ tools/make/image.mk: image.verify:
     -	@$(MAKE) $(addprefix image.build., $(addprefix $(IMAGE_PLAT)., $(IMAGES)))
     +image.build: $(addprefix image.build.$(IMAGE_PLAT)., $(IMAGES))
      
    - .PHONY: image.build.multiarch
    - image.build.multiarch: image.verify
    - 	# TODO: use buildx
    - 
      .PHONY: image.build.%
     -image.build.%: go.build.%
     +image.build.%: go.build.% image.verify
      	$(eval IMAGES := $(COMMAND))
      	$(eval IMAGE_PLAT := $(subst _,/,$(PLATFORM)))
    - 	@echo "===========> Building image $(IMAGES) in tag $(TAG) for $(IMAGE_PLAT)"
    + 	@$(call log, "Building image $(IMAGES) in tag $(TAG) for $(IMAGE_PLAT)"
     @@ tools/make/image.mk: image.build.%: go.build.%
      	$(DOCKER) build --platform $(IMAGE_PLAT) $(BUILD_SUFFIX)
      
    @@ tools/make/image.mk: image.build.%: go.build.%
     -image.push: image.verify $(addprefix image.push., $(addprefix $(IMAGE_PLAT)., $(IMAGES)))
     +image.push: $(addprefix image.push.$(IMAGE_PLAT)., $(IMAGES))
      
    - .PHONY: image.push.multiarch
    --image.push.multiarch: image.verify  $(foreach p,$(IMAGE_PLATFORMS),$(addprefix image.push., $(addprefix $(p)., $(IMAGES)))) 
    -+image.push.multiarch: $(foreach p,$(IMAGE_PLATFORMS),$(addprefix image.push.$(p)., $(IMAGES)))
    - 
      .PHONY: image.push.%
     -image.push.%:
     +image.push.%: image.build.%
      	$(eval COMMAND := $(word 2,$(subst ., ,$*)))
      	$(eval IMAGES := $(COMMAND))
      	$(eval PLATFORM := $(word 1,$(subst ., ,$*)))
    -@@ tools/make/image.mk: image.push.%:
    +@@ tools/make/image.mk: image.push.multiarch: image.multiarch.setup go.build.multiarch
      
      .PHONY: image
      image: ## Build docker images for host platform. See Option PLATFORM and BINS.
 4:  ba12762 !  4:  6ad7760 build-sys: Don't hide what commands are running
    @@ tools/make/golang.mk
     @@ tools/make/golang.mk: go.build.%:
      	$(eval OS := $(word 1,$(subst _, ,$(PLATFORM))))
      	$(eval ARCH := $(word 2,$(subst _, ,$(PLATFORM))))
    - 	@echo "===========> Building binary $(COMMAND) with commit $(REV) in version $(VERSION) for $(OS) $(ARCH)"
    + 	@$(call log, "Building binary $(COMMAND) with commit $(REV) in version $(VERSION) for $(OS) $(ARCH)")
     -	@CGO_ENABLED=0 GOOS=$(OS) GOARCH=$(ARCH) go build -o $(OUTPUT_DIR)/$(OS)/$(ARCH)/$(COMMAND) $(ROOT_PACKAGE)/cmd/$(COMMAND)
     +	CGO_ENABLED=0 GOOS=$(OS) GOARCH=$(ARCH) go build -o $(OUTPUT_DIR)/$(OS)/$(ARCH)/$(COMMAND) $(ROOT_PACKAGE)/cmd/$(COMMAND)
      
    @@ tools/make/golang.mk: go.build.multiarch: $(foreach p,$(PLATFORMS),$(addprefix g
      
      .PHONY: go.clean
      go.clean: ## Clean the building output files
    - 	@echo "===========> Cleaning all build output"
    + 	@$(call log, "Cleaning all build output")
     -	@rm -rf $(OUTPUT_DIR)
     +	rm -rf $(OUTPUT_DIR)
      
      .PHONY: go.tidy
      go.tidy:
    - 	@echo "===========> Running go tidy" $(pwd)
    + 	@$(LOG_TARGET)
     -	@go mod tidy -compat=$(GO_VERSION)
     +	go mod tidy -compat=$(GO_VERSION)
      	## ensure all changes have been committed
      	git diff --exit-code go.mod
      	git diff --exit-code go.sum
     
    + ## tools/make/image.mk ##
    +@@ tools/make/image.mk: image.push.%: image.build.%
    + .PHONY: image.multiarch.verify
    + image.multiarch.verify:
    + 	$(eval pass := $(shell ))
    +-	@docker buildx --help | grep -qw "docker buildx" || { \
    ++	docker buildx --help | grep -qw "docker buildx" || { \
    + 		echo "Cannot find `docker buildx`, please install first"; \
    + 		exit 1; }
    + 
    + .PHONY: image.multiarch.emulate $(EMULATE_TARGETS)
    + image.multiarch.emulate: $(EMULATE_TARGETS)
    + $(EMULATE_TARGETS): image.multiarch.emulate.%:
    +-	@docker run --rm --privileged tonistiigi/binfmt --install linux/$* # Install QEMU emulator, the same emulator as the host will report an error but can safe ignore
    ++	docker run --rm --privileged tonistiigi/binfmt --install linux/$* # Install QEMU emulator, the same emulator as the host will report an error but can safe ignore
    + 
    + .PHONY: image.multiarch.setup
    + image.multiarch.setup: image.verify image.multiarch.verify image.multiarch.emulate
    +-	@docker run --rm --privileged tonistiigi/binfmt --install all # Install QEMU emulators
    +-	@docker buildx rm $(BUILDX_CONTEXT) || :
    +-	@docker buildx create --use --name $(BUILDX_CONTEXT) --platform "${BUILDX_PLATFORMS}"
    ++	docker run --rm --privileged tonistiigi/binfmt --install all # Install QEMU emulators
    ++	docker buildx rm $(BUILDX_CONTEXT) || :
    ++	docker buildx create --use --name $(BUILDX_CONTEXT) --platform "${BUILDX_PLATFORMS}"
    + 
    + .PHONY: image.build.multiarch
    + image.build.multiarch: image.multiarch.setup go.build.multiarch
    +
      ## tools/make/lint.mk ##
     @@ tools/make/lint.mk: lint: lint.golint lint.yamllint lint.codespell lint.whitenoise
      .PHONY: lint.golint
 5:  6ba4d4e =  5:  374d776 .github: Refactor a bit, splitting things up
 6:  c5e0c36 =  6:  cbb804d build-sys, .github: Have CI use the combined `make lint`
 7:  ec2c018 =  7:  77471cc build-sys: make lint: Restore integration with GitHub Actions annotations
 8:  e9fca96 =  8:  a0cb1bd build-sys: Introduce tools.mk to manage required tools
 9:  4779baa =  9:  6ac963e build-sys: Begone with MAKE_IN_DOCKER

[youngnick]

This LGTM; I think that any further changes would be better done as fresh PRs, the history and context here is getting a little large.

[LukeShu]

Just resolved minor conflicts with #146.

It can't be merged until @lizan resolves his change request.

Lizan is OOO this week. Any issues can be addressed in a follow-on PR