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

Build: simplify how protos are built #4639

Merged
merged 5 commits into from
Nov 4, 2021
Merged

Build: simplify how protos are built #4639

merged 5 commits into from
Nov 4, 2021

Conversation

slim-bean
Copy link
Collaborator

What this PR does / why we need it:

Hopefully this helps with some flakes in our CI and issues seen locally trying to build Loki.

Instead of relying on the Make semantics of using timestamps to determine if a file should be recompiled just delete all the compiled protos every time.

Removed some dependant targets from the loki and loki canary targets, we don't need to rebuild these files to build Loki. if you are mucking around with protos or yacc files you will likely know you need to build them anyway.

Which issue(s) this PR fixes:
Fixes #

Special notes for your reviewer:

Checklist

  • Documentation added
  • Tests updated
  • Add an entry in the CHANGELOG.md about the changes.

…and Makes use of timestamps to determine if files should be recompiled. Instead of touching files and altering timestamps always delete the compiled proto files when calling `protos` to make sure they are compiled every time.

Removed targets to build protos, yaccs, and ragel files when trying to build loki or the canary. This isn't necesary, if you are changing these files you would know you need to build them and the `check-generated-files` should catch any changes to them not committed.
@slim-bean slim-bean requested a review from a team as a code owner November 3, 2021 20:58
…let that run first.

also removing `check-generated-files` from the `all` target because its redundant with a separate step and could also race with the parallel lint
@pull-request-size pull-request-size bot added size/L and removed size/M labels Nov 3, 2021
make('check-generated-files', container=false) { depends_on: ['clone'] },
make('test', container=false) { depends_on: ['clone','check-generated-files'] },
make('lint', container=false) { depends_on: ['clone','check-generated-files'] },
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

make test and lint depend on check-generated-files so they wait for it to finish as it will rebuild protos

loki: protos yacc ragel cmd/loki/loki
loki-debug: protos yacc ragel cmd/loki/loki-debug
loki: cmd/loki/loki
loki-debug: cmd/loki/loki-debug
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

we don't need to build protos, yacc, ragel to build loki, if someone is changing those things they will need to know how to run make protos, make yacc etc


%.pb.go: $(PROTO_DEFS)
%.pb.go:
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

depending on the proto definitions triggers Makes desire to compare timestamps of file before choosing to do anything, we always want to build protos every time.

# use with care. This signals to make that the proto definitions don't need recompiling.
touch-protos:
for proto in $(PROTO_GOS); do [ -f "./$${proto}" ] && touch "$${proto}" && echo "touched $${proto}"; done
protos: clean-protos $(PROTO_GOS)
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

delete all the compile protos so they will be guaranteed to be built every time this is called.

@slim-bean
Copy link
Collaborator Author

the TL;DR here is I changed the make protos to force build the protos every time it's called (by deleting the output pb.go files first) and I removed everywhere we were building protos except for check-generated-files

Doing this means we dont' care about any timestamps on the proto files or their generated files anymore so I removed all references to TOUCH_PROTOS and the like because we don't need this anymore

Copy link
Contributor

@cyriltovena cyriltovena left a comment

Choose a reason for hiding this comment

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

LGTM

@slim-bean slim-bean merged commit 2ac409c into main Nov 4, 2021
@slim-bean slim-bean deleted the change-proto-handling branch November 4, 2021 13:54
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants