Code review - Issue 8686048: Add a Makefile npm-cache target to upload to LPhttps://codereview.appspot.com/2013-04-30T15:27:25+00:00rietveld
Message from unknown
2013-04-29T19:08:56+00:00benjiurn:md5:17965690f6ee4fe541a0db3dc8dc8a2c
Message from benji.york@gmail.com
2013-04-29T19:09:01+00:00benjiurn:md5:57966b799cc8608bf72dbdab42ff4f26
Please take a look.
Message from rharding@mitechie.com
2013-04-29T19:15:13+00:00rhardingurn:md5:c826860c341bd75ed6072293ef5aa44d
LGTM very cool use of make to push updates to the cache.
Message from gary.poster@canonical.com
2013-04-29T20:18:34+00:00gary.posterurn:md5:e54aa92a6b1ff13ed135b0b7a383abf1
LGTM with responses--changes preferred, but I'm open to counterarguments instead--to comments.
Thank you! This is cool to have.
Gary
https://codereview.appspot.com/8686048/diff/1/Makefile
File Makefile (right):
https://codereview.appspot.com/8686048/diff/1/Makefile#newcode98
Makefile:98: NPM_CACHE_VERSION=$(shell date +%s)# Seconds since the epoch.
I would prefer to connect this to the revid, since we have shrinkwrap now. Is there a reason to prefer the date?
https://codereview.appspot.com/8686048/diff/1/Makefile#newcode553
Makefile:553: $(MAKE) clean-all
We really have to do that even if you specify a different cache location in an environment variable? Yuck. :-(
https://codereview.appspot.com/8686048/diff/1/Makefile#newcode591
Makefile:591: main-doc npm-cache npm-cache-file npm-cache-file-signature prep prod \
I wish you had divided things up the way you describe here--or at least the file and the npm-cache. Would you be willing to add those in? If not, clean these out.
Message from benji.york@gmail.com
2013-04-29T20:38:49+00:00benjiurn:md5:e0525b19661c8cd09159c7fd6b1e9012
Good points. I haven't taken action yet, pending discussion.
https://codereview.appspot.com/8686048/diff/1/Makefile
File Makefile (right):
https://codereview.appspot.com/8686048/diff/1/Makefile#newcode98
Makefile:98: NPM_CACHE_VERSION=$(shell date +%s)# Seconds since the epoch.
On 2013/04/29 20:18:34, gary.poster wrote:
> I would prefer to connect this to the revid, since we have shrinkwrap now. Is
> there a reason to prefer the date?
The date is ever so slightly more reliable (say someone builds a cache from a branch, that revno will be wrong) but it may not be worth worrying about. I'm open to switching to revno. Also see the multi-target discussion on your comment on the .PHONY targets.
https://codereview.appspot.com/8686048/diff/1/Makefile#newcode553
Makefile:553: $(MAKE) clean-all
On 2013/04/29 20:18:34, gary.poster wrote:
> We really have to do that even if you specify a different cache location in an
> environment variable? Yuck. :-(
We do because NPM will notice that everything has been installed and not bother to fetch anything if we don't remove the installed packages first.
https://codereview.appspot.com/8686048/diff/1/Makefile#newcode591
Makefile:591: main-doc npm-cache npm-cache-file npm-cache-file-signature prep prod \
On 2013/04/29 20:18:34, gary.poster wrote:
> I wish you had divided things up the way you describe here--or at least the file
> and the npm-cache. Would you be willing to add those in? If not, clean these
> out.
It turned out that there were some interactions of using the time for the version number and recursively calling make that made it easier to just do it all in one command. If we switch to revno, then it would be easy to break out.
Message from unknown
2013-04-30T14:58:46+00:00benjiurn:md5:7cc47c91ef1a05afb029055f718e5e2e
Message from benji.york@gmail.com
2013-04-30T14:58:47+00:00benjiurn:md5:ccd2f3fc5678f4a73be1c0301d6a6ef8
Please take a look.
Message from unknown
2013-04-30T15:22:56+00:00benjiurn:md5:aa89fdb42d929c8247a23fa1048da9da
Message from benji.york@gmail.com
2013-04-30T15:27:25+00:00benjiurn:md5:e9d89117b00c37051e3508b98eb9f438
*** Submitted:
Add a Makefile npm-cache target to upload to LP
There is a parallel charm branch that uses the cache file to speed deploying
of branch builds.
R=rharding, gary.poster
CC=
https://codereview.appspot.com/8686048