[rfc][icedtea-web] Makefile New Whitelisted-Dist-Test option

[Jiri Vanek]

On 09/23/2014 08:16 PM, Jie Kang wrote:
> ----- Original Message -----
>> >On 09/22/2014 11:17 PM, Jie Kang wrote:
>>> > >Hello,
>>> > >
>>> > >This patch expands on the Makefile Reproducer Test patch [1].
>>> > >
>>> > >[1]
>>> > >http://mail.openjdk.java.net/pipermail/distro-pkg-dev/2014-September/029581.html
>>> > >
>>> > >
>>> > >There is now a new make rule: 'run-netx-whitelisted-dist-tests'
>>> > >
>>> > >This rule runs the reproducers similar to 'run-netx-dist-tests', except it
>>> > >only processes (compiles, etc.) the resources filtered by the whitelist.
>>> > >
>>> > >As well, the rule 'run-netx-dist-tests' has now reverted back to processing
>>> > >all resources (ie. what it used to do before the Makefile Reproducer Test
>>> > >patch [1])
>>> > >
>>> > >Thoughts?
>>> > >
>>> > >Just a note, the purpose of the "stamps/whitelist-filter.stamp" rule
>>> > >having:
>>> > >    echo ".*" ...
>>> > >is to maintain compatibility. Though the "all-whitelist" rule also contains
>>> > >duplicate code, there can be situations where neither "all-whitelist" or
>>> > >"filtered-whitelist" are not called. E.g. if user runs "make
>>> > >stamps/netx-dist-tests-prepare-reproducers.stamp". This is the solution I
>>> > >came up with to allow for all possible make commands to continue to work
>>> > >and for the user to be able to quickly switch between running "make
>>> > >run-netx-dist-tests" and "make run-netx-whitelisted-dist-tests" without
>>> > >always having to perform "make clean-netx-dist-tests". I guess a TLDR is
>>> > >that it's to prevent regressions.
>>> > >
>>> > >If this patch is accepted I will update the wiki and documentation for this
>>> > >feature.
>>> > >
>>> > >
>>> > >Regards,
>>> > >
>> >
>> >Hm. only hint to approach. Why  not configure switch? This seems to me
>> >overcomplexed.  What is
>> >advantage of this?
> Hello,
>
> I have attached a completely new patch that follows your hint. Thanks a lot, it is much better now, and more simple.
> New option for configure: --enable-whitelist-processing
>
> With the flag on, the Makefile generated will filter by whitelist for processing, otherwise, it will process all, just like before patch [1].
>
> [1]http://mail.openjdk.java.net/pipermail/distro-pkg-dev/2014-September/029581.html
>
>> >Once the new and old targets will be run in unpredicted order, the result can
>> >be unpredictible.
> The complexity was to make sure running new and old targets in random order still worked hahah. And you could run them and stop them whenever, and still work properly like before too! But now no longer needed so...
>
>> >Also I can not see the reverted part in patch.
> There wasn't really any reversion in code, just reversion in behaviour. Anyways, no longer relevant.
>
>
> Thanks a lot!!
>
>
> Regards,
>
>> >
>> >J.
>> >
> -- Jie Kang
>
>
> itw-make-configure-whitelist-1.patch
>
>
> diff --git a/Makefile.am b/Makefile.am
> --- a/Makefile.am
> +++ b/Makefile.am
> @@ -178,6 +178,12 @@
>   endif
>   endif
>
> +if ENABLE_WHITELIST
> +WHITELIST=`cat $(REPRODUCERS_CLASS_WHITELIST)`
> +else
> +WHITELIST=.*
> +endif
> +
>   if WITH_RHINO
>     RHINO_TESTS=stamps/check-pac-functions.stamp
>   else
> @@ -691,26 +697,23 @@
>   	mkdir -p $(REPRODUCERS_BUILD_DIR)
>   	touch $@
>
> -junit-jnlp-dist-custom.txt: $(REPRODUCERS_CLASS_WHITELIST)
> +junit-jnlp-dist-custom.txt:
>   	cd $(REPRODUCERS_TESTS_SRCDIR)/$(CUSTOM_REPRODUCERS)/ ; \
> -	whiteListed=`cat $(REPRODUCERS_CLASS_WHITELIST)`; \
> -	for x in $$whiteListed ; do \
> +	for x in $(WHITELIST) ; do \
>   	  find .  -maxdepth 1 -mindepth 1 | sed "s/.\/*//" | grep $$x > $(abs_top_builddir)/$@ || true ; \
>   	done
>
> -junit-jnlp-dist-simple.txt: $(REPRODUCERS_CLASS_WHITELIST)
> +junit-jnlp-dist-simple.txt:
>   	cd $(REPRODUCERS_TESTS_SRCDIR)/simple/  ; \
> -	whiteListed=`cat $(REPRODUCERS_CLASS_WHITELIST)`; \
> -	for x in $$whiteListed ; do \
> +	for x in $(WHITELIST) ; do \
>   	  find .  -maxdepth 1 -mindepth 1 | sed "s/.\/*//" | grep $$x > $(abs_top_builddir)/$@ || true ; \
>   	done
>
> -stamps/junit-jnlp-dist-signed.stamp: $(REPRODUCERS_CLASS_WHITELIST)
> +stamps/junit-jnlp-dist-signed.stamp:
>   	types=($(SIGNED_REPRODUCERS)) ; \
>   	for which in "$${types[@]}" ; do \
>   	  pushd $(REPRODUCERS_TESTS_SRCDIR)/$$which/ ; \
> -	  whiteListed=`cat $(REPRODUCERS_CLASS_WHITELIST)`; \
> -	    for x in $$whiteListed ; do \
> +	    for x in $(WHITELIST) ; do \
>   	      find .  -maxdepth 1 -mindepth 1 | sed "s/.\/*//" | grep $$x > $(abs_top_builddir)/junit-jnlp-dist-$$which.txt ; \
>   	    done ; \
>   	  popd ; \
> diff --git a/configure.ac b/configure.ac
> --- a/configure.ac
> +++ b/configure.ac
> @@ -28,6 +28,14 @@
>   AM_CONDITIONAL([ENABLE_DOCS], [test x$ENABLE_DOCS = xyes])
>   AC_MSG_RESULT(${ENABLE_DOCS})
>
> +AC_MSG_CHECKING([whether to process reproducers using whitelist])
> +AC_ARG_ENABLE([whitelist-processing],
> +	      [AS_HELP_STRING([--enable-whitelist-processing],
> +	      		      [Enable processing of reproducers using whitelist])],

I would probably reflector those sentences. Even to me they are not much describing.
Try to debug those sentences  with somebody who never seen itw or our testuite :) Once he understood 
  the help message, it is right.

