Makefile: refactor 'woke', wrap longer lines #86
Conversation
|
@lblythen adding a |
Makefile
Outdated
| rm -rf $(VENVDIR) | ||
| @{ echo "Removing \"woke\" snap... \n"; sudo snap remove woke; } |
There was a problem hiding this comment.
No, we can't do that - I have woke already installed on my system, clean shouldn't remove it.
There's probably no way to install woke locally somehow in case it's not installed globally, or is there? That would be cleaner, and then it could of course also be removed afterwards.
There was a problem hiding this comment.
Makes sense. I believe this is the answer:
https://snapcraft.io/docs/parallel-installs
Do you like this approach?
There was a problem hiding this comment.
In general yes - but we can't reconfigure snapd and reboot the machine. :(
There was a problem hiding this comment.
Maybe @degville has some suggestion here?
There was a problem hiding this comment.
@ru-fu Perhaps we can drop the remove part for now?
There was a problem hiding this comment.
Sure - the other changes all look good to me. :)
|
I'd like to see use improve the initial descriptions in some of these PRs. |
|
@pmatulis can you please elaborate? |
|
@akcano In the initial PR's description field we should describe the PR. |
akcano
changed the title
|
@pmatulis I added some details; looks better now? |
Makefile
Outdated
| @test ! -e "$(VENVDIR)" -o -d "$(VENVDIR)" \ | ||
| -a "$(abspath $(VENVDIR))" != "$(VENVDIR)" | ||
| rm -rf $(VENVDIR) |
There was a problem hiding this comment.
If "$(VENVDIR)" doesn't exist, ! -e "$(VENVDIR)" will be TRUE. Short-circuit evaluation will yield TRUE for the whole @test statement, so make will continue instead of quitting: rm -rf $(VENVDIR) will be performed on a non-existent $VENVDIR.
Otherwise make clean's working because:
- VENVDIR's set to
.sphinx/venvwhich generally exists whenmake cleanis run - Therefore
! -e "$(VENVDIR)"is FALSE, but there's a -o expression which @test therefore must evaluate -d "$(VENVDIR"is TRUE (.sphinx/venvis a directory), but there's a -a expression which must also be evaluated"$(abspath $(VENVDIR))"generally isn't identical to"$(VENVDIR)", so the -a expression is TRUE.- FALSE OR TRUE AND TRUE yields TRUE, so make continues:
rm -rf $(VENVDIR)is performed.
It's good practice to wrap lines in a way that emphasises operator precedence rather than disguising it - it makes errors easier to spot. At present:
@test ! -e "$(VENVDIR)" -o -d "$(VENVDIR)" \
-a ...
keeps two expressions together with -o between them and splits the line between two expressions associated through -a, as if -o has higher precedence than -a. Ignoring problems with the logic, a representation less prone to being misinterpreted would be:
@test ! -e "$(VENVDIR)" -o \
-d "$(VENVDIR)" -a "$(abspath $(VENVDIR))" != "$(VENVDIR)"
There was a problem hiding this comment.
Personally, I think trying to delete a non-existent directory is not an issue at this moment. Technically, it's a no-op; we could stop to report instead of silently falling through, yes, but that goes beyond the scope of Makefile's current state. After all, this simply preserves current behaviour but adds safeguards.
Regarding the line wrapping: you're right, this reads better.
This aims to extract the installation logic of the 'make woke' target into other targets while making the process more explicit for the user with echo commands.
Some of the longer recipe lines affected by this could benefit from wrapping, so a related change is added as a follow-up.