Makefile: use XZ_OPT=-0 for make vm
#495
Conversation
|
|
Makefile
Outdated
| prepare-check: $(NODE_MODULES_TEST) $(VM_IMAGE) test/common | ||
| prepare-check: $(NODE_MODULES_TEST) vm test/common |
There was a problem hiding this comment.
Please don't depend on a phony target. This was deliberately done with VM_IMAGE so that repeated make checks don't always rebuild the VM (which is annoying). I.e. revert this and the hunk above.
There was a problem hiding this comment.
This will depend on the .PHONY target, which will always call make on the $(VM_IMAGE) target. but that will be a no-op if nothing changed. I tested this. :)
There was a problem hiding this comment.
I added a note about this to the commit message.
There was a problem hiding this comment.
So what's the advantage of doing it like this? It feels weird that you introduce an extra (useless) make call in a PR which is supposed to make things faster.
There was a problem hiding this comment.
It's about giving me a way to set the environment variable.
Makefile
Outdated
| vm: $(VM_IMAGE) | ||
| .PHONY: vm | ||
| vm: | ||
| XZ_OPT=-0 make "$(VM_IMAGE)" |
There was a problem hiding this comment.
This looks fine. Can you please send this to starter-kit as well?
There was a problem hiding this comment.
Was planning to after we sort this out here 👍
They're definitely related, and I'm not in a rush, so let's talk it out. |
...targets which depend on it, like `make check`. This shaves quite some seconds off of image building (42s instead of 1m36s). Most of that comes from compressing node_modules/ which seems like the obvious candidate to get rid of, but it's required by the build. We could work around that, but it would involve some deeper hacks, so let's make the easy change, instead.
|
Here's another approach that you might like better. It's less explicit, and I've seen weird behaviour with these sorts of |
allisonkarlitskaya
changed the title
make vmXZ_OPT=-0 for make vm
...and make
make checkgo viamake vmso that it benefits as well.This shaves quite some seconds off of image building (42s instead of 1m36s). Most of that comes from compressing node_modules/ which seems like the obvious candidate to get rid of, but it's required by the build. We could work around that, but it would involve some deeper hacks, so let's make the easy change, instead.