🌱 Makefile: fix issues with repeated evaluations, other minor improvements #813
Conversation
✅ Deploy Preview for olmv1 ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #813 +/- ##
==========================================
- Coverage 67.21% 64.64% -2.57%
==========================================
Files 23 16 -7
Lines 1467 1321 -146
==========================================
- Hits 986 854 -132
+ Misses 415 404 -11
+ Partials 66 63 -3
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
|
/lgtm Just had the one comment about removing a comment. |
|
|
||
| # Define dependency versions (use go.mod if we also use Go code from dependency) | ||
| export CERT_MGR_VERSION := v1.9.0 | ||
| export CATALOGD_VERSION := $(shell go list -mod=mod -m -f "{{.Version}}" github.com/operator-framework/catalogd) |
There was a problem hiding this comment.
How does this new variable assignment impact doing something like:
make run CATALOGD_VERSION=vX.Y.ZIIUC using the ?= variable assignment would only expand the variables if they were not already defined via something like the above example. Is overriding the values still possible with this new variable assignment?
There was a problem hiding this comment.
If we want the variables to be overridable, they would need this sort of construction:
ifeq (,$(VAR))
VAR := $(shell echo "Hello")
endif
Does the catalogd version need to be configurable like that? If so, I can change it. Let me know if there are other variables in the same boat that you see with the non-overridable assignment.
Makefile
Outdated
| export WAIT_TIMEOUT ?= 60s | ||
| IMG?=$(IMAGE_REPO):$(IMAGE_TAG) | ||
| TESTDATA_DIR := testdata | ||
| ifeq (, $(IMAGE_REPO)) |
There was a problem hiding this comment.
These questions are all for the changes to former ?= statements.
The gnu recommendation is to have ifeq(test, expected), e.g.
ifeq ($(IMAGE_REPO),)but AFAIK this should be fine. It should be noted that this will trigger even if the test var is empty, which in this case would overwrite with the default value for the arg. I can think of no case where we would want the variable to be defined-but-empty, so it's probably fine.
Any reason that this is superior to the conditional variable example:
ifeq ($(origin IMAGE_REPO), undefined)
IMAGE_REPO = blah
endif?
Finally, this is supposed to be the semantic equivalent of ?= and it seems reasonable to expect no performance benefit from this particular change. If that's not the case, how big a performance impact are we seeing?
There was a problem hiding this comment.
This is not great
ifeq ($(origin IMAGE_REPO), undefined)
IMAGE_REPO = $(shell long-running.sh)
endif
This is better
ifeq ($(origin IMAGE_REPO), undefined)
IMAGE_REPO := $(shell long-running.sh)
endif
In the first case (which is equivalent to ?=), long-running-sh runs every time $(IMAGE_REPO) is referenced.
In the second case, long-running.sh only runs once.
There was a problem hiding this comment.
I'll switch to this style though. I just saw that in the docs as well
ifeq ($(origin IMAGE_REPO), undefined)
IMAGE_REPO := $(shell long-running.sh)
endif
joelanford
force-pushed
the
makefile-improvements
branch
from
c58accc
to
75570c2
Compare
|
New changes are detected. LGTM label has been removed. |
b08a1e9
| E2E_REGISTRY_NAME := docker-registry | ||
| E2E_REGISTRY_NAMESPACE := operator-controller-e2e | ||
| export REG_PKG_NAME := registry-operator |
There was a problem hiding this comment.
@joelanford I found a misalignment here ;)
I would prefer if we could avoid formatting like this as it is hard to maintain this style. This also leads to larger diffs: for example, here I wanted to only remove one line but I had to change another for it to not look weird.
Description
I was experiencing extreme delays in running
makefor any target so finally decided to run it throughstraceto see what I could find. It turns outmakewas evaluating almost all of our variables many many times due to the use of=and?=instead of:=.See https://www.gnu.org/software/make/manual/html_node/Flavors.html#Flavors for docs about the various flavors of variables.
While I was at it, I also cleaned up and improved a few other things:
Reviewer Checklist