Make `make` require only one param to run and add 'serve' alias (issue 5651086)

[tech...@gmail.com]

Reviewers: Andi Albrecht,

Description:
Make `make` require only one param to run and add 'serve' alias

Please review this at http://codereview.appspot.com/5651086/

Affected files:
M Makefile

Index: Makefile
===================================================================
--- a/Makefile
+++ b/Makefile
@@ -1,15 +1,17 @@
# Makefile to simplify some common AppEngine actions.
# Use 'make help' for a list of commands.

-DEV_APPSERVER?= dev_appserver.py
+SDK_PATH ?= .
+
+DEV_APPSERVER?= $(SDK_PATH)/dev_appserver.py
DEV_APPSERVER_FLAGS?=

-APPCFG?= appcfg.py
+APPCFG?= $(SDK_PATH)/appcfg.py
APPCFG_FLAGS?=

PYTHON?= python2.5
COVERAGE?= coverage
-SDK_PATH?=
+

default: help

@@ -17,6 +19,8 @@
@echo "Available commands:"
@sed -n '/^[a-zA-Z0-9_.]*:/s/:.*//p' <Makefile | sort

+run: serve
+
serve: update_revision
@echo "---[Starting SDK AppEngine Server]---"
$(DEV_APPSERVER) $(DEV_APPSERVER_FLAGS) .

[albrec...@googlemail.com]

http://codereview.appspot.com/5651086/diff/1/Makefile
File Makefile (right):

http://codereview.appspot.com/5651086/diff/1/Makefile#newcode6
Makefile:6: DEV_APPSERVER?= $(SDK_PATH)/dev_appserver.py
This breaks setups where dev_appserver.py (and appcfg.py) is somewhere
in $PATH.

http://codereview.appspot.com/5651086/diff/1/Makefile#newcode22
Makefile:22: run: serve
Do we really need this extra alias?

http://codereview.appspot.com/5651086/

[tech...@gmail.com]

http://codereview.appspot.com/5651086/diff/1/Makefile
File Makefile (right):

http://codereview.appspot.com/5651086/diff/1/Makefile#newcode6
Makefile:6: DEV_APPSERVER?= $(SDK_PATH)/dev_appserver.py

On 2012/02/13 10:07:01, Andi Albrecht wrote:
> This breaks setups where dev_appserver.py (and appcfg.py) is somewhere
in $PATH.

If I remove '.' from the SDK_PATH, the DEV_APPSERVER will be
'/dev_appserver.py'. Do you know a way to output '/' only if SDK_PATH is
set in Makefile?

http://codereview.appspot.com/5651086/diff/1/Makefile#newcode22
Makefile:22: run: serve

On 2012/02/13 10:07:01, Andi Albrecht wrote:
> Do we really need this extra alias?

I'd rename to 'run', because the task semantically tied to this command
is to make Rietveld running, not 'served' or 'serving'. At least that is
what I thought of when adding instructions on OpenHatch page.

http://codereview.appspot.com/5651086/

[tech...@gmail.com]

http://codereview.appspot.com/5651086/

[tech...@gmail.com]

See the PatchSet no.2

http://codereview.appspot.com/5651086/diff/1/Makefile
File Makefile (right):

http://codereview.appspot.com/5651086/diff/1/Makefile#newcode6
Makefile:6: DEV_APPSERVER?= $(SDK_PATH)/dev_appserver.py

On 2012/02/13 10:12:56, techtonik wrote:
> On 2012/02/13 10:07:01, Andi Albrecht wrote:
> > This breaks setups where dev_appserver.py (and appcfg.py) is
somewhere in
> $PATH.

> If I remove '.' from the SDK_PATH, the DEV_APPSERVER will be
> '/dev_appserver.py'. Do you know a way to output '/' only if SDK_PATH
is set in
> Makefile?

Done.

http://codereview.appspot.com/5651086/

[albrec...@googlemail.com]

LGTM - but I'm not sure if this works for all make implementations.

http://codereview.appspot.com/5651086/

[Andi Albrecht]

On Mon, Feb 13, 2012 at 4:50 PM, <mar...@chromium.org> wrote:

> On 2012/02/13 11:21:02, Andi Albrecht wrote:
>>
>> LGTM - but I'm not sure if this works for all make implementations.
>
>

> ?
>
> I don' mind, lgtm.
> IMHO I'd prefer it to just look at ../google_appengine by default but
> that's me.

me too :)

>
> http://codereview.appspot.com/5651086/

[tech...@gmail.com]

I prefer to look for ../google_appengine by default too, but can't
immediately see [1] the way to provide compatibility for PATH setups
except rewriting Makefile with Python.

Anyway, committed the current fix. Thanks for review. =)

1.
http://sunsite.ualberta.ca/Documentation/Gnu/make-3.79/html_chapter/make_8.html#SEC77

http://codereview.appspot.com/5651086/

[tech...@gmail.com]

On 2012/02/13 19:00:26, M-A wrote:

> On 2012/02/13 18:57:42, techtonik wrote:
> > I prefer to look for ../google_appengine by default too, but can't
immediately
> > see [1] the way to provide compatibility for PATH setups except
rewriting
> > Makefile with Python.

> In general, adding the AppEngine SDK in PATH doesn't make sense for
most people.
> And if all rietveld devs prefer to have SDK_PATH prepopulated, it's
probably a
> good idea to just do it.

So I'm going to change the default value to ../google_appengine then.
Andi? =)

http://codereview.appspot.com/5651086/

[albrec...@googlemail.com]

I don't have a special opinion, "../google_appengine" is just where the
SDK lives in my setup. But in general I'm fine with how it is now.

http://codereview.appspot.com/5651086/

[tech...@gmail.com]

http://codereview.appspot.com/5651086/

[tech...@gmail.com]

Adding comment manuall, because -m upload.py option didn't work.
Reopening as I've learned some more Makefile spells. Please, check. The
only leftover is the sane error message when SDK is not found.

http://codereview.appspot.com/5651086/

[tech...@gmail.com]

Reopening the stale issue is a bad way to communicate. Closing.

http://codereview.appspot.com/5651086/