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

Jiri Vanek jvanek at redhat.com
Wed Oct 15 13:28:35 UTC 2014


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?



More information about the distro-pkg-dev mailing list