Skip to content

Polishes makefile#388

Merged
codefromthecrypt merged 2 commits intomasterfrom
make-polish
Oct 6, 2021
Merged

Polishes makefile#388
codefromthecrypt merged 2 commits intomasterfrom
make-polish

Conversation

@codefromthecrypt
Copy link
Copy Markdown
Contributor

@codefromthecrypt codefromthecrypt commented Oct 6, 2021

  • tidies how we recursively detect sources
  • makes lint and formatting conditional on source change
  • fixes redundant slash in e2e default path
  • makes formatting consistent
  • makes non-overridable variable expansion immediate, while keeping macros deferred

Copy link
Copy Markdown
Contributor Author

@codefromthecrypt codefromthecrypt left a comment

Choose a reason for hiding this comment

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

notes

Comment thread Makefile Outdated
Comment thread Makefile
format:
@printf "$(ansi_format_dark)" format "formatting project files"
# format is a PHONY target, so always runs. This allows skipping when sources didn't change.
build/format: go.mod $(all_sources)
Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

format and lint are some of the most expensive parts of the build, and we'd run them always even if sources didn't change before.

Comment thread Makefile
@$(go) run $(golangci_lint) run --timeout 5m --config .golangci.yml ./...
@$(MAKE) build/lint
@# this will taint if we are behind from latest binary. printf avoids adding a newline to the file
@curl -fsSL https://archive.tetratelabs.io/envoy/envoy-versions.json |jq -er .latestVersion|xargs printf "%s" \
Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

notably, we always check for latest envoy version as that's not implied by local code.

Comment thread Makefile

# Excludes all *_test.go files.
gocodes := [!_test].go
sources := $(wildcard internal/*/*$(gocodes) internal/*/*/*$(gocodes))
Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

this was a little loose as it could get sources in the "test" or "testdata" directories (aren't actually used in main)

Comment thread Makefile
gocodes := [!_test].go
sources := $(wildcard internal/*/*$(gocodes) internal/*/*/*$(gocodes))
# Make 3.81 doesn't support '**' globbing: Set explicitly instead of recursion.
all_patterns := *.go */*.go */*/*.go */*/*/*.go */*/*/*.go */*/*/*/*.go
Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

if we go past this directory depth I would probably be upset for other reasons :D

Comment thread Makefile
@$(go) mod tidy
@$(go) run $(licenser) apply -r "Tetrate"
@$(go)fmt -s -w $$(find . -type f -name '*.go')
@$(go)fmt -s -w $(all_sources)
Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

much better than loose shelling out

@codefromthecrypt codefromthecrypt marked this pull request as ready for review October 6, 2021 19:59
* tidies how we recursively detect sources
* makes lint and formatting conditional on source change
* stops requiring gcc for e2e tests
* makes formatting consistent
* makes non-overridable variable expansion immediate, while keeping macros deferred

Signed-off-by: Adrian Cole <adrian@tetrate.io>
Signed-off-by: Adrian Cole <adrian@tetrate.io>
@codefromthecrypt
Copy link
Copy Markdown
Contributor Author

I'm going to merge this due to holiday schedules, but welcome review feedback later!

@codefromthecrypt codefromthecrypt merged commit 617162f into master Oct 6, 2021
@codefromthecrypt codefromthecrypt deleted the make-polish branch October 6, 2021 23:08
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant