Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add the bindata source as a target to the Makefile #12367

Conversation

zeripath
Copy link
Contributor

make backend fails if parallelism is turned on with make -jx.

This happens because when the executable starts to be built the sources
for the bindata files are not available.

If however, we add a target that makes the sources for the bindata -
i.e. the generate target - then parallelism will work again.

References: #11756

Signed-off-by: Andrew Thornton art27@cantab.net

`make backend` fails if parallelism is turned on with `make -jx`.

This happens because when the executable starts to be built the sources
for the bindata files are not available.

If however, we add a target that makes the sources for the bindata -
i.e. the generate target - then parallelism will work again.

Signed-off-by: Andrew Thornton <art27@cantab.net>
@zeripath zeripath added type/bug topic/build PR changes how Gitea is built, i.e. regarding Docker or the Makefile backport/v1.12 labels Jul 29, 2020
@zeripath zeripath added this to the 1.13.0 milestone Jul 29, 2020
Makefile Outdated Show resolved Hide resolved
@GiteaBot GiteaBot added the lgtm/need 2 This PR needs two approvals by maintainers to be considered for merging. label Jul 29, 2020
Makefile Outdated Show resolved Hide resolved
@silverwind
Copy link
Member

silverwind commented Jul 29, 2020

How about doing it the other way around and making a proper dependency chain? I think it'd be more in line with our other targets like frontend where that target is a simple phony target that has the actual files as dependencies.

.PHONY: generate
generate: $(BINDATA_DEST)

$(BINDATA_DEST): $(TAGS_PREREQ)

@zeripath
Copy link
Contributor Author

Because generate does not and should not only produce BINDATA

@silverwind
Copy link
Member

It should not matter if we specify all generated files or not. For example WEBPACK_DEST also only specifies two files while actually generating a lot more. Make will work fine if there's at least one correct target file.

… finished

Signed-off-by: Andrew Thornton <art27@cantab.net>
@zeripath
Copy link
Contributor Author

What we do not want is a situation whereby generate will only run if you are using bindata.

Signed-off-by: Andrew Thornton <art27@cantab.net>
@silverwind
Copy link
Member

generate is phony so will always run. You can even put it into $(EXECUTABLE), it would have the same effect as it does now.

Signed-off-by: Andrew Thornton <art27@cantab.net>
@zeripath
Copy link
Contributor Author

Damn we have to use .NOTPARALLEL

@silverwind
Copy link
Member

Can we not just put the generate command into $(EXECUTABLE)? That will ensure it'll always run correctly, even when parallel I think.

@silverwind
Copy link
Member

Ah wait, no that can't work because of the release tasks which expect generate to have ran.

@@ -526,9 +526,12 @@ frontend: node-check $(FOMANTIC_DEST) $(WEBPACK_DEST)
backend: go-check generate $(EXECUTABLE)

.PHONY: generate
.NOTPARALLEL: generate
Copy link
Member

@silverwind silverwind Jul 29, 2020

Choose a reason for hiding this comment

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

From what I gather .NOTPARALLEL does not take any arguments, just plain .NOTPARALLEL: anywhere in the makefile should disable parallelism for the whole makefile.

@silverwind
Copy link
Member

Actually, .NOTPARALLEL is a no-go imho. It would prevent useful stuff like

make -j2 watch-frontend watch-backend

@silverwind
Copy link
Member

silverwind commented Jul 29, 2020

So tar preserves 1-second precision timestamp on extraction, which is good enough for make to not trigger a frontend rebuild. It's also seen by running this from a src tarball extraction:

$ make webpack
make: Nothing to be done for 'webpack'.
$ make fomantic
make: Nothing to be done for 'fomantic'.

So I think the best solution is adding frontend to generate dependencies and move node-check to targets $(FOMANTIC_DEST) $(WEBPACK_DEST). It should only actually run the node build if web_src files are modified.

@silverwind
Copy link
Member

silverwind commented Jul 29, 2020

So I got some success with below changes but there's no node-check in the chain anymore (it seems to trigger builds when its there). Note I reduced fomantic/webpack tracking files to 1 so make -j2 fomantic does not that build run twice in parallel.

diff --git a/Makefile b/Makefile
index 890332fde..baa54ec1e 100644
--- a/Makefile
+++ b/Makefile
@@ -89,14 +89,14 @@ LDFLAGS := $(LDFLAGS) -X "main.MakeVersion=$(MAKE_VERSION)" -X "main.Version=$(G

 GO_PACKAGES ?= $(filter-out code.gitea.io/gitea/integrations/migration-test,$(filter-out code.gitea.io/gitea/integrations,$(shell $(GO) list -mod=vendor ./... | grep -v /vendor/)))

 FOMANTIC_CONFIGS := semantic.json web_src/fomantic/theme.config.less web_src/fomantic/_site/globals/site.variables
-FOMANTIC_DEST := web_src/fomantic/build/semantic.js web_src/fomantic/build/semantic.css
+FOMANTIC_DEST := web_src/fomantic/build/semantic.js
 FOMANTIC_DEST_DIR := web_src/fomantic/build

 WEBPACK_SOURCES := $(shell find web_src/js web_src/less -type f) $(FOMANTIC_DEST)
 WEBPACK_CONFIGS := webpack.config.js
-WEBPACK_DEST := public/js/index.js public/css/index.css
+WEBPACK_DEST := public/js/index.js
 WEBPACK_DEST_ENTRIES := public/js public/css public/fonts public/img/webpack public/serviceworker.js

 BINDATA_DEST := modules/public/bindata.go modules/options/bindata.go modules/templates/bindata.go
 BINDATA_HASH := $(addsuffix .hash,$(BINDATA_DEST))
@@ -519,18 +519,18 @@ install: $(wildcard *.go)
 .PHONY: build
 build: frontend backend

 .PHONY: frontend
-frontend: node-check $(FOMANTIC_DEST) $(WEBPACK_DEST)
+frontend: $(FOMANTIC_DEST) $(WEBPACK_DEST)

 .PHONY: backend
-backend: go-check generate $(EXECUTABLE)
+backend: go-check $(EXECUTABLE)

 .PHONY: generate
-generate: $(TAGS_PREREQ)
+generate: frontend $(TAGS_PREREQ)
 	CC= GOOS= GOARCH= $(GO) generate -mod=vendor -tags '$(TAGS)' $(GO_PACKAGES)

-$(EXECUTABLE): $(GO_SOURCES) $(TAGS_PREREQ)
+$(EXECUTABLE): generate $(GO_SOURCES) $(TAGS_PREREQ)
 	CGO_CFLAGS="$(CGO_CFLAGS)" $(GO) build -mod=vendor $(GOFLAGS) $(EXTRA_GOFLAGS) -tags '$(TAGS)' -ldflags '-s -w $(LDFLAGS)' -o $@

 .PHONY: release
 release: frontend generate release-windows release-linux release-darwin release-copy release-compress release-sources release-docs release-check

@silverwind
Copy link
Member

silverwind commented Jul 30, 2020

Regarding node-check and go-check, maybe they can be made a simple if condition that checks supplied make arguments so it won't mess up dependency tracking:

NODE_GOALS: frontend webpack fomantic

ifeq ($(filter $(NODE_GOALS),$(MAKECMDGOALS)),)
  # do node check
endif

@silverwind
Copy link
Member

I'm questioning if it's really worth it to introduce the frontend dependency in the backend chain. For example the dev setup recommends frontend/backend ran in two tabs. Now if we have the dependency frontend will build in the backend tab if its unclean and then again in the frontend tab.

I think we're better off with a recommendation to not enable make parallism in README.md for the regular builds. There's nothing gained from enabling it anyways because all our steps depend on each other.

silverwind added a commit to silverwind/gitea that referenced this pull request Jul 30, 2020
@zeripath zeripath closed this Jul 30, 2020
techknowlogick pushed a commit that referenced this pull request Jul 30, 2020
@zeripath zeripath deleted the add-bindata-source-targets-to-makefile branch September 13, 2020 12:31
@go-gitea go-gitea locked and limited conversation to collaborators Nov 24, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
lgtm/need 2 This PR needs two approvals by maintainers to be considered for merging. topic/build PR changes how Gitea is built, i.e. regarding Docker or the Makefile type/bug
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants