Re: [Tarantool-patches] [PATCH luajit 1/3] test: remove excess dependency for tests target

[Igor Munkin via Tarantool-patches <tarantool-patches@dev.tarantool.org>]

Sergey,

Thanks for your review!

On 06.04.21, Sergey Kaplun wrote:
> Hi, Igor!
> 
> Thanks for the patch!
> 
> LGTM, except a few nits below.

Added your tag:
| Reviewed-by: Sergey Kaplun <skaplun@tarantool.org>

> 
> On 05.04.21, Igor Munkin wrote:
> 
> Nit: I propose the following commit title rewording:
> | test: remove excess dependency for tarantool suite
> Feel free to ignore.

I was thinking a lot about this commit subject and was so close to the
one you proposed, but I afraid it can misguide the reader: I tried to
emphasize, that we removed the *target prerequisite* (in terms of GNU
Make), but from yours it looks we removed the suite dependency. Changed
"dependency" to "prerequisite" to follow GNU Make terms.

> 
> > TEST_DEPS list was introduced to prevent <tarantool-tests> target from
> > being run in case no dependencies are changed. As a result of review it
> 
> Typo: s/result of review/result of the review/

Fixed.

> 
> > was decided to make all *-tests targets be .PHONY rules, hence TEST_DEPS
> 
> Typo? "be" looks excess here. I propose the following rewording

No, it's not, but changed to yours if it makes the sense clearer.

> | As a result of the review it was decided to turn all *-tests targets
> | into .PHONY rules, hence TEST_DEPS variable is excess and can be
> | dropped.

Here is the final wording:

================================================================================

test: remove excess prerequisite for tests target

TEST_DEPS list was introduced to prevent <tarantool-tests> target from
being run in case no dependencies are changed. As a result of the review
it was decided to turn all *-tests targets into .PHONY rules, hence
TEST_DEPS variable is excess and can be dropped.

Follows up tarantool/tarantool#4862

Reviewed-by: Sergey Kaplun <skaplun@tarantool.org>
Signed-off-by: Igor Munkin <imun@tarantool.org>

================================================================================

> 
> > variable is excess and can be dropped.
> > 
> > Follows up tarantool/tarantool#4862
> > 
> > Signed-off-by: Igor Munkin <imun@tarantool.org>
> > ---
> >  test/tarantool-tests/CMakeLists.txt | 3 +--
> >  1 file changed, 1 insertion(+), 2 deletions(-)
> > 
> > diff --git a/test/tarantool-tests/CMakeLists.txt b/test/tarantool-tests/CMakeLists.txt
> > index 9bb26f32..3e36ff86 100644
> > --- a/test/tarantool-tests/CMakeLists.txt
> > +++ b/test/tarantool-tests/CMakeLists.txt
> > @@ -69,7 +69,6 @@ set(LUA_PATH
> >  )
> >  set(LUA_TEST_SUFFIX .test.lua)
> 
> Nit: looks like `LUA_TEST_SUFFIX` variable is excess now, and we can
> move suffix ".test.lua" to `--ext` flag directly.

I don't think so. Considering your comment we need to drop every
variable that can be used in-place :)

> Feel free to ignore.

Ignoring.

> 

<snipped>

> > -- 
> > 2.25.0
> > 
> 
> -- 
> Best regards,
> Sergey Kaplun

-- 
Best regards,
IM