[rfc][icedtea-web] Makefile New Whitelisted-Dist-Test option
Jie Kang
jkang at redhat.com
Wed Oct 15 13:37:55 UTC 2014
----- Original Message -----
> On 10/15/2014 02:59 PM, Jie Kang wrote:
> >
> >
> > ----- 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?
>
>
> May yuu please describe littl ebit more detailed ? What option A and what
> option B?
Hello,
Sorry for lack of description.
I mean something like:
./configure = Option A : Compile all and run subset using whitelist on testcase name.
./configure --enable-whitelist-processing = Option B : Compile subset and run subset using whitelist on directory name.
Then we can switch between the two easily.
Thoughts?
>
>
--
Jie Kang
More information about the distro-pkg-dev
mailing list