-
Notifications
You must be signed in to change notification settings - Fork 8
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
Chore refactor makefile to reduce repetition #211
base: master
Are you sure you want to change the base?
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks fine overall but there are a few cosmetic issues and not all phony targets are declared as .PHONY
.
BTW, if you've created a new pull request you should close #199 (and drop a comment saying that it has been superseded by a new pull request).
containers/Makefile
Outdated
verifier-clean: | ||
docker rmi $(IMAGE_PREFIX)verifier:$(IMAGE_VERSION) --force | ||
%-rmi: | ||
docker rmi $(IMAGE_PREFIX)$*:$(IMAGE_VERSION) --force |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why two tabs?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
fixed
@@ -95,6 +95,7 @@ $(IMAGE_TARGETS): %: %!dependencies build/ | |||
|
|||
docker-clean: nginx-proxy-rmi nginx-storage-rmi postgresql-rmi concent-api-rmi verifier-rmi | |||
|
|||
.PHONY: %-rmi | |||
%-rmi: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please put it at the bottom of the file with all the other .PHONY
targets.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done
|
||
verifier-clean: | ||
docker rmi $(IMAGE_PREFIX)verifier:$(IMAGE_VERSION) --force | ||
%-rmi: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You'e renaming the targets here but the old names are still listed at the bottom of the file where they're declared .PHONY
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
old .PHONY targets removed
containers/Makefile
Outdated
$(VERIFIER_SOURCE) \ | ||
build/repositories/golem/ \ | ||
build/verifier/golem/scripts/ \ | ||
build/verifier/golem/imgverifier-requirements.txt |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why two tabs?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
most of the file lines uses 4 spaces as indent (55 occurences), some one tab (52 occurences) some two tabs (29 occurences) and some two spaces (3 occurences). I used two tabs as in mine editor tab=two spaces. To make it consistent, I propose to change every indent to one tab.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Have you seen #211 (comment)?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yes, replied and fixed
containers/Makefile
Outdated
verifier!dependencies: \ | ||
$(VERIFIER_SOURCE) \ | ||
build/repositories/golem/ \ | ||
build/verifier/golem/scripts/ \ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please fix the whitespace here.
- You're inserting tabs everywhere. Even inside the text (e.g.
build/verifier/golem
and\
). We're using spaces everywhere, please use them too. Otherwise the code looks very misaligned.- The only place where we use tabs is at the beginning of a line containing a bash command in a target. And that's only because make syntax requires it.
- It's fine to add a little whitespace before
\
but this is way too much. There's so much that I have to scroll the diff horizontally to see that there's\
at the end of some lines. \
should all be either in the same column (preferred) or immediately after the text.
I have just realized that we don't have this written down in our style yet so I have added a new proposal. Please see Code-Poets/coding-style#16
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yeah, the problem is with different rendering engine in github and atom, apparently tabs in atom does not have fixed length, and slashes appeared to be alligned, definetely within screen width
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
excessive tabs replaced with spaces
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Still some cosmetic changes needed.
Also, please squash the commits related to .PHONY
targets into one. It's actually a missing part of the commit that removed those targets, not an independent change.
containers/Makefile
Outdated
"build/repositories/concent/" \ | ||
"$(abspath package-builder)" \ | ||
"$(abspath build)" \ | ||
"$(dir $@)" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You have replaced all indents with 2-space ones. As you can see from the diff, we're already using 4-space indents. Changing it in the middle of an unrelated pull request is a bad idea.
Please keep the original indentation. Just fix the code you have added.
I've updated the issue about coding style to make it clear that we're using 4-space indents.
containers/Makefile
Outdated
"$(abspath build)" \ | ||
"$(dir $@)" | ||
build/package-builder/build-concent-api-docs.sh\ | ||
"build/repositories/concent/" \ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's a good practice to always leave a space before \
. Otherwise it's easy to run into problems like this:
cat\
a.txt\
b.txt\
c.txt
bash: cata.txtb.txtc.txt: command not found
That's because bash does not add spaces on its own and sees your command as cata.txtb.txtc.txt
rather than cat a.txt b.txt c.txt
.
There is some whitespace at the beginning of the line in this case which would prevent this problem but that's not always the case so, as a rule, we always add at least one space at the end.
I've updated the issue for updating coding style to make it clear.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, this is good remark. I added space before each "".
containers/Makefile
Outdated
verifier verifier-clean \ | ||
docker-clean \ | ||
clean \ | ||
%-rmi |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Are you sure this works? I think that pattern rules will only work if %
is in the target as well. But I may be wrong. Please check.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That is correct, I did some tests and .PHONY targets must be declared explicitly.
containers/Makefile
Outdated
--output "$@" | ||
|
||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please avoid unnecessary changes, like removing these empty lines.
I'm not saying that removing them is a bad idea but I don't want you and @bartoszbetka to get into a loop where you remove them and he adds them back :)
When editing an existing file please adapt to the style already used in it unless the issue is specifically about refactoring the style.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It was the only one occurrence of two empty lines in a row, so it looked more than correcting a mistake than imposing own coding style. Fully agree about adapting to existing style.
containers/Makefile
Outdated
"$(abspath package-builder)" \ | ||
"$(abspath build)" \ | ||
"$(dir $@)" | ||
"build/repositories/concent/" \ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not sure if having a mix of tabs and spaces at the beginning of a line is a good idea.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
according to coding style, I used tab where is needed by makefile syntax, and space everywhere else (shell arguments)
Could you look at the diff and ensure that there are no unnecessary trivial whitespace changes? |
new pull request due to wrong branch name, and comments in #199 ( particulary #199; #pullrequestreview-144854386 ). Solves issues found in previous PR as: separate commits, separate PR, chore in branch name, phony target and static pattern rules
Resolves: #193