use the Makefile.common#3978
Conversation
99c347c to
3a8a739
Compare
|
This is a good start. I think that the common Makefile should live in either the promu or prometheus repos, not in common which is library code. Downloading it on the fly will be fragile, I'd prefer that it get updated via tooling+PRs. |
|
the idea is to keep a local cached copy like I did in the prometheus PR , the What is your idea to keep it up to date? |
|
We send a batch of PRs every time it changes. That way we know that each repo is always buildable. |
|
a PR on every change of Makefile.common? what about that -f flag do you see any problem with that? |
Makefiles never went away, and each Makefile was still altered on an as-needed basis and things still drifted.
Changing behaviour depending on whether we get a HTTP response is fragile. |
|
your idea is to keep Makefile.common in lets say prometheus/prometheus and each time it needs a change to open a PR against each other prometheus repo ?
|
|
Yes.
Only using it sometimes is fragile and will lead to confusion. |
|
maybe adding one problem I am trying to avoid is merging PR's by mistakes on repos different than the main source of the Makefile.common which might happen quite often if Makefile.common gets addopted by many repos. |
People on flaky internet connections get different behaviour, and there's now N potential makefiles that could be in use depending on when it last worked. |
34940ce to
d501b15
Compare
|
as per the prometheus devs objection I have removed the auto-update https://groups.google.com/forum/#!topic/prometheus-developers/4734cvJHCn4 |
|
👍 |
|
@brian-brazil thanks next I will open issues for the other golang repos to start using the common targets. |
There was a problem hiding this comment.
I dislike making a fast internet connection a development requirement (by redownloading promu on every single make build execution).
I also don't like hiding the actually executed commands, as this means I first need to read the Makefile to understand how to build Prometheus without triggering the promu download.
Makefile.common
Outdated
Makefile.common
Outdated
There was a problem hiding this comment.
Not showing what is being executed and especially downloaded in a Makefile is something I consider an anti-pattern and has led to several questions and support overhead when these downloads hang/fail.
I don't understand why a line >> fetching promu is preferred over showing the user what we install directly (e.g. go get -u github.com/prometheus/promu). Not show the commands makes debugging very hard.
I'm not a fan either, but it's what the existing Makefile does so I don't think this is the PR to propose changing it. |
|
I agree that we should show the commands that are running so updated and will merge on green. the promu fetch will be left for another PR. |
I understood this PR to introduce the common Makefile which will be introduced in all projects, as it aims to resolve #3904. If this is not the final PR for that, feel free to merge. |
|
@grobie any suggestions or a link to another makefile that handles the promu fetch better? |
|
as of golang 1.9 the vendored dir is excluded so I updated it to just use |
|
client_golang still supports back to 1.7.x |
|
Though on the other hand client_golang doesn't have any vendoring. |
|
I thought of that , but client_golang doesn't have a |
|
Most of it won't be useful, but the things like tests apply. |
|
what it is the promise on the golang version for the client_golang? |
Makefile.common
Outdated
There was a problem hiding this comment.
The all target should abort incorrectly formatted code, instead of silently formatting it. Replace format with style.
There was a problem hiding this comment.
I think this should stay as-is as this is what a human will use. Maybe a separate CI target?
This is a matter for another PR in any case, we shouldn't be making changes here just refactoring.
There was a problem hiding this comment.
All other targets abort the command and require the user to fix it, format is the only one behaving differently. Introducing another target is unnecessary and complicates all CI integrations which commonly just execute make.
This PR is explicitely labeled to introduce the new common Makefile for all projects, so either that needs to be changed or this is the PR where I have to request such changes.
There was a problem hiding this comment.
This PR is explicitely labeled to introduce the new common Makefile for all projects, so either that needs to be changed or this is the PR where I have to request such changes.
This is just a refactoring. Once it is in changes can be made before it is propagated out.
|
Sorry for being idle on this conversation. I like the idea of having One idea, we could include this in our cicleci test pipeline. |
021f475 to
6968ba2
Compare
@SuperQ can you explain a bit more as I didn't quite understand the idea |
grobie
left a comment
There was a problem hiding this comment.
Approving given @brian-brazil says "This is just a refactoring.".
|
would need to merge this one first to pass the tests. |
|
@krasi-georgiev We run a number of tests automatically. We could add a test failure into the build setup to fail if the common makefile changes. This might be slightly annoying for unrelated updates, but it helps address @grobie's desire for offline development. (I agree with the desire for offline development) |
|
@SuperQ the original PR allowed offline mode, by using a cached version when no internet connection. |
brian-brazil
left a comment
There was a problem hiding this comment.
Changing things in a PR where you're mainly moving code around makes it hard to review, and increases the chances that a mistake/change will be missed. Keep this to moving things around, changes can be done in the next PR.
Makefile.common
Outdated
There was a problem hiding this comment.
This is changed from the original, don't change semantics in a refactoring PR
There was a problem hiding this comment.
completely agree in general.
These changes are small enough and I hope we can make an exception.
There was a problem hiding this comment.
All the small changes in this PR are rewriting most of the file. It'd be easier to review if things were kept simpler. At least split it into sane commits.
There was a problem hiding this comment.
I think I had them split logically, but I squashed few because of the DCO.
Now rebased with master and all tests look good.
Makefile.common
Outdated
There was a problem hiding this comment.
This is changed from the original
Signed-off-by: Krasi Georgiev <krasi.root@gmail.com>
Signed-off-by: Krasi Georgiev <krasi.root@gmail.com>
Signed-off-by: Krasi Georgiev <krasi.root@gmail.com>
198090d to
44b8cac
Compare
brian-brazil
left a comment
There was a problem hiding this comment.
This is still hard to review with mixing of refactoring and changes.
Makefile.common
Outdated
|
|
||
| unused: $(GOVENDOR) | ||
| @echo ">> running check for unused packages" | ||
| @$(GOVENDOR) list +unused | grep . && exit 1 || echo 'No unused packages' |
613fc4d to
62ed0e5
Compare
Signed-off-by: Krasi Georgiev <krasi.root@gmail.com>
62ed0e5 to
3977d8f
Compare
|
Reverted all mentioned changes. |
|
👍 |
|
if no one else has anymore suggestions merging tomorrow. |
|
|
||
| style: | ||
| @echo ">> checking code style" | ||
| ! $(GOFMT) -d $$(find . -path ./vendor -prune -o -name '*.go' -print) | grep '^' |
There was a problem hiding this comment.
yes at the moment the output is quite busy , but I will open another PR to simply ./...
Makefile.common
Outdated
|
|
||
| check_license: | ||
| @echo ">> checking license header" | ||
| for file in $$(find . -type f -iname '*.go' ! -path './vendor/*') ; do \ |
There was a problem hiding this comment.
The recipe doesn't fail when license headers are missing. I got it fixed as:
- for file in $$(find . -type f -iname '*.go' ! -path './vendor/*') ; do \
- awk 'NR<=3' $$file | grep -Eq "(Copyright|generated|GENERATED)" || echo "license header checking failed for:" $$file ; \
- done
+ @licRes=$$(for file in $$(find . -type f -iname '*.go' ! -path './vendor/*') ; do \
+ awk 'NR<=3' $$file | grep -Eq "(Copyright|generated|GENERATED)" || echo $$file; \
+ done); \
+ if [ -n "$${licRes}" ]; then \
+ echo "license header checking failed:"; echo "$${licRes}"; \
+ exit 1; \
+ fiThere was a problem hiding this comment.
yep , thanks , updated.
|
|
||
| style: | ||
| @echo ">> checking code style" | ||
| ! $(GOFMT) -d $$(find . -path ./vendor -prune -o -name '*.go' -print) | grep '^' |
There was a problem hiding this comment.
yes at the moment the output is quite busy , but I will open another PR to simply ./...
Signed-off-by: Krasi Georgiev <krasi.root@gmail.com>
19c3042 to
f1c8709
Compare
split common targets in a Makefile.common to reuse it across projects Signed-off-by: Krasi Georgiev <krasi.root@gmail.com>
fixes #3904
depends on prometheus/common#121
cc @brian-brazil