Use whitelist to find go files, run find only once

[silverwind]

• Use a whitelist-based approach to find *.go files so we can use standard find syntax. Also included is a change that files no longer pass a leading './' which should help performance further.

• Instead of running find multiple times in make because of the lazy evaluation, run it only once and add the bindata files when the bindata tag is present

This is another huge speedup on my machine of around 2000%.

[silverwind]

@guillep2k this should solve what you mentioned yesterday.

What's interesting is that even if I exclude the bindata files from GO_SOURCES, it still seems to be able to build in the bindata. To be sure they are still in the list, I did some extra work to have them included when the tag is present.

[codecov-io]

Codecov Report

Merging #10594 into master will decrease coverage by 0.02%.
The diff coverage is n/a.

@@            Coverage Diff             @@
##           master   #10594      +/-   ##
==========================================
- Coverage   43.62%    43.6%   -0.03%     
==========================================
  Files         588      588              
  Lines       82474    82483       +9     
==========================================
- Hits        35980    35966      -14     
- Misses      42029    42055      +26     
+ Partials     4465     4462       -3

Impacted Files	Coverage Δ
models/repo_mirror.go	2.12% <0%> (-10.64%)	⬇️
services/pull/patch.go	61.93% <0%> (-2.59%)	⬇️
services/pull/temp_repo.go	29.05% <0%> (-2.57%)	⬇️
services/pull/check.go	53.04% <0%> (-2.44%)	⬇️
modules/queue/unique_queue_disk_channel.go	53.84% <0%> (-1.93%)	⬇️
modules/queue/workerpool.go	56.93% <0%> (-1.07%)	⬇️
modules/log/event.go	64.61% <0%> (-1.03%)	⬇️
services/mirror/mirror.go	18.75% <0%> (-0.66%)	⬇️
models/migrations/v130.go	0% <0%> (ø)	⬆️
services/pull/pull.go	35.88% <0%> (+0.88%)	⬆️
... and 2 more

---

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 123d386...4f4d5a5. Read the comment docs.

[guillep2k]

@guillep2k this should solve what you mentioned yesterday.

What's interesting is that even if I exclude the bindata files from GO_SOURCES, it still seems to be able to build in the bindata. To be sure they are still in the list, I did some extra work to have them included when the tag is present.

It's because the go compiler compiles the whole package. Including them in the source list is only needed for change detection.

[silverwind]

One more potential point for optimization is PACKAGES which is also lazy-evaluated. Any pointers if this can be made one-time as well?

[guillep2k]

One more potential point for optimization is PACKAGES which is also lazy-evaluated. Any pointers if this can be made one-time as well?

I don't think we need to have it dynamic (but keep on reading). AFAIU, the only reason for keeping it "dynamic" would be to support a forward reference. Consider the following:

GO_SOURCES = $(shell find $(SOURCE_DIRS) -name "*.go" -print)

[...]

SOURCE_DIRS = something something something

[...]

goexe: $(GO_SOURCES)
    ....

Here GO_SOURCES needs to be evaluated later because SOURCE_DIRS is not defined at the point GO_SOURCES is declared. If you'd use :=, then find would have no directory list and the results would be invalid.

I don't think that's the situation with PACKAGES.

Having said that, $(PACKAGES) seems to be used only within specific recipes (i.e. not for rules), like vet or test, so unless we're executing more than one of those rules in the same run, there will probably be no benefit in changing the definition.

[silverwind]

Right, PACKAGES can be handled in another PR. It does seem like a expensive operation so should bring more speed improvements.

[silverwind]

@guillep2k your example contains the anti-pattern of using a variable before it was defined. I would not allow such usage, even with lazy evaluation.

Maybe there are linters for Makefiles that could catch such anti-patterns, a quick search leads me to https://github.com/mrtazz/checkmake.

[guillep2k]

@guillep2k your example contains the anti-pattern of using a variable before it was defined. I would not allow such usage, even with lazy evaluation.

It was just a (lame) example, but quite a useful pattern in make, because then you've got a find functionality and many places where you can use it in different scenarios. We're using only one Makefile, but there are build systems with 10s or 100s that chain up and down. As with C++, you will need "foward declarations" at some point. 😄

[silverwind]

@jolheiser it seems your review was requested. Would you mind?

[jolheiser]

LGTM

[guillep2k]

Is this error coming from the PR itself or is it a glitch?

https://drone.gitea.io/go-gitea/gitea/22479/2/10

Although it only failed in MySQL builds, sounds Makefiley to me.

I'm restarting the CI just in case.

[silverwind]

contrib is not included in GO_DIRS. I assume it should?

[silverwind]

Added contrib which should get rid of these warnings.

[silverwind]

Hmm that didn't do anything to the warnings. The warnings don't seem come from this PR. I see them on https://drone.gitea.io/go-gitea/gitea/22489/2/10 too.

contrib was included in the build before this change, but I deemed it unneccesary. It does contain three go files:

contrib/pr/checkout.go
contrib/fixtures/fixture_generation.go
contrib/environment-to-ini/environment-to-ini.go

Should I remove it again?

[guillep2k]

I don't think /contrib should be part of the main build. There's a pr\#%: rule that uses it, though, but that's for manual invocation only.

[silverwind]

ok, reverted

[silverwind]

rebased, good to go

[guillep2k]

Ping LG-TM