[rfc][icedtea-web] Makefile New Whitelisted-Dist-Test option
Jie Kang
jkang at redhat.com
Wed Oct 15 12:59:57 UTC 2014
----- Original Message -----
> On 10/10/2014 03:04 PM, Jie Kang wrote:
> >
> >
> > ----- Original Message -----
> >> On 09/26/2014 08:16 PM, Jie Kang wrote:
> >>>
> >>>
> >>> ----- 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.;
> >>>
> >>>
> >>>
> >>
> >> Hi!
> >>
> >> I have realized an issue with all this filtering.
> >> The whitelist is regex on testcase.java files. But for the rest, it seems
> >> to
> >> be match on directory
> >> file. Is there any option, different then extend regex to match dir or
> >> testcase?
> >>
> >> /me hoping tomiss something
> >>
> >> Imho the correct solution would be
> >> a) if tetscase matches, then include all reproducers other files
> >> or
> >> b)if dir matches include testcase and all other reprodcuer's files
> >>
> >> I dont like neither :(
> >
> >
> > Hello,
> >
> > Before reproducers are made by
> > 1. Creating list of reproducer directories in text file
> > (junit-jnlp-dist-simple.txt, junit-jnlp-dist-custom.txt, ...)
> > 2. Going through list, processing the directories (compile, move resource
> > files, etc.)
> > 3. Finally using whitelist on the java files of the processed reproducers
> > to create list of tests to run. And then run what's in the list.
> >
> > The current HEAD has made it so whitelist is applied to the list of
> > directories in step one. This means that whitelist must be regex on
> > directory and testcase. I think if we choose to whitelist on directory, we
> > don't whitelist on java files and if we choose to whitelist on java files,
> > we don't whitelist on directory. Alternatively, we have two different
> > whitelists... (bad idea).
> >
> >
> > However: Note that this proposed patch, makes it so:
> >
> > OLD behaviour is back to HEAD.
> >
> > NEW behaviour is when --enable-whitelist-processing is supplied to
> > ./configure
> >
> >
> > This way, nothing gets changed unless you use: ./configure
> > --enable-whitelist-processing
> > I think this is good. Thoughts?
> >
> >
> > Regards,
> >
> >>
> >>
> >> J.
> >>
> >>
> >
>
> One more thing to think: My quite often usecase is - lets all reproducers are
> prepared, and then I'm
> Modifying the whitelist, and running always some subset of tests.
> I would like not to lsot this usecase:(
>
>
> As from our IRC converasation - directory x tetscase name issues (+
> tests-extensions tests issues)
>
> Any progress?
>
>
> More thingking about this, more I'm inclined to have two lists:(( One for
> compilation - based on
> directory name (but all testcases compiled), one for run - based on testcase
> name. But I dont like
> two lists. ANy more ideas??
Hello,
Atm I still think having two options in ./configure is okay.
Option A : Compile all and run subset using whitelist on testcase name. (original behaviour)
Option B : Compile subset and run subset using whitelist on directory name.
What drawbacks do you see here? Using ./configure is really small amount of time and is easy to switch from A to B, no?
Regards,
>
>
> J.
>
>
--
Jie Kang
More information about the distro-pkg-dev
mailing list