[0/3] validation: changing module random

On Tue, May 12, 2015 at 09:02:18AM +0200, Christophe Milard wrote:
> On 11 May 2015 at 20:23, Stuart Haslam <stuart.haslam@linaro.org> wrote:
> 
> > On Mon, May 11, 2015 at 12:48:24PM +0200, Christophe Milard wrote:
> > > Hopefully according to the agreement of last week.
> > > The changes are applyed to module random, one of the simplest.
> > > Please review carefully as all modules will go through the same changes
> > > when this goes through...
> > > After this patch, at this stage, the tests are still ran from the
> > > validation side so as to keep the make check as usual.
> > > The plan is to call all the executable from the platform side in one go
> > > in a final patch, when all the modules are ready.
> > >
> > > Christophe Milard (3):
> > >   validation: preparing for main in tests
> > >   validation: own main and renaming in odp_random.c
> > >   validation: creating own dir and lib for random
> > >
> >
> > This patch set is a bit confusing, in patch 2 you make changes in
> > preparation for something else that may be done "one day" but then go
> > ahead and do it in patch 3. I would rather just see the end result.
> >
> 
> No, I don't go ahead:  I do create the lib, indeed, but none of the renamed
> things (except the main program) actually get exported in the lib (they
> remain static).

Understood, but you added some code in patch 2 which I was going to
comment on before noticing that it was removed in patch 3.

> I still think it is important with the renaming to achieve
> naming uniformity for clearer tests and future-proofness in case we do have
> to export more in the lib.
> Having a lib containing just the main gives a chance for platform which
> needs it to do the test setup in C before calling the test, from the lib,
> in C. (thus allowing for both script and/or C platform side test setup)

I agree, and this sort of thing should be in the commit message to
explaining why the change is needed rather that just what is being
changed.

> > Apart from that, the series isn't actually *fixing* anything, so the
> > patch descriptions say what you're changing but not why, and without the
> > context of future similar changes this just looks like churn. I know
> > you've picked the simplest module to start with, but I think it would be
> > better to start out with a module that has a problem which this
> > restructuring fixes (e.g. pktio), then go from there applying the same
> > structure to other modules.
> >
> >
> I see your point here, but I don't know what to do: to actually fix
> something, like pktio, the test will have to be ran from the platform side
> (that is when we can actually clean-up the pktio things in validation): But
> then, during the transition phase, the "make check" results would be split
> in 2 (results from tests running from platform side vs results from those
> running the old way, from validation side). So a complete patch for pktio
> (running from the platform side) would probably be rejected because it
> would make the "make check" result more confusing.

Based on what we discussed last week I was expecting a first patch that
simply moves the TESTS definition from test/validation/Makefile.am and
into platform/<platform>/test/Makefile.am (something like, but not
exactly, the diff below). Is this not what you had in mind?

> And sending a series of patch taking all modules in one go does not feel
> right either if we haven't agreed on one. Hence this patch.

