git724/forgejo
1
0
Fork 0
Code Issues Pull requests 3 Projects 1 Releases Packages Wiki Activity

refactored for lazy evaluation of MIGRATION_PACKAGES and GO_TEST_PACKAGES (#9208)

Browse source 
solves: 9204

## About the PR

In the original Makefile, the variables `GO_TEST_PACKAGES` and `MIGRATION_PACKAGES` have been evaluated always and for every target. See issue 9204

This Change moves the evaluation of these Variables into a dedicated new make-target. This target then is added as dependency only to the other targets, that require/access these variables at all, so targets that don't require the variables don't do an evaluation of them.

### Note on the new Makefile Dependencies

I'm using Order-Only Prerequisites in the `Makefile`because, its good habit to only impose the least  force necessary. As we're only going for `.PHONY` targets here,
See: https://www.gnu.org/software/make/manual/make.html#Prerequisite-Types
- We need to ensure that the variable computation targets (compute-go-test-packages, compute-migration-packages) run before the targets that use those variables. This is purely about execution ordering - we need the variables to be defined before they're used.
- We don't need the timestamp-based rebuild logic that comes with normal dependencies. Normal dependencies say "if the dependency is newer than the target, rebuild the target." But we don't want the variable computation to trigger rebuilds - we only want it to ensure the variables are available.
- It's about minimal constraint - only imposing the constraints you actually need. Order-only prerequisites express our intent more precisely: we want sequencing without timestamp dependency. This makes the Makefile more semantically correct and future-proof.
Even though in this specific case (with PHONY targets) the practical difference is minimal, using order-only prerequisites is better engineering practice because it accurately represents the relationship we want between these targets.

## Checklist

The [contributor guide](https://forgejo.org/docs/next/contributor/) contains information that will be helpful to first time contributors. There also are a few [conditions for merging Pull Requests in Forgejo repositories](https://codeberg.org/forgejo/governance/src/branch/main/PullRequestsAgreement.md). You are also welcome to join the [Forgejo development chatroom](https://matrix.to/#/#forgejo-development:matrix.org).

### Tests

- I added test coverage for Go changes...
  - [ ] in their respective `*_test.go` for unit tests.
  - [ ] in the `tests/integration` directory if it involves interactions with a live Forgejo server.
  - [ ] in `web_src/js/*.test.js` if it can be unit tested.
  - [ ] in `tests/e2e/*.test.e2e.js` if it requires interactions with a live Forgejo server (see also the [developer guide for JavaScript testing](https://codeberg.org/forgejo/forgejo/src/branch/forgejo/tests/e2e/README.md#end-to-end-tests)).

- [x] I tested the results of the affected targets and some other targets before and after the change:
    - [x] `make  help` -- before: slow, called git and downloaded -- after: fast, direct result
    - [x] `make  clean` -- before: slow, called git and downloaded -- after: fast, direct result
    - [x] `make  clean-all ` -- before: slow, called git and downloaded -- after: fast, direct result
    - [x] `make frontend` -- before: called git and downloaded -- after: fast, frontend build succeeded
    - [x] `make go-check` -- before: called git and downloaded -- after: faster, go check succeeded
    - [x] `make generate-backend` -- before: called git and downloaded -- after: fast, backend generated successfully
    - [x] `make static-executable` -- before: called git and downloaded -- after: fast, executable generated
    - [x] `make build` -- before: called git and downloaded -- after: fast, build succeeded
    - [x] 'test-backend' -- before: succeeded -- after succeeded.
    - [x] 'test' -- before: succeeded -- after succeeded.
    - [x] `migrations.individual.mysql.test' -- before: succeeded -- after succeeded.
    - [x] 'test' -- before: succeeded -- after succeeded.

### Documentation

- [ ] I created a pull request [to the documentation](https://codeberg.org/forgejo/docs) to explain to Forgejo users how to use this change.
- [x] I did not document these changes and I do not expect someone else to do it.

### Release notes

- [ ] I do not want this change to show in the release notes.
- [x] I want the title to show in the release notes with a link to this pull request.
- [ ] I want the content of the `release-notes/<pull request number>.md` to be be used for the release notes instead of the title.

Co-authored-by: iQ <iq@in6-addr.net>
Reviewed-on: https://codeberg.org/forgejo/forgejo/pulls/9208
Reviewed-by: Earl Warren <earl-warren@noreply.codeberg.org>
Co-authored-by: iQ <iq@noreply.codeberg.org>
Co-committed-by: iQ <iq@noreply.codeberg.org>
This commit is contained in:
iQ 2025-09-09 09:48:20 +02:00 • committed by Earl Warren
commit 2aa73a8fad
1 changed files with 25 additions and 13 deletions
Download patch file Download diff file Expand all files Collapse all files

38
Makefile
View file

@ -115,9 +115,6 @@ LDFLAGS := $(LDFLAGS) -X "main.ReleaseVersion=$(RELEASE_VERSION)" -X "main.MakeV
LINUX_ARCHS ?= linux/amd64,linux/386,linux/arm-5,linux/arm-6,linux/arm64
ifeq ($(HAS_GO), yes)
GO_TEST_PACKAGES ?= $(filter-out $(shell $(GO) list forgejo.org/models/migrations/...) $(shell $(GO) list forgejo.org/models/forgejo_migrations/...) forgejo.org/tests/integration/migration-test forgejo.org/tests forgejo.org/tests/integration forgejo.org/tests/e2e,$(shell $(GO) list ./...))
endif
REMOTE_CACHER_MODULES ?= cache nosql session queue
GO_TEST_REMOTE_CACHER_PACKAGES ?= $(addprefix forgejo.org/modules/,$(REMOTE_CACHER_MODULES))
@ -159,9 +156,6 @@ GO_SOURCES += $(shell find $(GO_DIRS) -type f -name "*.go" ! -path modules/optio
GO_SOURCES += $(GENERATED_GO_DEST)
GO_SOURCES_NO_BINDATA := $(GO_SOURCES)
ifeq ($(HAS_GO), yes)
MIGRATION_PACKAGES := $(shell $(GO) list forgejo.org/models/migrations/... forgejo.org/models/forgejo_migrations/...)
endif
ifeq ($(filter $(TAGS_SPLIT),bindata),bindata)
GO_SOURCES += $(BINDATA_DEST)
@ -285,6 +279,24 @@ show-version-minor: verify-version
show-version-api: verify-version
@echo ${FORGEJO_VERSION_API}
###
# Package computation targets
###
# Target to compute GO_TEST_PACKAGES - only runs when needed
.PHONY: compute-go-test-packages
compute-go-test-packages:
ifeq ($(HAS_GO), yes)
$(eval GO_TEST_PACKAGES ?= $(filter-out $(shell $(GO) list forgejo.org/models/migrations/...) $(shell $(GO) list forgejo.org/models/forgejo_migrations/...) forgejo.org/tests/integration/migration-test forgejo.org/tests forgejo.org/tests/integration forgejo.org/tests/e2e,$(shell $(GO) list ./...)))
endif
# Target to compute MIGRATION_PACKAGES - only runs when needed
.PHONY: compute-migration-packages
compute-migration-packages:
ifeq ($(HAS_GO), yes)
$(eval MIGRATION_PACKAGES := $(shell $(GO) list forgejo.org/models/migrations/... forgejo.org/models/forgejo_migrations/...))
endif
###
# Check system and environment requirements
###
@ -525,7 +537,7 @@ watch-backend: go-check
test: test-frontend test-backend
.PHONY: test-backend
test-backend:
test-backend: | compute-go-test-packages
@echo "Running go test with $(GOTESTFLAGS) -tags '$(TEST_TAGS)'..."
@TZ=UTC $(GOTEST) $(GOTESTFLAGS) -tags='$(TEST_TAGS)' $(GO_TEST_PACKAGES)
@ -555,7 +567,7 @@ test-check:
fi
.PHONY: test\#%
test\#%:
test\#%: | compute-go-test-packages
@echo "Running go test with $(GOTESTFLAGS) -tags '$(TEST_TAGS)'..."
@TZ=UTC $(GOTEST) $(GOTESTFLAGS) -tags='$(TEST_TAGS)' -run $(subst .,/,$*) $(GO_TEST_PACKAGES)
@ -573,10 +585,10 @@ coverage-show-html: coverage-convert
coverage-show-percentage: coverage-convert
go tool cover -func=coverage/textfmt.out
coverage-run:
coverage-run: | compute-go-test-packages
contrib/coverage-helper.sh test_packages $(COVERAGE_TEST_PACKAGES)
coverage-run-%: generate-ini-%
coverage-run-%: generate-ini-% | compute-migration-packages
#
# Migration tests go first
#
@ -755,7 +767,7 @@ migrations.sqlite.test: $(GO_SOURCES) generate-ini-sqlite
GITEA_ROOT="$(CURDIR)" GITEA_CONF=tests/sqlite.ini $(GOTESTCOMPILEDRUNPREFIX) ./migrations.sqlite.test $(GOTESTCOMPILEDRUNSUFFIX)
.PHONY: migrations.individual.mysql.test
migrations.individual.mysql.test: $(GO_SOURCES)
migrations.individual.mysql.test: $(GO_SOURCES) | compute-migration-packages
for pkg in $(MIGRATION_PACKAGES); do \
GITEA_ROOT="$(CURDIR)" GITEA_CONF=tests/mysql.ini $(GOTEST) $(GOTESTFLAGS) -tags '$(TEST_TAGS)' $$pkg || exit 1; \
done
@ -765,7 +777,7 @@ migrations.individual.sqlite.test\#%: $(GO_SOURCES) generate-ini-sqlite
GITEA_ROOT="$(CURDIR)" GITEA_CONF=tests/sqlite.ini $(GOTEST) $(GOTESTFLAGS) -tags '$(TEST_TAGS)' forgejo.org/models/migrations/$*
.PHONY: migrations.individual.pgsql.test
migrations.individual.pgsql.test: $(GO_SOURCES)
migrations.individual.pgsql.test: $(GO_SOURCES) | compute-migration-packages
for pkg in $(MIGRATION_PACKAGES); do \
GITEA_ROOT="$(CURDIR)" GITEA_CONF=tests/pgsql.ini $(GOTEST) $(GOTESTFLAGS) -tags '$(TEST_TAGS)' $$pkg || exit 1;\
done
@ -775,7 +787,7 @@ migrations.individual.pgsql.test\#%: $(GO_SOURCES) generate-ini-pgsql
GITEA_ROOT="$(CURDIR)" GITEA_CONF=tests/pgsql.ini $(GOTEST) $(GOTESTFLAGS) -tags '$(TEST_TAGS)' forgejo.org/models/migrations/$*
.PHONY: migrations.individual.sqlite.test
migrations.individual.sqlite.test: $(GO_SOURCES) generate-ini-sqlite
migrations.individual.sqlite.test: $(GO_SOURCES) generate-ini-sqlite | compute-migration-packages
for pkg in $(MIGRATION_PACKAGES); do \
GITEA_ROOT="$(CURDIR)" GITEA_CONF=tests/sqlite.ini $(GOTEST) $(GOTESTFLAGS) -tags '$(TEST_TAGS)' $$pkg || exit 1; \
done