Re: [PATCHv2] Makefile: add missing phony target

[Matthieu Moy <Matthieu.Moy@grenoble-inp.fr>]

Elia Pinto <gitter.spiros@gmail.com> writes:

> 2015-12-11 15:40 GMT+01:00 Matthieu Moy <Matthieu.Moy@grenoble-inp.fr>:
>> Elia Pinto <gitter.spiros@gmail.com> writes:
>>
>>> This is the second version of this patch.
>>> Added the corrections suggested by Matthieu Moy ($gmane/282221)
>>
>> Sorry, but my main concern was that the patch could not be reviewed in
>> good conditions as-is, and I think it still cannot be. It's very hard to
>> spot which .PHONY rules you're adding and which are just code movement.
>> You should really split this into one "code movement" patch and one
>> "actual bugfix" patch. Or someone with better eyes than me should review
>> the patch ;-).
>
> Ok. No problem. I thought there was no need for a patch so simple. But ok.

The point is: once a tricky bug was found in a patch (and I did on v1),
you cannot claim anymore that it is "so simple". If it was that simple,
you would have cought it before sending.

>>> @@ -2215,6 +2221,7 @@ test-svn-fe$X: vcs-svn/lib.a
>>>  test-%$X: test-%.o GIT-LDFLAGS $(GITLIBS)
>>>       $(QUIET_LINK)$(CC) $(ALL_CFLAGS) -o $@ $(ALL_LDFLAGS) $(filter %.o,$^) $(filter %.a,$^) $(LIBS)
>>>
>>> +.PHONY: check_sha1 $(SP_OBJ) sparse check check-sha1
>>>  check-sha1:: test-sha1$X
>>>       ./test-sha1.sh
>>>
>>> @@ -2224,7 +2231,6 @@ $(SP_OBJ): %.sp: %.c GIT-CFLAGS FORCE
>>>       $(QUIET_SP)cgcc -no-compile $(ALL_CFLAGS) $(EXTRA_CPPFLAGS) \
>>>               $(SPARSE_FLAGS) $<
>>>
>>> -.PHONY: sparse $(SP_OBJ)
>>>  sparse: $(SP_OBJ)
>>
>> This "sparse" movement looks again contradictory with the goal announced
>> in the commit message.
> The idea was to group all the phony before all the target, not to put
> the phony necessarily before the closest target. but ok

I personally prefer the old way. I have no strong objection to changing,
but currently your commit message says "Also put the .PHONY declaration
immediately before the target declaration", which is clearly not a
justification to do this change.

-- 
Matthieu Moy
http://www-verimag.imag.fr/~moy/