[rfc][icedtea-web] Makefile New Whitelisted-Dist-Test option
Jie Kang
jkang at redhat.com
Fri Sep 26 18:16:27 UTC 2014
----- Original Message -----
> 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?)
Hello,
I've altered the messages to explain the purpose better. Does it look okay to you?
+AC_MSG_CHECKING([whether to filter by whitelist when processing, compiling and running reproducers])
+AC_ARG_ENABLE([whitelist-processing],
+ [AS_HELP_STRING([--enable-whitelist-processing],
+ [Enable whitelist filter when processing, compiling and running reproducers])],
> > + [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
>
I think this is a good idea as well. The attached patch does this. Can you check to make sure it looks okay? (changes start at "-$(REPRODUCERS_CLASS_NAMES): $(REPRODUCERS_CLASS_WHITELIST)")
Or should this be in another changeset?
>
> 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.
I think keeping it as is fine. Having the ".*" is good. Making it empty might make it confusing as to why it is in repo..;
If we want to remove it I think we'd need a patch to let user supply the whitelist file location since we can't guarantee it exists with the same name anymore.;
Regards,
>
> J.
>
--
Jie Kang
-------------- next part --------------
A non-text attachment was scrubbed...
Name: itw-make-configure-whitelist-2.patch
Type: text/x-patch
Size: 3585 bytes
Desc: not available
URL: <http://mail.openjdk.java.net/pipermail/distro-pkg-dev/attachments/20140926/514ef24f/itw-make-configure-whitelist-2-0001.patch>
More information about the distro-pkg-dev
mailing list