Makefile: unconditionally build plugins

[marquiz]

Changes the build targets of plugins to effectively "phony", always unconditionally running the build without trying to check dependencies. The previous make targets were faulty, e.g. changing something in the library didn't rebuild the plugins. This is a simple and stupid fix that I believe is good enough - building the plugins is really fast and go compiler caching makes it super fast for "unnecessary" builds.

NOTE: If merged, obsoletes #207

[marquiz]

@klihub @chrishenzie @mikebrow

[thaJeztah]

Wondering if a pattern as is used in containerd (using a FORCE target) would make sense, so that the targets still correlate with the path they're built in https://github.com/containerd/containerd/blob/da60207dc4b13b91bcee8a7ded48fc6dd022a96c/Makefile#L236

That said, there's probably more to do in this Makefile to make it fit standard conventions; https://github.com/containerd/containerd/blob/da60207dc4b13b91bcee8a7ded48fc6dd022a96c/Makefile#L236

# Base path used to install.
# The files will be installed under `$(DESTDIR)/$(PREFIX)`.
# The convention of `DESTDIR` was changed in containerd v1.6.
PREFIX        ?= /usr/local
BINDIR        ?= $(PREFIX)/bin
DATADIR       ?= $(PREFIX)/share
DOCDIR        ?= $(DATADIR)/doc
MANDIR        ?= $(DATADIR)/man

[marquiz]

I don't have strong opinions, whatever the maintainers accept. I have no personal passion for makefile wizardry, just want the make target to work for me 😅

That said, there's probably more to do in this Makefile to make it fit standard conventions

Agree on that :)

[klihub]

I think FORCE is better than .PHONY. It is semantically accurate, while .PHONY is not if targets are files to be built.

[klihub]

@marquiz @thaJeztah I don't want to bikeshed (too much) about the Makefile, so I just note here that if we go by @thaJeztah suggestions, then we end up with a much smaller diff and (as a bikeshedder-bonus) we keep file-based targets:

diff --git a/Makefile b/Makefile
index 71f52d6..e01b448 100644
--- a/Makefile
+++ b/Makefile
@@ -98,14 +98,16 @@ clean-cache:
 # plugins build targets
 #
 
-$(BIN_PATH)/%: plugins/%/*
+$(BIN_PATH)/% build/bin/%: FORCE
        $(Q)echo "Building $@..."; \
-       cd $(dir $<) && $(GO_BUILD) -o $@ .
+       $(GO_BUILD) -C plugins/$(notdir $@) -o $(abspath $@) .
 
-$(BIN_PATH)/wasm: plugins/wasm/
+$(BIN_PATH)/wasm build/bin/wasm: FORCE
        $(Q)echo "Building $@..."; \
        mkdir -p $(BIN_PATH) && \
-       cd $(dir $<) && GOOS=wasip1 GOARCH=wasm $(GO_BUILD) -o $@ -buildmode=c-shared .
+       GOOS=wasip1 GOARCH=wasm $(GO_BUILD) -C plugins/$(notdir $@) -o $(abspath $@) -buildmode=c-shared .
+
+FORCE:

Also, we could also use then -C for consistency in testing-related rules since it seems to work there as well, with a small extra patch:

klitkey1-mobl1 nri $ git diff 
diff --git a/Makefile b/Makefile
index 610d8f6..117656e 100644
--- a/Makefile
+++ b/Makefile
@@ -132,10 +132,10 @@ ginkgo-tests:
        $(GO_CMD) tool cover -html=$(COVERAGE_PATH)/coverprofile -o $(COVERAGE_PATH)/coverage.html
 
 test-ulimits:
-       $(Q)cd ./plugins/ulimit-adjuster && $(GO_TEST) -v
+       $(Q)$(GO_TEST) -C plugins/ulimit-adjuster -v
 
 test-device-injector:
-       $(Q)cd ./plugins/device-injector && $(GO_TEST) -v
+       $(Q)$(GO_TEST) -C plugins/device-injector -v
 
 codecov: SHELL := $(shell which bash)
 codecov:

[thaJeztah]

FWIW, current versions of Go also allows for a -C <path> argument, which can safe some headaches on having to cd to a directory ("and back again" in some cases).

From go help build;

The build flags are shared by the build, clean, get, install, list, run,
and test commands:

	-C dir
		Change to dir before running the command.
		Any files named on the command line are interpreted after
		changing directories.
		If used, this flag must be the first one in the command line.
	-a
		force rebuilding of packages that are already up-to-date.

[marquiz]

Updated to use FORCE. Changed the target so that I'm able to do make build/bin/hook-injector (instead of requiring make $(pwd)/build/bin/hook-injector)

[thaJeztah]

I don't have strong opinions, whatever the maintainers accept. I have no personal passion for makefile wizardry, just want the make target to work for me 😅

Ha! I'm definitely not a Makefile aficionado myself. Or at least; I can make them work for my own needs, but it always takes me 10 tries to write them to be "mostly correct" to fit standards.

Either .Phony or Force is fine with me (many of our (docker, moby) Makefiles use .Phony), but it's good to have a consistent approach between projects within the containerd org.

w.r.t. fixing the PREFIX and BINPATH (etc.) conventions; it's probably fine to keep that as a follow-up exercise. I know those are very easy to get wrong, so (again) if would be good to have an proven approach / pattern that works, and re-use the same approach within Makefiles used within containerd. I know I've run into ever-so-subtly differences in how they're handled between projects, and now ending up with binaries in the wrong place (so having to taper over those differences).

I recently found that the OpenSSF "best practices" also has it as one of the requirements (for "silver"); https://www.bestpractices.dev/en/projects/1271?criteria_level=1#quality

The installation system for end-users MUST honor standard conventions for selecting the location where built artifacts are written to at installation time. For example, if it installs files on a POSIX system it MUST honor the DESTDIR environment variable. If there is no installation system or no standard convention, select "not applicable" (N/A).

(in addition to);

Build systems for native binaries MUST honor the relevant compiler and linker (environment) variables passed in to them (e.g., CC, CFLAGS, CXX, CXXFLAGS, and LDFLAGS) and pass them to compiler and linker invocations. A build system MAY extend them with additional flags; it MUST NOT simply replace provided values with its own. It should be easy to enable special build features like Address Sanitizer (ASAN), or to comply with distribution hardening best practices (e.g., by easily turning on compiler flags to do so).

[marquiz]

Update: implemented @klihub's suggestions to make the diff a tad smaller

[mikebrow]

sure why not
LGTM