Given that this is a restructure with the promise of making future
changes easier/possible (these changes alone don't fix anything), and
it's intended to be applied to all modules, I think agreeing on the
changes being applied to one module is basically the same as agreeing to
them for all modules.

> So I really wonder whether the approach taken here (agreeing on one module
> before taking all other, one by one, and eventually run from the platform
> side -all in one go to avoid make check result confusion-) isn't the best...
> Do you really think that having a pktio patch and split "make check"
> results would have more change to be accepted
> 
> And thanks for taking the time to look at it!!
> 
> Christophe.

On 12 May 2015 at 19:28, Stuart Haslam <stuart.haslam@linaro.org> wrote:

> On Tue, May 12, 2015 at 09:02:18AM +0200, Christophe Milard wrote:
> > On 11 May 2015 at 20:23, Stuart Haslam <stuart.haslam@linaro.org> wrote:
> >
> > > On Mon, May 11, 2015 at 12:48:24PM +0200, Christophe Milard wrote:
> > > > Hopefully according to the agreement of last week.
> > > > The changes are applyed to module random, one of the simplest.
> > > > Please review carefully as all modules will go through the same
> changes
> > > > when this goes through...
> > > > After this patch, at this stage, the tests are still ran from the
> > > > validation side so as to keep the make check as usual.
> > > > The plan is to call all the executable from the platform side in one
> go
> > > > in a final patch, when all the modules are ready.
> > > >
> > > > Christophe Milard (3):
> > > >   validation: preparing for main in tests
> > > >   validation: own main and renaming in odp_random.c
> > > >   validation: creating own dir and lib for random
> > > >
> > >
> > > This patch set is a bit confusing, in patch 2 you make changes in
> > > preparation for something else that may be done "one day" but then go
> > > ahead and do it in patch 3. I would rather just see the end result.
> > >
> >
> > No, I don't go ahead:  I do create the lib, indeed, but none of the
> renamed
> > things (except the main program) actually get exported in the lib (they
> > remain static).
>
> Understood, but you added some code in patch 2 which I was going to
> comment on before noticing that it was removed in patch 3.
>

I assume you men the main() function. It is there so that patch 2 still
works. It is commented as
" the following main function will be separated when lib is created".
I could squash patsh 2 & 3, of course, but then the renaming aspect (for
possible lib export) and the
separation in private directories would be gathered. Is this what you
prefer??


> > I still think it is important with the renaming to achieve
> > naming uniformity for clearer tests and future-proofness in case we do
> have
> > to export more in the lib.
> > Having a lib containing just the main gives a chance for platform which
> > needs it to do the test setup in C before calling the test, from the lib,
> > in C. (thus allowing for both script and/or C platform side test setup)
>
> I agree, and this sort of thing should be in the commit message to
> explaining why the change is needed rather that just what is being
> changed.
>

I will update the commit message.


>
> > > Apart from that, the series isn't actually *fixing* anything, so the
> > > patch descriptions say what you're changing but not why, and without
> the
> > > context of future similar changes this just looks like churn. I know
> > > you've picked the simplest module to start with, but I think it would
> be
> > > better to start out with a module that has a problem which this
> > > restructuring fixes (e.g. pktio), then go from there applying the same
> > > structure to other modules.
> > >
> > >
> > I see your point here, but I don't know what to do: to actually fix
> > something, like pktio, the test will have to be ran from the platform
> side
> > (that is when we can actually clean-up the pktio things in validation):
> But
> > then, during the transition phase, the "make check" results would be
> split
> > in 2 (results from tests running from platform side vs results from those
> > running the old way, from validation side). So a complete patch for pktio
> > (running from the platform side) would probably be rejected because it
> > would make the "make check" result more confusing.
>
> Based on what we discussed last week I was expecting a first patch that
> simply moves the TESTS definition from test/validation/Makefile.am and
> into platform/<platform>/test/Makefile.am (something like, but not
> exactly, the diff below). Is this not what you had in mind?
>

Yes, this is definitively the goal. I though I would "reorganise" each
module before "pointing" to them from  the platform side, thus avoiding to
point -even if temporary-  to wrong executables (read: meant to change).
So my intention was to reorganize each module (renaming test, suites and
main and creating the libs). and then to change  to the platform side.


> > And sending a series of patch taking all modules in one go does not feel
> > right either if we haven't agreed on one. Hence this patch.
>
> Given that this is a restructure with the promise of making future
> changes easier/possible (these changes alone don't fix anything), and
> it's intended to be applied to all modules, I think agreeing on the
> changes being applied to one module is basically the same as agreeing to
> them for all modules.
>

Here I am getting a bit puzzled... Your first wish was to get a patch for
pktio (I am working on that), then I understood you'd prefer a first patch
moving check TESTS from validation/makefile to <platform>/test/makefile
(above). Are you now saying you'd like everything (including all modules)
in one go?
You are right on "agreeing on the changes being applied to one module is
basically the same as agreeing to them for all modules", but I definitely
would prefer a agreement on one module before fixed them all. Seems we can
save time there starting with one...

Thanks for your comments,
Christophe.


> > So I really wonder whether the approach taken here (agreeing on one
> module
> > before taking all other, one by one, and eventually run from the platform
> > side -all in one go to avoid make check result confusion-) isn't the
> best...
> > Do you really think that having a pktio patch and split "make check"
> > results would have more change to be accepted
> >
> > And thanks for taking the time to look at it!!
> >
> > Christophe.
>
> --
> Stuart.
>
> diff --git a/platform/linux-generic/test/Makefile.am
> b/platform/linux-generic/test/Makefile.am
> index 91e361c..a54ccf1 100644
> --- a/platform/linux-generic/test/Makefile.am
> +++ b/platform/linux-generic/test/Makefile.am
> @@ -1 +1,23 @@
>  dist_bin_SCRIPTS = $(srcdir)/pktio_env
> +
> +COMMON_TEST_DIR=../../../test/validation
> +
> +TESTS = $(COMMON_TEST_DIR)/odp_buffer \
> +       $(COMMON_TEST_DIR)/odp_classification \
> +       $(COMMON_TEST_DIR)/odp_cpumask \
> +       $(COMMON_TEST_DIR)/odp_crypto \
> +       $(COMMON_TEST_DIR)/odp_init \
> +       $(COMMON_TEST_DIR)/odp_init_abort \
> +       $(COMMON_TEST_DIR)/odp_init_log \
> +       $(COMMON_TEST_DIR)/odp_packet \
> +       odp_pktio_run \
> +       $(COMMON_TEST_DIR)/odp_pool \
> +       $(COMMON_TEST_DIR)/odp_queue \
> +       $(COMMON_TEST_DIR)/odp_random \
> +       $(COMMON_TEST_DIR)/odp_scheduler \
> +       $(COMMON_TEST_DIR)/odp_shared_memory \
> +       $(COMMON_TEST_DIR)/odp_synchronizers \
> +       $(COMMON_TEST_DIR)/odp_time \
> +       $(COMMON_TEST_DIR)/odp_timer \
> +       $(COMMON_TEST_DIR)/odp_thread \
> +       $(COMMON_TEST_DIR)/odp_ver_abt_log_dbg
> diff --git a/test/validation/odp_pktio_run
> b/platform/linux-generic/test/odp_pktio_run
> similarity index 100%
> rename from test/validation/odp_pktio_run
> rename to platform/linux-generic/test/odp_pktio_run
> diff --git a/test/validation/Makefile.am b/test/validation/Makefile.am
> index 7470d9d..0962c3e 100644
> --- a/test/validation/Makefile.am
> +++ b/test/validation/Makefile.am
> @@ -13,6 +13,7 @@ EXECUTABLES = odp_buffer \
>               odp_init_abort \
>               odp_init_log \
>               odp_packet \
> +             odp_pktio \
>               odp_pool \
>               odp_queue \
>               odp_random \
> @@ -24,17 +25,7 @@ EXECUTABLES = odp_buffer \
>               odp_thread \
>               odp_ver_abt_log_dbg
>
> -COMPILE_ONLY = odp_pktio
> -
> -TESTSCRIPTS = odp_pktio_run
> -
> -if test_vald
> -TESTS = $(EXECUTABLES) $(TESTSCRIPTS)
> -endif
> -
> -dist_bin_SCRIPTS = odp_pktio_run
> -
> -bin_PROGRAMS = $(EXECUTABLES) $(COMPILE_ONLY)
> +bin_PROGRAMS = $(EXECUTABLES)
>
>  ODP_CU_COMMON=common/odp_cunit_common.c
>
>
>