for ideas:
AC_MSG_CHECKING([whether to process reproducers using whitelist])
   - this will be nearly correct if you adapt my idea lower. No it imho is not, and should be like 
"whether to compile and deploy reproducers using filtering described whitelist"
AC_ARG_ENABLE([whitelist-processing],
	      [AS_HELP_STRING([--enable-whitelist-processing],
	      		      [Enable processing of reproducers using whitelist])],
Same:
"Enable compilation and deploy of reproducers using filtering described whitelist"

In this or in (below) suggested approach, I would like to mention all three targets (or more?) when 
the whitelist is used - compile, deploy run (some more?)
> +	      [ENABLE_WHITELIST="${enableval}"], [ENABLE_WHITELIST='no'])
> +AM_CONDITIONAL([ENABLE_WHITELIST], [test x$ENABLE_WHITELIST = xyes])
> +AC_MSG_RESULT(${ENABLE_WHITELIST})
> +
>   AC_PATH_PROG([BIN_BASH], [bash],, [/bin])
>   if test x"$BIN_BASH" = x ; then
>       AC_MSG_ERROR([/bin/bash is used in runtime and for about generation. Dying sooner rather then later])
>


Except this, looks ok.

Well - one general hint.

Right now, if ENABLE_WHITELIST is true, then:
  - only reprodcuers matching regeexes in list are compiled, deployed and run
if ENABLE_WHITELIST is false then:
  - all reprodcuers are compiled and deployed, but only  reprodcuers matching regeexes in whitelist 
are run.


I was thinking about to unifying it
  - only reprodcuers matching regeexes in list are compiled, deployed and run
if ENABLE_WHITELIST is false then:
  - all reprodcuers are compiled and deployed, and run


Well - this have question - what to do with current whitelist. To kept it in repo enad ampty? As it 
is? To rmeove? I probably incline to first one.

J.