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

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

[Andi Albrecht, 2012-02-13 10:07:01]

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?

[techtonik, 2012-02-13 10:12:56]

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.

[techtonik, 2012-02-13 10:32:09]

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.

[Andi Albrecht, 2012-02-13 11:21:02]

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

[M-A, 2012-02-13 15:50:00]

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.

[Andi Albrecht, 2012-02-13 15:55:23]

On Mon, Feb 13, 2012 at 4:50 PM,  <maruel@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/

[techtonik, 2012-02-13 18:57:42]

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.ht...

[M-A, 2012-02-13 19:00:26]

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.

[techtonik, 2012-02-14 06:09:42]

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? =)

[Andi Albrecht, 2012-02-15 20:17:03]

On 2012/02/14 06:09:42, techtonik wrote:
> 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? =)

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.

[techtonik, 2012-02-16 07:31:33]

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.

[techtonik, 2012-02-18 08:00:08]

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